On Mon, 3 Mar 2025 14:36:32 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. > > Alexander Zvegintsev has updated the pull request incrementally with one > additional commit since the last revision: > > return CACHE.get(signature); Looks good to me. I agree it's better to handle other issues under a separate bug or bugs. src/java.desktop/share/classes/com/sun/beans/finder/MethodFinder.java line 1: > 1: /* Could you update the copyright year? ------------- Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23845#pullrequestreview-2654619166 PR Review Comment: https://git.openjdk.org/jdk/pull/23845#discussion_r1977805251
