On Sun, 10 Jan 2021 17:04:19 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 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. Nice work as ever, Attila. Looks good to me. ------------- Marked as reviewed by hannesw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1918
