On Fri, 8 Jan 2021 08:53: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 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 106: > 104: > 105: private volatile Map<Class<?>, T> forward = Map.of(); > 106: private volatile Map<Class<?>, T> reverse = Map.of(); Since maps are immutable now and allow safe publication via data race (i.e. their internal state is composed of final fields), you could relax the forward/referse fields to be plain fields. You would just have to leave them uninitialized then and check for null at some places if BiClassValues could be published via data race since I don't know whether ClassValue guarantees that the associated values are published safely or not. If it does, then you could pre-initialize the field with empty maps as now, if it does not, then even volatile modifiers don't guarantee that unsafely published BiClassValues object can only be seen with the fields initialized. i.e. you could read null from them. ------------- PR: https://git.openjdk.java.net/jdk/pull/1918
