On Sat, 2 Jan 2021 14:45:51 GMT, Attila Szegedi <att...@openjdk.org> wrote:
> _(NB: For this leak to occur, an application needs to be either creating and > discarding linkers frequently, or creating and discarding class loaders > frequently while creating Dynalink type converters for classes in them. > Neither is typical usage, although they can occur in dynamic code loading > environments such as OSGi.)_ > > I'll explain this one in a bit more detail. Dynalink's creates and caches > method handles for language-specific conversions between types. Each > conversion is thus characterized by a `Class` object representing the type > converted from, and a `Class` object representing the type converted to, thus > they're keyed by a pair of `(Class, Class)`. Java API provides the excellent > `ClassValue` class for associating values with a single class, but it lacks > the – admittedly more niche – facility for associating a value with a pair of > classes. I originally solved this problem using something that looks like a > `ClassValue<Map<Class<?>, T>>`. I say "looks like" because Dynalink has a > specialized `ClassMap` class that works like `Map<Class<?>, T>` but it also > goes to some pains to _not_ establish strong references from a class loader > to either its children or to unrelated class loaders, for obvious reasons. > > Surprisingly, the problem didn't lie in there, though. The problem was in the > fact that `TypeConverterFactory` used `ClassValue` objects that were its > non-static anonymous inner classes, and further the values they computed were > also of non-static anonymous inner classes. Due to the way `ClassValue` > internals work, this led to the `TypeConverterFactory` objects becoming > anchored in a GC root of `Class.classValueMap` through the synthetic `this$0` > reference chains in said inner classes… talk about non-obvious memory leaks. > (I guess there's a lesson in there about not using `ClassValue` as an > instance field, but there's a genuine need for them here, so…) > > … so the first thing I did was I deconstructed all those anonymous inner > classes into named static inner classes, and ended up with something that > _did_ fix the memory leak (yay!) but at the same time there was a bunch of > copying of constructor parameters from outer classes into the inner classes, > the whole thing was very clunky with scary amounts of boilerplate. I felt > there must exist a better solution. > > And it exists! I ended up replacing the `ClassValue<ClassMap<T>>` construct > with a ground-up implementation of `BiClassValue<T>`, representation of > lazily computed values associated with a pair of types. This was the right > abstraction the `TypeConverterFactory` code needed all along. > `BiClassValue<T>` is also not implemented as an abstract class but rather it > is a final class and takes a `BiFunction<Class<?>, Class<?>, T>` for > computation of its values, thus there's never a risk of making it an > instance-specific inner class (well, you still need to be careful with the > bifunction lambda…) It also has an even better solution for avoiding strong > references to child class loaders. > > And that's why I'm not submitting here the smallest possible point fix to the > memory leak, but rather a slightly larger one that architecturally fixes it > the right way. > > There are two test classes, they test subtly different scenarios. > `TypeConverterFactoryMemoryLeakTest` tests that when a `TypeConverterFactory` > instance becomes unreachable, all method handles it created also become > eligible for collection. `TypeConverterFactoryRetentionTests` on the other > hand test that the factory itself won't prevent class loaders from being > collected by virtue of creating converters for them. test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java line 69: > 67: public GuardedInvocation convertToType(Class<?> sourceType, > Class<?> targetType, Supplier<MethodHandles.Lookup> lookupSupplier) { > 68: // Never meant to be invoked, just a dummy MH that conforms > to the expected type. > 69: MethodHandle result = MethodHandles.constant(Object.class, > null).asType(MethodType.methodType(targetType)); Suggestion: MethodHandle result = MethodHandles.zero(targetType); test/jdk/jdk/dynalink/TypeConverterFactoryRetentionTests.java line 64: > 62: public GuardedInvocation convertToType(Class<?> sourceType, > Class<?> targetType, Supplier<MethodHandles.Lookup> lookupSupplier) { > 63: // Never meant to be invoked, just a dummy MH that conforms > to the expected type. > 64: MethodHandle result = MethodHandles.constant(Object.class, > null).asType(MethodType.methodType(targetType)); Suggestion: MethodHandle result = MethodHandles.zero(targetType); ------------- PR: https://git.openjdk.java.net/jdk/pull/1918