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

    +72

    1. 01
    2. 02
    3. 03
    4. 04
    5. 05
    6. 06
    7. 07
    8. 08
    9. 09
    10. 10
    11. 11
    12. 12
    13. 13
    14. 14
    15. 15
    16. 16
    17. 17
    18. 18
    19. 19
    20. 20
    21. 21
    22. 22
    23. 23
    24. 24
    25. 25
    26. 26
    27. 27
    28. 28
    29. 29
    30. 30
    31. 31
    32. 32
    33. 33
    34. 34
    35. 35
    36. 36
    37. 37
    38. 38
    39. 39
    40. 40
    41. 41
    42. 42
    43. 43
    44. 44
    45. 45
    46. 46
    47. 47
    48. 48
    49. 49
    50. 50
    51. 51
    52. 52
    53. 53
    54. 54
    55. 55
    56. 56
    57. 57
    58. 58
    59. 59
    60. 60
    61. 61
    62. 62
    63. 63
    64. 64
    65. 65
    66. 66
    67. 67
    68. 68
    69. 69
    70. 70
    71. 71
    72. 72
    73. 73
    74. 74
    75. 75
    76. 76
    77. 77
    78. 78
    79. 79
    80. 80
    public class PLock {
        private Map<Thread, Integer> readLocks = new HashMap<Thread, Integer>();
        private Thread writeLock = null;
        private int writeLockCount = 0;
        
        public synchronized void getReadLock() {
            Thread currentThread = Thread.currentThread();
            long startTimeMillis = System.currentTimeMillis();
            boolean gotStuck = false;
            while (canClaimReadLock(currentThread) == false) {
                gotStuck = true;
                try {
                    wait();
                } catch (InterruptedException ex) {
                    Log.warn("Interrupted while attempting to get read lock.", ex);
                }
            }
            report(gotStuck, startTimeMillis, "read");
            if (readLocks.containsKey(currentThread)) {
                readLocks.put(currentThread, 1 + readLocks.get(currentThread));
            } else {
                readLocks.put(currentThread, 1);
            }
        }
        
        ...
        
        public synchronized void relinquishReadLock() {
            Thread currentThread = Thread.currentThread();
            if (readLocks.containsKey(currentThread) == false) {
                throw new RuntimeException("Cannot relinquish read lock on thread " + currentThread + " because it does not hold a lock.");
            }
            int newLockCount = readLocks.get(currentThread) - 1;
            if (newLockCount == 0) {
                readLocks.remove(currentThread);
                notifyAll();  // IMPORTANT: allow other threads to wake up and check if they can get locks now.
            } else {
                readLocks.put(currentThread, newLockCount);
            }
        }
        
        public synchronized void getWriteLock() {
            //Log.warn("getWriteLock() in thread " + Thread.currentThread());
            //dumpLocks();
            Thread currentThread = Thread.currentThread();
            long startTimeMillis = System.currentTimeMillis();
            boolean gotStuck = false;
            while (canClaimWriteLock(currentThread) == false) {
                gotStuck = true;
                try {
                    wait();
                } catch (InterruptedException ex) {
                    Log.warn("Interrupted while attempting to get write lock.", ex);
                }
            }
            report(gotStuck, startTimeMillis, "write");
            writeLock = currentThread;
            writeLockCount++;
        }
        
        ...
        
        public synchronized void relinquishWriteLock() {
            //Log.warn("relinquishWriteLock() in thread " + Thread.currentThread());
            Thread currentThread = Thread.currentThread();
            if (writeLock != currentThread) {
                throw new RuntimeException("Cannot relinquish write lock on thread " + currentThread + " because it does not hold the lock.");
            }
            if (writeLockCount <= 0) {
                throw new RuntimeException("Tried to relinquish write lock on thread " + currentThread + " while write lock count is " + writeLockCount);
            }
            writeLockCount--;
            if (writeLockCount == 0) {
                writeLock = null;
                notifyAll();  // IMPORTANT: allow other threads to wake up and check if they can get locks now.
            }
        }
        
        ...
    }

    А у вас уже есть свой теплый ламповый ReentrantReadWriteLock?
    Нет? Тогда https://github.com/orph/jujitsu/blob/master/src/e/ptextarea/PLock.java идет к вам...

    Запостил: kostoprav, 30 Мая 2014

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

    • > catch (InterruptedException ex)
      Супер. Нефиг треду знать, что его хотели тормознуть.
      Ответить
    • > while (canClaimReadLock(currentThread) == false) {
      У нас на первом курсе препод за такое замечания делал.
      Ответить
      • Это не самое страшное в этом коде, поверь.
        Более того даже простительно если код нормальный(м.б. кодстайл такой. не все ! замечают, NOTа на них нет)
        Это мелочь. Вот за деревьями леса не видишь.
        Ответить
        • Я в остальное не вникал, а это сразу выдает первокурсника.
          Ответить
          • >Я в остальное не вникал, а это сразу выдает первокурсника.
            На самом деле сомневаюсь что это писал первокурсник.
            > on Aug 24, 2007 Initial Import.
            Если это какой-то древний код написанный еще до 1.5 (на что намекает многое, тот же System.currentTimeMillis), то вполне сойдет. Небрежно конечно написано, но так вроде всё на месте.
            Ответить
            • 1.5 все таки в 2004 появился, за 3-то года можно и поправить. А вообще - черт его знает... Хотя вроде как generics я там видел.
              Ответить
              • Так гитхаб собственно появился в 2007. И то типа initial import в гит. Код мог быть написан раньше.
                Косвенные признаки, которые я сразу счёл говном - не имплементим интерфейс Lock, не используем nanoTime, атомики и волатилы идут лесом - только synchronized, только хардкор.
                А вот если предположить что писалось для 1.4 и раньше - то всё стаёт на свои места и наверное единственный весомый факап, как отметил борманд
                > catch (InterruptedException ex)
                Еще и логгер стандартный - уже давно не пишут так.

                >>Map<Thread, Integer>
                Могли допилить позже, когда исправляли ворнинги.
                Ответить
    • > public synchronized void getReadLock() {
      Пиздец. Сэкономил называется. Какая-то совершенно безумная premature optimization.
      Типа много читателей один писатель, и в то же время всё работает в страшно однопоточном режиме.
      Ответить
      • > и в то же время всё работает в страшно однопоточном режиме
        Ну все же не совсем в однопоточном. Монитор захватывается только на время взятия и отпускания R/W лока, в остальное время он отпущен. Поэтому если R/W лок берут надолго - может быть и потянет.
        Ответить
      • Зато тут, походу, у writer'а нет преимущества, и хуй он дождется своей очереди, если reader'ов много, и все они хотят читать ;)
        Ответить
        • >Зато тут, походу, у writer'а нет преимущества
          Нет. Но это не фатально. Так же устроены и Collections.synchronized...(). Работают как-то ведь.

          >и хуй он дождется своей очереди,
          Встроенную лочку не дураки писали, там очередь стоит. Правда не она всегда совсем уж честная.
          Но тут вообще вероятность не на стороне writera.
          Ответить
          • > очередь
            > вероятность
            Не поможет же, хоть честная, хоть не честная. И вероятность его не спасет. Если reader'ов много, и нет таких моментов, когда все они отпустили лок (а это вполне нормальная ситуация для R/W lock'а, ради нее он и запилен) - canClaimWriteLock() никогда не вернет true, и writer соснет хуйца.

            Нормальный R/W лок, емнип, обламывает reader'ов и ставит их в очередь если в ней есть writer. Тем самым writer получает преимущество.
            Ответить
            • С одной стороны действительно так, если canClaimReadLock/canClaimWriteLock написаны как мы ожидаем - максимально тупо
              А с другой там может быть любое безумие. canClaimReadLock может возвращать false, если есть writer который дохера ждёт. Там вот startTimeMillis есть.

              relinquishReadLock
              Ответить
              • > startTimeMillis
                Дык она локальная.

                Еще предположение: canClaimWriteLock может взводить некий флаг, который заставляет canClaimReadLock возвращать false. Вот только кто его сбросит?
                Ответить
                • >Нормальный R/W лок, емнип, обламывает reader'ов и ставит их в очередь если в ней есть writer.
                  Ну тогда постоянные writerы будут обламывать readerы, что так же плохо.
                  >Вот только кто его сбросит?
                  При освобождении writeLock возле notify.

                  На самом деле там много разных стратегий есть. Смотреть по времени ожидания - это и есть честная.
                  Если например суммарное время ожидания читателей больше чем писателя, то дать им. Если наоборот - писать.

                  А там еще есть апгрейд лочки, если например есть ридер, который решил что-то писать. В общем целая наука.
                  Ответить
                  • > Ну тогда постоянные writerы будут обламывать readerы, что тоже не всегда хорошо.
                    Ну все-таки, имхо, R/W лок выручает как раз когда writer работает достаточно редко, а reader'ов дохуя и больше, и захватывают лок они на достаточно большое время... Так что потерпят.

                    > На самом деле там много разных стратегий есть.
                    +1
                    Ответить
            • Таки да:
              private boolean canClaimWriteLock(Thread currentThread) {
                      if (writeLock == currentThread) {
                          return true;
                      } else if (writeLock != null) {
                          return false;
                      } else if (readLocks.size() == 0) {
                          return true;
                      } else if (readLocks.size() > 1) {
                          return false;
                      } else {
                          return readLocks.containsKey(currentThread);
                      }
                  }

              но даже ReentrantReadWriteLock старадает от этого
              When constructed as non-fair (the default), the order of entry to the read and write lock is unspecified, subject to reentrancy constraints. A nonfair lock that is continuously contended may indefinitely postpone one or more reader or writer threads, but will normally have higher throughput than a fair lock.
              Ответить
              • Последняя ветка это апгрейд с R на W?
                Ответить
                • Угу. Типа если 1 читатель, и это мы, то можно хватать на запись.
                  А существует и даунгрейд, типа освободить W и взять R не дожидаясь готовности.
                  Ответить
    • >свой теплый ламповый ReentrantReadWriteLock?
      На самом деле он не всегда хорош, часто обычная лочка его уделывает.
      Вот StampedLock - отдельный разоговор.
      Ответить

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