RFR: 8058563: InstanceKlass::_dependencies list isn't cleared from empty nmethodBucket entries
stefan.karlsson at oracle.com
Mon Oct 19 08:17:44 UTC 2015
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.
> 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
> 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
> 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
> 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:
> Otherwise I think the change is ok, looking forward to further
> cleanups here ;)
>> 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:
>> Tested with:
>> JPRT, Kitchensink, parallel_class_unloading, Weblogic12medrec,
>> runThese, new unit test
More information about the hotspot-dev