[9] RFR (M): 8139595: MethodHandles::remove_dependent_nmethod is not MT safe

[Coleen Phillimore]

Hi Vladimir,

I think DependencyContext is big enough to be a new file, and not be 
added to instanceKlass.hpp.  There are too many unrelated things in 
instanceKlass.  The hidden bit stuff is ok in InstanceKlass.  Does 
has_unloaded_dependent accessed concurrently like 
_is_marked_dependent?   It would be nice to move _is_marked_dependent 
field also to encapsulate it but that would ruin your bit packing.

Also, since you have changes to vmStructs, do you have duplicate changes 
to make to the serviceability agent code?  Are there duplicate changes 
for the jvmci code?

Thanks,
Coleen

On 11/5/15 1:54 PM, Vladimir Ivanov wrote:
> Updated version:
> http://cr.openjdk.java.net/~vlivanov/8139595/webrev.01/
>
> In addition to name polishing and encapsulating purging logic in 
> {add|remove}_dependent_nmethod methods, I got rid of 
> DependencyContext::wipe() method and added "friend class 
> TestDependencyContext" declaration (noticed such practice in other 
> cases when tests need access to tested class internals).
>
> Best regards,
> Vladimir Ivanov
>
> On 11/5/15 7:46 PM, Aleksey Shipilev wrote:
>> On 11/05/2015 04:54 PM, Vladimir Ivanov wrote:
>>> Regarding internal purging, add_dependent_methods doesn't iterate over
>>> the whole list, but looks for the relevant entry. It means that "purge"
>>> flag should force full iteration. I think that keeping them separately
>>> (add_dependent_methods() vs purge()) looks cleaner.
>>
>> I think it is better to encapsulate the "piggybacking" behavior within
>> the DependencyContext itself, because that seems to be a
>> general/expected feature of DependencyContext class. It is weird to ask
>> class users to spell it out specifically.
>>
>>
>>> I could add "purge" flag and move the following code into
>>> add_dependent_methods:
>>> void DependencyContext::add_dependent_nmethod(nmethod* nm, bool 
>>> do_purge
>>> = false) {
>>>     ...
>>>     if (has_unloaded_dependent() && do_purge) {
>>>       purge();
>>>     }
>>> }
>>>
>>> Is it what you are suggesting?
>>
>> Yes.
>>
>>
>>>>    * DependencyContext now has three methods: purge(), clear(), 
>>>> wipe(). I
>>>> have trouble understanding which method does what!
>>
>>> There are basically 2 operations needed:
>>>    * purge(): only remove stale entries; no additional work is 
>>> performed;
>>
>> Oh. Should probably mention "stale" in the name. See e.g. Java-ish:
>>   WeakHashMap.expungeStaleEntries()
>>   ThreadLocal.expungeStaleEntries()
>>   WeakCache.expungeStaleEntries()
>>
>>
>>> wipe() is only for unit-testing purposes (see TestNmethodBucket): it
>>> just deallocates all nmethodBucket entries without touching 
>>> nmethods. It
>>> is necessary since nmethod pointers are faked by the test.
>>
>> Okay. Is there a convention how you should name the test-related methods
>> like these? I would expect something like debug_deallocate_buckets().
>>
>> Thanks,
>> -Aleksey
>>
>>