Является ли этот incrementAndGet потокобезопасным? Кажется, он вытаскивает объект из кеша

Этот сервлет, кажется, извлекает объект из ehCache из элемента, который имеет этот объект: http://code.google.com/p/adwhirl/source/browse/src/obj/HitObject.java?repo=servers-mobile

Затем он продолжает увеличивать счетчик, который является атомарным длинным:

http://code.google.com/p/adwhirl/source/browse/src/servlet/MetricsServlet.java?repo=servers-mobile#174

    //Atomically record the hit
    if(i_hitType == AdWhirlUtil.HITTYPE.IMPRESSION.ordinal()) {
        ho.impressions.incrementAndGet();
    }
    else {
        ho.clicks.incrementAndGet();
    }

Мне это не кажется потокобезопасным, поскольку несколько потоков могут извлекаться из кеша, и если оба увеличиваются одновременно, вы можете потерять количество кликов/показов.

Вы согласны с тем, что это не потокобезопасно?


java multithreading concurrency ehcache
person codecompleting    schedule 19.12.2011    source источник
comment
Инкрементная часть довольно безопасна. Способ извлечения и хранения объекта ho может иметь некоторые проблемы, что, кстати, указано в самом коде с TODO.   -  person denis.solonenko    schedule 20.12.2011

Ответы (2)


arrow_upward
6
arrow_downward

AtomicLong и AtomicInteger используют CAS для внутреннего использования — сравнивают и устанавливают (или сравнивают и меняют местами). Идея состоит в том, что вы сообщаете CAS две вещи: ожидаемое значение long/int и значение, до которого вы хотите его обновить. Если long/int имеет значение, которое, как вы говорите, должно быть, CAS автоматически произведет обновление и вернет true; в противном случае обновление не будет выполнено и будет возвращено false. Многие современные чипы очень эффективно поддерживают CAS на уровне машинного кода; если JVM работает в среде, в которой нет CAS, она может использовать мьютексы (то, что Java называет синхронизацией) для реализации CAS. В любом случае, если у вас есть CAS, вы можете безопасно реализовать атомарное приращение с помощью этой логики (в псевдокоде):

long incrementAndGet(atomicLong, byIncrement)
    do
        oldValue = atomicLong.get()            // 1
        newValue = oldValue + byIncrement
    while ! atomicLong.cas(oldValue, newValue) // 2
    return newValue

Если другой поток войдет и сделает свое собственное приращение между строками // 1 и // 2, CAS потерпит неудачу, и цикл повторит попытку. В противном случае CAS сработает.

В таком подходе есть авантюра: если конкуренция низкая, CAS работает быстрее, чем синхронизированный блок, и вероятность переключения контекста потока меньше. Но если есть много конфликтов, некоторым потокам придется пройти через несколько итераций цикла для каждого шага, что, очевидно, равносильно напрасной работе. Вообще говоря, incrementAndGet будет работать быстрее при наиболее распространенных нагрузках.

person yshavit    schedule 20.12.2011

arrow_upward
0
arrow_downward

Инкремент является потокобезопасным, поскольку AtomicInteger и семейство гарантируют это. Но есть проблема с вставкой и выборкой из кеша, где могли быть созданы и вставлены два (или более) HitObject. Это может привести к потенциальной потере некоторых попаданий при первом доступе к этому HitObject. Как указал @denis.solonenko, в коде уже есть TODO, чтобы исправить это.

Однако я хотел бы отметить, что этот код страдает от параллелизма только при первом доступе к данному HitObject. Когда у вас есть HitObject в кеше (и нет больше потоков, создающих или вставляющих HitObject), тогда этот код полностью потокобезопасен. Так что это лишь очень ограниченная проблема параллелизма, и, вероятно, именно поэтому они еще не исправили ее.

person Riseven    schedule 20.12.2011