8198540: Dynalink leaks memory when generating type converters

[szegedi]

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

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8198540: Dynalink leaks memory when generating type converters

Reviewers

• Peter Levart (@plevart - Reviewer)
• Hannes Wallnöfer (@hns - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1918/head:pull/1918
$ git checkout pull/1918

[bridgekeeper]

👋 Welcome back attila! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@szegedi To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

• 2d
• awt
• beans
• build
• compiler
• core-libs
• hotspot
• hotspot-compiler
• hotspot-gc
• hotspot-jfr
• hotspot-runtime
• i18n
• javadoc
• jdk
• jmx
• kulla
• net
• nio
• security
• serviceability
• shenandoah
• sound
• swing

[szegedi]

/label core-libs

[openjdk]

@szegedi
The core-libs label was successfully added.

[mlbridge]

Webrevs

• 06: Full - Incremental (81bf988)
• 05: Full - Incremental (cb399d7)
• 04: Full - Incremental (a8d81eb)
• 03: Full - Incremental (3f934a2)
• 02: Full - Incremental (eaddc17)
• 01: Full - Incremental (3f3efed)
• 00: Full (24d3787)

[DasBrain]

Just a small suggestion using MethodHandles.empty instead of MethodHandles.constant().asType().dropArguments().

[plevart]

Hi Attila,

This looks good.

If I understand correctly, your BiClassValue is a holder for a BiFunction and a BiClassValueRoot which is a ClassValue<BiClassValues>. The BiClassValues contains two lazily constructed Map(s): forward and reverse. You then employ a logic where you either use the forward map obtained from c1 if c1.classLoader sees c2.classLoader or backward map obtained from c2 if c2.classLoader sees c1.classLoader or you don't employ caching if c1.classLoader and c2.classLoader are unrelated.
The need for two Maps is probably because the two Class components of the "bi-key" are ordered, right? So BiClassValue bcv = ...; bcv.get(c1, c2) is not the same as bcv.get(c2, c1) ....
Alternatively you could have a BiClassValue with two embedded ClassValue(s):

final CVRoot<T> forward = new CVRoot<>();
final CVRoot<T> reverse = new CVRoor<>();

static class CVRoot<T> extends ClassValue<CHMCL<T>> {
  public CHMCL<T> get(Class<?> clazz) { return new CHMCL<>(getClassLoader(clazz)); }
}

static class CHMCL<T> extends ConcurrentHashMap<Class<?>, T> {
  final ClassLoader classLoader;
  CHMCL(ClassLoader cl) { classLoader = cl; }
}

...with that you could get rid of the intermediary BiClassValues object. It would be replaced with a ConcurrentHashMap subclass that would contain a field to hold the cached ClassLoader of the c1/c2. One de-reference less...

As for the main logic, it would be similar to what you have now. Or it could be different. I wonder what is the performance of canReferenceDirectly(). If you used SharedSecrets to obtain a ClassLoader of a Class without security checks (and thus overhead), you could perhaps evaluate the canReferenceDirectly() before lookups so you would not needlessly trigger initialization of ClassValue(s) that don't need to get initialized.

WDYT?

[szegedi]

Hey Peter,

thank you for the most thoughtful review! You understood everything right. Your approach looks mostly equivalent to mine, with bit of different tradeoffs. I'm indeed using BiClassValues as an intermediate object; it has two benefits. The smaller benefit is that the class loader lookup is performed once per class and stored only once per class. The larger benefit is that it defers the creation of the CHMs until it has something to put into them. Most of the time, only one of either forward or reverse will be created. You're right that the same benefit could be had if we checked canReferenceDirectly first, although I'm trying to make the fast path as fast as possible.

I'm even thinking I could afford to read the class loader on each use of main logic when a cached value is not available and not even use a CHM subclass to hold it. canReferenceDirectly is basically equivalent to "isDescendantOf" and can be evaluated fairly quickly, unless there's a really long class loader chain. (FWIW, it is also equivalent to !ClassLoader.needsClassLoaderPermissionCheck(from, to) but, alas, that's private, and even ClassLoader.isAncestor that I could similarly use is package-private.

In any case, your suggestion nudged me towards a different rework: BiClassValues now uses immutable maps instead of concurrent ones, and uses those VarHandles for compareAndExchange on them. In this sense, BiClassValue is now little more than a pair of atomic references. I decided CHM was an overkill here as the map contents stabilize over time; using immutable maps feels more natural.

I also realized that canReferenceDirectly invocations also need to happen in a doPrivileged block (Dynalink itself is loaded through the platform class loader, so needs these.)

FWIW, your suggestion to use SharedSecrets is intriguing - if I could access both ClassLoader.isAncestor and ClassLoader.getClassLoader through JavaLangAccess, that'd indeed spare me having to go through doPrivileged blocks. OTOH, there's other places in Dynalink that could benefit from those methods, and strictly speaking it's a performance optimization, so I'll rather save that for a separate PR instead of expanding this one's scope. If I adopted using JavaLangAccess I might also look into whether I could replace this class' implementation with a ClassLoaderValue instead, but again, that's beyond the scope of this PR.

[plevart]

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

[plevart]

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.

[plevart]

So what do you think of this variant:
plevart@7af5b81
...reading of forward/reverse fields in compute can still be volatile while in fast path it can be relaxed. In addition, since the ClassValue spec does not say whether it publishes associated values safely or not (I think the impl. does publish them safely), it is better to not rely on that and not pre-initialize the forward/reverse fields with empty maps and rely on them never to be observed as null...

[szegedi]

So what do you think of this variant:

I like it. I originally kept the fields volatile so that we don't observe stale values on getForward/getReverse, but you're right in that even if we do, the correct value will be observed when doing a volatile read in compute, at the additional expense of evaluating class loader relationships, but again, that's the slow path.

IIUC, your changes mostly all flow from the decision to declare the fields as non-volatile; if they were still declared as volatile then it'd be impossible to observe null in them, I think (correct me if I'm wrong; it seems like you thought through this quite thoroughly) as then I don't see how could a volatile read happen before the initial volatile writes as the writes are part of the ClassValues constructor invocation and the reference to the ClassValues object is unavailable externally before the constructor completes. In any case, your approach definitely avoids any of these concerns so I'm inclined to go with it.

[plevart]

IIUC, your changes mostly all flow from the decision to declare the fields as non-volatile; if they were still declared as volatile then it'd be impossible to observe null in them, I think (correct me if I'm wrong; it seems like you thought through this quite thoroughly) as then I don't see how could a volatile read happen before the initial volatile writes as the writes are part of the ClassValues constructor invocation and the reference to the ClassValues object is unavailable externally before the constructor completes. In any case, your approach definitely avoids any of these concerns so I'm inclined to go with it.

It depends entirely on the guarantees of ClassValue and not on whether the fields are volatile or not. If ClassValue publishes the BiClassValues object via data race then even if the fields in BiClassValues are volatile and initialized in constructor, the publishing write in ClassValue could get reordered before the volatile writes of the fields, so you could observe the fields uninitialized.
I can't find in the spec of ClassValue any guarantees of ordering, but I guess the implementation does guarantee safe publication. So if you want to rely on ClassValue guaranteeing safe publication, you can pre-initialized the fields in constructor and code can assume they are never null even if they are not volatile.

[plevart]

Pardon me! A result of introducing new local late in the process.

[mlbridge]

Mailing list message from Peter Levart on core-libs-dev:

On 1/8/21 2:03 PM, Peter Levart wrote:

On Fri, 8 Jan 2021 12:23:36 GMT, Attila Szegedi <attila at openjdk.org> wrote:

IIUC, your changes mostly all flow from the decision to declare the fields as non-volatile; if they were still declared as volatile then it'd be impossible to observe null in them, I think (correct me if I'm wrong; it seems like you thought through this quite thoroughly) as then I don't see how could a volatile read happen before the initial volatile writes as the writes are part of the ClassValues constructor invocation and the reference to the ClassValues object is unavailable externally before the constructor completes. In any case, your approach definitely avoids any of these concerns so I'm inclined to go with it.
It depends entirely on the guarantees of ClassValue and not on whether the fields are volatile or not. If ClassValue publishes the BiClassValues object via data race then even if the fields in BiClassValues are volatile and initialized in constructor, the publishing write in ClassValue could get reordered past

correction: past -> before

To explain: Normal writes that appear in program order before a volatile
write can not be observed to appear later than the volatile write. But
normal writes that appear in program order after a volatile write can be
observed to appear before the volatile write.

Regards, Peter

[plevart]

I checked the code of ClassValue and it can be assumed that it publishes associated values safely. The proof is that it keeps values that it publishes assigned to the final field java.lang.ClassValue.Entry#value.

[szegedi]

So, are you saying the solution where I kept the fields volatile and initialized them with Map.of() is safe? If so, that'd be good news; I'm inclined these days to write as much null-free code as possible :-)

[plevart]

Yes, the pre-initialized fields to Map.of() are safe regardless of whether they are volatile or not (so I would keep them non-volatile to optimize fast-path). Because the BiClassValues instance is published safely to other threads via ClassValue and because you never assign null to the fields later on.

[szegedi]

Alright, I made a new hybrid of non-volatile fields and never null fields. Hopefully we're getting to the ideal. Again, I really appreciate all the advice and direction you provided here.

[plevart]

Hello Attila,
This looks good to me. Just a question: How frequent are situations where the two classloaders are unrelated?

[szegedi]

How frequent are situations where the two classloaders are unrelated?

I think they'd be extremely unlikely. The only user of these right now is Dynalink's type converter factory. I can imagine a situation where there's a conversion from a dynamic language runtime's internal "object" type to an application-specific Java interface, or from its internal "function object" type to an app-specific Java SAM type, and for some reason the app-specific types aren't in the same or descendant class loader of the language runtime's loader.
Frankly, I'd expect 99.99% of the time, app classes would be in the same-or-descendant class loader relative to the dynamic language runtime types. It'd have to be a really exotic setup for this not to be the case, but I'd rather not second guess the users and provide a reasonable functionality even in this case.
If you're thinking of rather throwing an exception when they're unrelated… well, we could certainly do that but I give it a mean time of six months before somebody runs into it and asks about it on Stack Overflow.

[plevart]

Well, I was just thinking if it might be more frequent and would benefit from caching the result too. But if it is not, then what you have now is OK.

[szegedi]

@plevart would you be then be okay with approving this PR? Also, @hns or @sundararajana can I maybe get a review from either of you?

[openjdk]

@plevart Unknown command approve - for a list of valid commands use /help.

[plevart]

I think this looks good Attila.

[openjdk]

@szegedi This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8198540: Dynalink leaks memory when generating type converters

Reviewed-by: plevart, hannesw

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 571 new commits pushed to the master branch:

• 5183d8a: 8260355: AArch64: deoptimization stub should save vector registers
• 5d8204b: 8261368: The new TestNullSetColor test is placed in the wrong group
• f03e839: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code
• 7451962: 8129776: The optimized Stream returned from Files.lines should unmap the mapped byte buffer (if created) when closed
• ad525bc: 8261281: Linking jdk.jpackage fails for linux aarch32 builds after 8254702
• 2fd8ed0: 8240632: Note differences between IEEE 754-2019 math lib special cases and java.lang.Math
• ace8f94: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed
• ab65d53: 8261261: The version extra fields needs to be overridable in jib-profiles.js
• 20d7713: 8261334: NMT: tuning statistic shows incorrect hash distribution
• 92c6e6d: 8261254: Initialize charset mapping data lazily
• ... and 561 more: https://git.openjdk.java.net/jdk/compare/07c93fab8506b844e3689fad92ec355ab0dd3c54...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[hns]

@plevart would you be then be okay with approving this PR? Also, @hns or @sundararajana can I maybe get a review from either of you?

@szegedi I've looked at the BiClassValue code and it looks good to me. If that's ok for you I'll review the rest of the PR tomorrow morning.

[hns]

Nice work as ever, Attila. Looks good to me.

[szegedi]

Thanks for the review, Hannes! I must credit Peter too with how the final version of the code ended up, he really helped a lot with insightful comments and advice.

[szegedi]

/integrate

[openjdk]

@szegedi Since your change was applied there have been 578 commits pushed to the master branch:

• edd5fc8: 8261096: Convert jlink tool to use Stream.toList()
• 2f893c2: 8260337: Optimize ImageReader lookup, used by Class.getResource
• f0bd9db: 8257569: Failure observed with JfrVirtualMemory::initialize
• 906faca: 8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841
• b38d5be: 8261340: Fix 'deprecated' warnings in the vmTestbase/nsk tests
• b0e7e5a: 8261263: Simplify javadoc link code
• 8ebed28: 8261237: remove isClassPathAttributePresent method
• 5183d8a: 8260355: AArch64: deoptimization stub should save vector registers
• 5d8204b: 8261368: The new TestNullSetColor test is placed in the wrong group
• f03e839: 8261127: Cleanup THREAD/TRAPS/CHECK usage in CDS code
• ... and 568 more: https://git.openjdk.java.net/jdk/compare/07c93fab8506b844e3689fad92ec355ab0dd3c54...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8f4c15f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.