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

[Vladimir Ivanov]

Coleen, John, thanks for the feedback!

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

* Added InstanceKlass::dependencies()

* Removed the fields from vmStructs since they aren't used in SA 
(tmtools tests passed).

* Added missing includes in dependencyContext.hpp. Previous version 
built fine (even w/o precompiled headers), but I think it worked due to 
header inclusion order.

* Added verification of CallSite oop validity across DependencyContext 
lifetime.

Though previous comments were focused on dependency management code 
refactoring, I'd like to hear opinions about the fix itself - dependency 
management changes for j.l.i.CallSites to avoid modifications during 
concurrent traversals by GC threads.

Quote from the original request:
  "The fix I propose is to avoid purging of CallSiteContext contexts at 
all during GC, but remove stale entries during dependency list updates 
which happen under CodeCache lock (see safe_to_purge() check in 
methodHandles.cpp). It happens outside of safepoints and it guarantees 
exclusive access to the list. It is performed during 
j.l.i.CallSite.target updates, CallSite.target inlining, j.l.Class 
unloading, etc. Such approach allows to avoid unbounded memory 
consumption growth, only some delay (till next dependency list update) 
in memory deallocation."

Best regards,
Vladimir Ivanov

On 11/10/15 5:37 AM, John Rose wrote:
> 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
>>>>>
>>>>>
>>>
>