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

[Coleen Phillimore]

On 11/6/15 8:03 AM, Vladimir Ivanov wrote:
> 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

I like this!!
>
>> 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?

If it's not int he SA or JVMCI code, I think it can just be removed from 
vmStructs.cpp (or not added).   There used to be some nsk.sajdi.testlist 
tests that are quarantined but you can run them to make sure.

http://cr.openjdk.java.net/~vlivanov/8139595/webrev.02/src/share/vm/code/dependencyContext.hpp.html

This file uses CHeapObj<mtClass> but doesn't include allocation.hpp.   
Does it compile without precompiled headers?

Thanks,
Coleen

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