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

[Mikael Gerdin]

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