On Fri, 27 Dec 2024 23:24:46 GMT, Chen Liang <li...@openjdk.org> wrote:
>>> For sure we should use result of `putIfAbsent` >> >> Drive-by comment... >> >> From what i can infer, the performance regression being addressed here is >> caused in part by the fact that (for example) >> `ConcurrentHashMap.computeIfAbsent()` provides an atomicity guarantee, which >> is a stronger requirement than is necessary here, and therefore by splitting >> up that call up into two separate, non-atomic `get()` and `put()` calls we >> get (counter-intuitively) faster execution time, even though there are more >> lines of code. Note `putIfAbsent()` also guarantees atomicity, so the same >> problem of slowness caused by "unnecessary atomicity" might occur with it as >> well. > > Indeed, just noticed that both `computeIfAbsent` and `putIfAbsent` may > acquire the lock when the key is present, while `get` never acquires a lock > for read-only access. > > Maybe the implementation was written back when locking was less costly (with > biased locking, etc.). Now we might have to reconsider locking until we know > for sure a plain get fails. This scenario is discussed in Effective Java by Joshua Block. His observation then (java 5/6 time frame?) was optimistically calling `get` first and only calling `putIfAbsent` or `computeIfAbsent` if the `get` returned `null` was 250% faster, and this is because calls to put/compute ifAbsent have contention. There have been changes made to those methods since then to try to avoid synchronization when the key is already present, but the observation seems to confirm that the optimistic `get` call first is still faster (though a much smaller difference). My comment was not to revert back to the prior change of just calling `computeIfAbsent`, but rather just to change the (expected rare) case when the first `get` returns `null` to replace the `putIfAbsent` and second `get` call with a single `computeIfAbsent` (or utilize the result of `putIfAbsent` to avoid the second call to `get`). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22854#discussion_r1899699668
