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

[Vladimir Ivanov]

Thanks, Coleen!

Updated version:
   http://cr.openjdk.java.net/~vlivanov/8139595/webrev.02/

> 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.
Agree. Moved it to src/share/vm/code/dependencyContext.[ch]pp

> The hidden bit stuff is ok in InstanceKlass.  Does
> has_unloaded_dependent accessed concurrently like
> _is_marked_dependent?
Good point. DependencyContext::remove_dependent_nmethod should set 
has_stale_entries in a thread safe manner (added Atomic::xchg_ptr call).
It is not needed when the flag is cleared.

> It would be nice to move _is_marked_dependent
> field also to encapsulate it but that would ruin your bit packing.
There's a spare bit to use, but I think _is_marked_dependent is specific 
to InstanceKlass and should remain there. It doesn't have any meaning 
for CallSite dependencies.

> 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?
I haven't found any mentions of _has_unloaded_dependent or _dependencies 
fields in SA or JVMCI code. Do you think it is an overlook on SA side 
and I should add it?

Best regards,
Vladimir Ivanov

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