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

Stefan Karlsson stefan.karlsson at oracle.com
Mon Oct 19 08:40:51 UTC 2015

On 2015-10-19 10:27, Mikael Gerdin wrote:

Stefan,

On 2015-10-19 10:17, Stefan Karlsson wrote: On 2015-10-14 14:29, Mikael Gerdin wrote:

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::removedependentnmethod would it make sense to assert that deleteimmediately is true iff the current thread is the owner of the CodeCachelock? I took a look at this. I think we should change all nmethodBucket mutating functions to have stricter checking. Maybe it would be enough to check that the either the CodeCachelock is held, or the invoking thread is the VM Thread. The current check assertlockedorsafepoint(CodeCachelock) opens up for the bugs reported in JDK-8139595. Maybe we could make this change as a part of JDK-8139595? Ok. in instanceKlass.hpp the bool parameter to removedependent* is named "bool deferreddelete". in the .cpp file it's "bool deleteimmediately" Fixed. I'm not particularly fond of the "convenience method": 1946 bool nmethodBucket::removedependentnmethod(nmethodBucket* deps, nmethod* nm) Its only caller, MethodHandles::removedependentnmethod, looks like it actually should do nmethodBucket::removedependentnmethod( javalanginvokeMethodHandleNativesCallSiteContext::vmdependenciesaddr(context), nm, true); since it immediately calls cleandependentnmethods and updates the list head if removedependent returned true. I was thinking the same, but since there currently is no vmdependenciesaddr function, I opted for not doing this change. Personally, I would have preferred if the nmethodBucket list was wrapped in a nmethodBucketList class. That way I wouldn't have to create this convenience method. I'd prefer to leave this potential cleanup to the compiler team. Sounds good.

I also rediscovered a known issue, that removedependentnmethod can actually be called during parallel unloading and thereby cause crashes but fixing that is another step. Yes, Maybe I should have mentioned: https://bugs.openjdk.java.net/browse/JDK-8139595 Otherwise I think the change is ok, looking forward to further cleanups here ;) Me too. Anyway, I think this change is ready to go.

Thanks! I'm going to push the patch above + the fix of the parameter name.

StefanK

/Mikael

Thanks, StefanK

/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