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

[Stefan Karlsson]

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::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?
> Ok.
>
>>
>>> 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.
> Sounds good.
>
>>
>>>
>>> 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.
>
> 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 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
>>>
>>
>