RFR: 8058563: InstanceKlass::_dependencies list isn't cleared from empty nmethodBucket entries

[Mikael Gerdin]

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 flush_dependencies was
> called from non-GC code, but today this code path also defers the
> deletion. This is unnecessary, since the callers hold the CodeCache_lock
> 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, parallel_class_unloading, Weblogic12medrec,
> runThese, new unit test
>
> Thanks,
> StefanK