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

[Coleen Phillimore]

I still like the refactoring.  I  don't have further comments but I 
didn't study what the DependencyContext does, so hopefully your other 
reviewers looked at that.

Coleen

On 11/11/15 10:43 AM, Vladimir Ivanov wrote:
> 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
>>>>>>
>>>>>>
>>>>
>>