<i18n dev> RFR: 8345668: ZoneOffset.ofTotalSeconds performance regression [v3]

[Brett Okken]

On Fri, 27 Dec 2024 23:24:46 GMT, Chen Liang <liach at 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