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

[John Rose]

I think this expression occurs too many times:
  DependencyContext dep_context(&_dep_context);

It would look better if factored into an access method:
  inline DependencyContext InstanceKlass::dependencies() {
    DependencyContext dep_context(&_dep_context);
    return dep_context;
  }

That way the raw machine word InstanceKlass::_dep_context can be isolated.
CallSiteContext::vmdependencies already does the corresponding move for CallSite.
(I understand that you don't want to make ik.hpp depend on dc.hpp.
Just put forward declarations of Deps and IK::deps in ik.hpp.)

Mild suggestion:  Add a constructor parameter to capture the base oop (of the callsite, or null for the IK).
Use it only in debug mode to check for GC changes.  Or sample a global GC or safepoint count.
The point is that the lifetime of the Deps object should not extend beyond the next safepoint.

I agree with Coleen:  This is a good refactor.

Reviewed.

— John

On Nov 6, 2015, at 5:03 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> 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
> 
>> 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
>>>> 
>>>> 
>>