On Sat, 2 Jan 2021 14:45:51 GMT, Attila Szegedi <[email protected]> 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.
Just a small suggestion using `MethodHandles.empty` instead of
`MethodHandles.constant().asType().dropArguments()`.
test/jdk/jdk/dynalink/TypeConverterFactoryRetentionTests.java line 65:
> 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));
> 65: result = MethodHandles.dropArguments(result, 0, sourceType);
Suggestion:
MethodHandle result =
MethodHandles.empty(MethodType.methodType(targetType, sourceType));
test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java line 70:
> 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));
> 70: result = MethodHandles.dropArguments(result, 0, sourceType);
Suggestion:
MethodHandle result =
MethodHandles.empty(MethodType.methodType(targetType, sourceType));
-------------
PR: https://git.openjdk.java.net/jdk/pull/1918
