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

[Attila Szegedi]

On Tue, 5 Jul 2022 15:26:06 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> Well, if you _really_ want to noodle this for performance, you can also store a `this`-bound lambda in a `private final`  instance field, so then it's only created once too. I wouldn't put it past `javac` to do this, but I'd have to disassemble the bytecode of a little test program to see whether it's the case.
>
> 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.

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

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