On Fri, 8 Jan 2021 08:53:19 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 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 two additional
> commits since the last revision:
>
> - Use immutable maps instead of concurrent ones
> - Remove classLoader field from BiClassValues and evaluate class loader
> relationships in a doPrivileged block
src/jdk.dynalink/share/classes/jdk/dynalink/BiClassValue.java line 126:
> 124: entries.add(Map.entry(c, value));
> 125: @SuppressWarnings("rawtypes")
> 126: final var newEntries = entries.toArray(new
> Map.Entry[0]);
Since map is immutable now, instead of:
var entries = new ArrayList<>(map.entrySet());
entries.add(Map.entry(c, value));
var newEntries = entries.toArray(new Map.Entry[0]);
you could write:
var entries = map.entrySet().toArray(new Map.Entry[map.size() + 1]);
entries[map.size()] = Map.entry(c, value);
-------------
PR: https://git.openjdk.java.net/jdk/pull/1918
