1. Java / Говнокод #11481

    +100

    1. 1
    2. 2
    3. 3
    synchronized(new Object()){
    ...
    }

    http://stackoverflow.com/questions/9840959/how-to-judge-which-object-to-be-synchronized-in-java-thread

    Запостил: 3.14159265, 27 Июля 2012

    Комментарии (16) RSS

    • Кстати раз уж все плюсуют, но никто не хочет комментировать, выскажусь я.
      Писал как-то раз такую синхронизацию в жабе. Сознательно писал.
      Ибо нужно было дублировать 2 куска кода - один для несинхронизированных инстансов (завернутый в synchronized), другой для синхронизированных (никуда не завернутый).

      Вот так:
      Object mutex=
      	(		map instanceof ConcurrentSkipListMap 
      		|| 	map instanceof ConcurrentHashMap 
      		|| 	map instanceof Hashtable
      	) ? new Object() //already sync
      	: map	
      	;
      synchronized (mutex) {
         <duplicated code>
      }
      Ответить
      • А почему бы не пользоваться декоратором с синхронизацией, вроде Collections.synchronizedMap() ?
        Ответить
        • Edit:
          >Collections.synchronizedMap()
          Кстати это мысль. Но только заворачивать в него несинхронизированные.

          Но как пример подход где можно использовать new Object(), вполне ведь легитимно?
          Ответить
          • > заворачивать в него несинхронизированные
            Вот-вот. Это позволит ускорить код, ведь не будет медленного захвата монитора для никому не нужного обжекта.

            new Object() можно, можно ещё Lock явно заюзать. Кода больше, но намерения более явные, и можно добиться условной блокировки.
            Ответить
            • C локом я что-то наврал, нам ведь мапу засинкать надо, а не блок.
              Кстати, если блок состоит из нескольких операций над мапой, для конкурентных мап появлятся race condition.
              Ответить
              • Ну да. Я написал и потом удолил.
                Там в некоторых других местах нужен именно atomic, потому тупо лок на мапе и проверка уже на ConcurrentMap.

                > не будет медленного захвата монитора для никому не нужного обжекта.
                Лажа в том что для synchronizedMap() тоже создается ненужный объект - обертка. Для GC однохуйственно.
                Но теперь на любом вызове метода get, put, contains будет происходть "медленный захват монитора".
                Ведь внутри synchronizedMap() те же секции synchronized.
                Ответить
                • ну просто мне вот что не совсем понятно: если нужна атомарная операция (putIfAbsent), то для СoncurrentMap проще вызвать родную реализацию, а для обычных мап нужно писать версию с synchronized с несколькими операциями, код уже не <duplicated>.
                  Если нужно сымитировать именно базовые операции из ConcurrentMap для обычных, я бы обошёлся обёрткой, делающий из обычной мапы ConcurrentMap с внутренней синхронизацией.
                  Если нужны более сложные операции, то для конкурентных коллекций всё равно потребуется синхронизация, ибо
                  if (!concurrentMap.contains(key)) {
                    concurrentMap.put(key, value);
                  }
                  создаёт race condition, хоть и не сломает саму мапу (как может приключиться с обычной мапой).
                  Ответить
                  • Да. Всё правильно. Я с этим не спорю.
                    > потому тупо лок на мапе и проверка уже на ConcurrentMap
                    Я об этом и написал. Проверка на ConcurrentMap, каст в неё и вызов того же putIfAbsent.

                    Просто говорю, что
                    а) от медленного захвата монитора никуда не уйти
                    б) в случае когда нет готовой синхронизированной обертки, а писать её лень лок по new Object() - самый простой и эффективный подход. По GC выигрыша нет. И там, и там создается объект.

                    >вот что не совсем понятно: если нужна атомарная операция
                    Она нужна в другом куске кода.
                    Ответить
                    • ну, я бы написал так
                      if (map instanceof ConcurrentMap) {
                        ((ConcurrentMap) map).putIfAbsent(key, value);
                      } else {
                        synchronized (map) {
                          if (!map.contains(key)) {
                            map.put(key, value);
                          }
                        }
                      }
                      Если конкурентная мапа, не лочим временных объектов, да и логика прозрачней.
                      Ответить
                      • Ром, мы вроде говорим о одном и том же:
                        >>Проверка на ConcurrentMap, каст в неё и вызов того же putIfAbsent.

                        Но это в другом месте, где нужна неделимость операций.

                        Это, по-моему, единственный здравый способ. Как-то по-другому написать это трудно.
                        Ответить
                        • а, до меня дошло. В <duplicated code> не используются никакие операции, требующие атомарности, только простой набор независимых изменений (что-то вроде последовательных добавлений), и требуется не поломать мапу. Ну, тогда new Object() - интересный хак :)

                          synchronizedMap() позволит лишь избежать лишнего захвата монитора для конкурентных мап, но привнесёт несколько захватов для неконкурентных. К тому же, его нельзя использовать, если мапа доступна извне, что скорее всего верно.
                          Ответить
                          • Угу. Точно так.
                            А в целом это увиливание от жавопроблем - в т.ч. плохого метода synchronizedMap, который тупо заворачивает всё. В этом плане чистые языки думаю избавят нас от лютого многопоточного майндфака.
                            Ответить
              • Вот, например
                V val =   map.       //enterMonitor
                get(key);//exitMonitor
                if (null==val) 
                	map.
                //enterMonitor
                put(key,newVal); //exitMonitor

                Только внутри sync-mapa
                Ответить
      • кстати, можно ещё так:
        private void doUpdate(Map map, Extra params) { /* ... */ }
        
        public void doPublic(Map map, Extra params) {
          if (map instanceof ConcurrentMap) {
            doUpdate(map, params);
          } else {
            synchronized (map) { doUpdate(map, params); }
          }
          /* ... */
        }
        Ответить
        • Та я думал над таким, но если параметров много, заполнение Extra потребует много строчек унылого кода.
          Ответить

    Добавить комментарий