<i18n dev> RFR: 8288723: Avoid redundant ConcurrentHashMap.get call in java.time [v2]

[Andrey Turbanov]

On Wed, 6 Jul 2022 14:11:39 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> @RogerRiggs in my view, it's not about performance per se… I'll sometimes help along these kinds of small fix PRs (that are more likely than not driven by IntelliJ code refactoring suggestions) if I think they lead to more idiomatic code in the JDK with no drawbacks. I think there's value in periodically revisiting existing code and improve it to use new language and library construct that have become available since – people study JDK source code, and I think it's important they see as good examples in there as possible. I do like seeing an old example of hand-rolled compute-if-absent being replaced by an actual `computeIfAbsent` call. Similarly, I think using records as composite keys is in general a better practice over simulating a composite key by creating a string representation of it. 
>> 
>> Yeah, it'll load additional code at runtime for that record's class, and yes, I'm aware records' `hashCode`/`equals`/`toString` are implemented by delegating to factory methods that heavily use method handle combinators. If records are meant as lightweight value-semantics composites, it should be okay to use them like this and if there are performance concerns with their use, then those concerns should be addressed by improving their performance. OTOH, MH combinators installed into constant call sites are well optimized by HotSpot these days… anyhow, a discussion for another day.
>> 
>> In this specific instance, the value domain of the keys is limited, but the number of method calls to `WeekFields.of` isn't. It could be benchmarked but if there's concerns, it's also okay to de-scope the record-as-key from this PR. I don't have strong feelings either way.
>
> The comment was about WeekFields.of(), I misplaced the comment.
> 
> @szegedi All good points about modernizing code...
> One of the reasons to ask about specific performance data is to validate the general performance impact of using lambdas. In the case of WeekFields.of(), the lambda is passed on every call to `computeIfAbsent` even if the key is present and the lambda won't be used. `WeekFields` is way-way down the long tail of frequency of use and I expect that 99% of calls are for the same one or two combinations.

I agree with @RogerRiggs that performance of JDK is very important. It was one of main motivation for removing `.get` call.
I'm not an expert in microbenchmarking, and it would require significant time/resources for me to investigate which is better.
So I decided to revert back to original proposal (without lambda and `computeIfAbsent`) in `WeekFields`. If you think that it's better to improve code, I think we can always fill a separate issue for it.

-------------

PR: https://git.openjdk.org/jdk/pull/9208