On Fri, 28 Feb 2025 14:41:41 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. > > _the unsynchronised access to the data structures doesn't gain anything_ > > The existing optimistic fast path can avoid the lock, with the assumption > that garbage collection of the cache values are rare. It is fine. However > these plain access do require that the cached values to be thread safe and > support safe publication. You're right, the lock in `removeStaleEntries` is acquired if there's a reference to remove. Accessing the `table` field isn't safe without synchronisation. When the table is re-allocated, it may go awry. ------------- PR Comment: https://git.openjdk.org/jdk/pull/23845#issuecomment-2691403946
