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 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