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

[Roger Riggs]

On Tue, 5 Jul 2022 21:27:16 GMT, Attila Szegedi <attila at openjdk.org> wrote:

>> Can there be some JMH tests to confirm the performance?
>> The value domain of the keys is pretty limited (7 * 7) max; and I'm not sure that the combination of creating a new record and the hashcode and equals methods would be faster than string concat of a constant and a single digit integer.
>
> @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.

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

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