RFR: 8058563: InstanceKlass::_dependencies list isn't cleared from empty nmethodBucket entries (original) (raw)

Mikael Gerdin mikael.gerdin at oracle.com
Wed Oct 14 12:29:34 UTC 2015

Hi Stefan,

On 2015-10-14 11:36, Stefan Karlsson wrote:

Hi all,

Please review this patch to reintroduce deletion of entries in the InstanceKlass::dependencies list. It would be good if this could be reviewed by both the GC team and the Compiler team. http://cr.openjdk.java.net/~stefank/8058563/webrev.01/

in instanceKlass.cpp, nmethodBucket::remove_dependent_nmethod would it make sense to assert that delete_immediately is true iff the current thread is the owner of the CodeCache_lock?

in instanceKlass.hpp the bool parameter to remove_dependent* is named "bool deferred_delete". in the .cpp file it's "bool delete_immediately"

I'm not particularly fond of the "convenience method": 1946 bool nmethodBucket::remove_dependent_nmethod(nmethodBucket* deps, nmethod* nm)

Its only caller, MethodHandles::remove_dependent_nmethod, looks like it actually should do nmethodBucket::remove_dependent_nmethod( java_lang_invoke_MethodHandleNatives_CallSiteContext::vmdependencies_addr(context), nm, true); since it immediately calls clean_dependent_nmethods and updates the list head if remove_dependent returned true.

I also rediscovered a known issue, that remove_dependent_nmethod can actually be called during parallel unloading and thereby cause crashes but fixing that is another step.

Otherwise I think the change is ok, looking forward to further cleanups here ;)

/Mikael

https://bugs.openjdk.java.net/browse/JDK-8058563

Some background to this bug: Before JDK-8049421, it was guaranteed that only one thread at a time could delete an nmethodBucket in the dependencies list. JDK-8049421 parallelized the unloading of nmethods for G1 and the deletion of the entries were deferred to a later GC phase, to save the cost of having to synchronize the deletion of entries between the GC threads. The deletions are instead done at a later phase when the GC threads claim Klasses for cleaning and it's guaranteed that each Klass will only be cleaned by one GC thread. This patch will solve two problems with the current implementation of the deferred deletion: 1) Today only G1 deletes the deferred entries and all other GCs leak the entries. The patch adds calls to clean out entries from all GCs. 2) Entries used to be deleted immediately when flushdependencies was called from non-GC code, but today this code path also defers the deletion. This is unnecessary, since the callers hold the CodeCachelock while flushing the dependencies, and the code is thereby only executed by one thread at a time. The patch adds back the immediate deletion of entries, when called from non-GC code. The code has changed a bit in JDK 9, but it might still be useful to take a look at the patch that introduced the deferred deletion and compare that to the suggested patch: http://hg.openjdk.java.net/jdk8u/hs-dev/hotspot/diff/2c6ef90f030a/src/share/vm/oops/instanceKlass.cpp

Tested with: JPRT, Kitchensink, parallelclassunloading, Weblogic12medrec, runThese, new unit test Thanks, StefanK

More information about the hotspot-dev mailing list