RFR: 8198540: Dynalink leaks memory when generating type converters [v7]

[Peter Levart]

On Sun, 10 Jan 2021 17:04:19 GMT, Attila Szegedi <attila at 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 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.
>
> Attila Szegedi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Rename ClassLoaderRelation to RetentionDirection; it is better self-documenting this way.

I think this looks good Attila.

-------------

Marked as reviewed by plevart (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1918