On Wed, 6 Jul 2022 14:11:39 GMT, Roger Riggs <rri...@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
