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

[Stefan Karlsson]

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

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 CodeCache_lock is held, or the invoking 
thread is the VM Thread. The current check 
assert_locked_or_safepoint(CodeCache_lock) opens up for the bugs 
reported in JDK-8139595. Maybe we could make this change as a part of 
JDK-8139595?

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

Fixed.

>
> 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 was thinking the same, but since there currently is no 
vmdependencies_addr 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.

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

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.

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