On Fri, 9 Jun 2023 22:17:39 GMT, Naoto Sato <na...@openjdk.org> wrote:
> This is stemming from the PR: https://github.com/openjdk/jdk/pull/14211 where > aggressive GC can cause NPE in `BaseLocale$Key` class. I refactored the > in-house cache with WeakHashMap, and removed the Key class as it is no longer > needed (thus the original NPE will no longer be possible). Also with the new > JMH test case, it gains some performance improvement: > > (w/o fix) > > Benchmark Mode Cnt Score Error Units > LocaleCache.testForLanguageTag avgt 20 5781.275 ± 569.580 ns/op > LocaleCache.testLocaleOf avgt 20 62564.079 ± 406.697 ns/op > > (w/ fix) > Benchmark Mode Cnt Score Error Units > LocaleCache.testForLanguageTag avgt 20 4801.175 ± 371.830 ns/op > LocaleCache.testLocaleOf avgt 20 60394.652 ± 352.471 ns/op The new code looks much easier to maintain than the old one. I couldn't figure out what the old code was actually trying to do, but reading the new code I now understand what it does. Caching, retrieval, hashCode and equals look good. From the code it's trivial to see that a new normalised BaseLocale will be created if none is found in the cache - so that will assuredly fix the NPE that was experienced previously when the cache was cleared. The main change is that base locales will now be cached using WeakReference rather than SoftReference, and thus probably collected more eagerly, but that actually looks like a positive change. Not a usual reviewer in this area so please get another reviewer before integrating (also I haven't looked at the test). ------------- Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14404#pullrequestreview-1475076367
