On Fri, 28 Feb 2025 14:48:45 GMT, Alexander Zvegintsev <[email protected]> wrote:
>> During the [JDK-8344891 Remove uses of sun.misc.ReflectUtil in >> java.desktop](https://bugs.openjdk.org/browse/JDK-8344891) it was discovered >> that `beans/finder/MethodFinder.findMethod' incorrectly returned null if a >> signature was not in the cache and added it to the cache if it was already >> there: >> >> >> Method method = CACHE.get(signature); >> return (method == null) ? method : CACHE.create(signature); >> >> This resulted in a [significant drop in >> performance](https://bugs.openjdk.org/browse/JDK-8350573). >> >> ---- >> >> Before ReflectUtil was removed, it worked by coincidence: >> >> >> Method method = CACHE.get(signature); >> return (method == null) || isPackageAccessible(method.getDeclaringClass()) ? >> method : CACHE.create(signature); >> >> >> >> >> 1. `Cache#get` non-obviously creates a value in Cache, this in turn allowed >> us to avoid the NPE in the `(method == null) || >> isPackageAccessible(method.getDeclaringClass())` condition >> >> >> https://github.com/openjdk/jdk/blob/d6c4be672f6348f8ed985416ed90d0447f5d5bb3/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L120-L126 >> >> 2. `isPackageAccessible(method.getDeclaringClass())` was evaluated as true >> >> This is how we previously returned the cached value. >> >> --- >> >> So the solution is obvious: >> >> >> Method method = CACHE.get(signature); >> return (method != null) ? method : CACHE.create(signature); >> >> >> Testing is green. > > src/java.desktop/share/classes/com/sun/beans/finder/MethodFinder.java line 81: > >> 79: try { >> 80: Method method = CACHE.get(signature); >> 81: return (method != null) ? method : CACHE.create(signature); > > This can be simplified to the `return CACHE.get(signature)`, since we know > that the implementation creates the value in the get method: > > https://github.com/openjdk/jdk/blob/d6c4be672f6348f8ed985416ed90d0447f5d5bb3/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L120-L126 > > However, I am not a fan of this solution as it is implementation dependent > and this behavior is not documented. I think this is confusing… If `CACHE.get(signature)` always returns a non-null value, there's no need for the condition `method != null` — it's always true. The javadoc for the `Cache` class specifies a `null` value can be returned: https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L98-L99 Is it a bug in `Cache` class that it creates the value and adds it to the cache when the value isn't found? https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L126-L127 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23845#discussion_r1975832067
