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

[Vladimir Ivanov]

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