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

[Vladimir Ivanov]

Thanks, Aleksey!

>>> http://cr.openjdk.java.net/~vlivanov/8139595/webrev.00
>>> https://bugs.openjdk.java.net/browse/JDK-8139595
>
> Not a Reviewer. Piggybacking the cleanup on (locked) mutation is a
> standard way to deal with racing issues like that, so I like it.
>
> Some comments:
>
>   * I am confused why the cleanup logic spread out to methodHandles.cpp:
> it would seem a general functionality for DependencyContext, and as such
> it should be handled internally by add_dependent_methods? I think you
> can pass the "safe to purge" flag there.
Cleanup logic in methodHandles.cpp is special to j.l.i.CallSite's and 
call_site_target dependency type. GC doesn't remove stale entries in 
these contexts, so I decided to piggyback on regular operations to purge 
stale entries. That's the idea of the fix.

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

>   * DependencyContext now has three methods: purge(), clear(), wipe(). I
> have trouble understanding which method does what!
I'll add comments describing the intended behavior. There are basically 
2 operations needed:
   * purge(): only remove stale entries; no additional work is performed;

   * clear(): invalidate all dependencies in the context: effectively 
removes all entries, deoptimizes and throws away all affected methods.

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.

I could have added that code into TestNmethodBucket, but then I would 
need to declare TestNmethodBucket as a friend of DependencyContext to 
preserve incapsulation.

>   * Since there is a change in vmStructs, does this also need SA changes?
/agent/src/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java refers 
neither to _dependencies nor _has_unloaded_dependent fields. So, it 
seems no changes are needed there.

Best regards,
Vladimir Ivanov