> _(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: Avoid null values in fields again ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/1918/files - new: https://git.openjdk.java.net/jdk/pull/1918/files/a8d81ebb..cb399d7b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1918&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1918&range=04-05 Stats: 18 lines in 1 file changed: 0 ins; 9 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/1918.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1918/head:pull/1918 PR: https://git.openjdk.java.net/jdk/pull/1918
