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

[Vladimir Ivanov]

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

Changes related to bug fix:
  http://cr.openjdk.java.net/~vlivanov/8139595/webrev.04.fix/

> The nmethod bucket links stay around for a while, but we hope they don't
> build up too badly.  Can you describe how the worst case scenario
> occurs?  (Lots of nmethod compilation inlining a CS, but then a large
> number of calls to nmethod::flush_dependencies during a safepoint.)
Worst case scenario is when all CallSiteContext buckets are marked stale 
(e.g. during GC when it is not safe to remove them), but CallSite 
remains alive and it is not inlined in any other nmethod. In that case, 
the context will never be cleaned up and native memory consumption will 
stay the same.

If a CallSite object becomes unreachable, dedicated sun.misc.Cleaner 
will wipe corresponding dependency context by calling 
MHN_clearCallSiteContext.

If CallSite is inlined in any nmethod (new bucket is added into the 
list), all stale entries from corresponding dependency context are 
removed.

> Suggestion:   Maybe put in counters to track track (a) total number of
> bucket links and (b) number of stale ones.  (Is there a family of
> counters like this that we can add to?  Something in NMT?  A debug-only
> counter pair would be helpful, but even better would be something that
> would be more routinely tracked and dumped.)
I added 4 jvmstat counters.

> What happens if we make a zillion nmethods using the same CS, and then
> they all go away at once during class unloading, so that the CS has
> *only* stale entries?  Who will clean up its bucket list?  When the GC
> frees the CS node from the managed heap, what will happen to the bucket
> list–will it be leaked onto the C heap?  Same question if the CS has one
> long-lived nmethod:  Will we hold onto the stale buckets?  If this turns
> out to be a problem (see counter suggestion above) maybe we need a
> sweeper task.  The nmethod sweeper might visit nmethods periodically and
> expunge their CS dependencies.
Doing some work in nmethod sweeper sounds interesting. The only drawback 
I see is that it has to do that while holding CodeCache lock.

> Another small point:  Are you sure Atomic::xchg_ptr does the job?  You
> don't check the return value, which is odd.  I think the code you wrote
> is the same as a store/release (volatile store).  Did you mean to use a
> CAS?  Or:
>    intptr_t w = Atomic::xchg_ptr(v, ...):
>    assert(((v ^ w) & ~_has_stale_entries_mask) == 0);
All parallel workers write the same value into the memory - first bucket 
address w/ _has_stale_entires bit set. What I wanted to achieve is 
guarantee that GC cleaners will see it. So, it is a StoreLoad barrier.

But on the second thought, I don't think it is necessary. Not much can 
be encapsulated into DependencyContext class. There should be external 
(w.r.t. dependency context) synchronization between removal tasks and 
cleaning tasks (cleaning starts after removal phase is over) to ensure 
cleaners don't see invalid state when some entry is dead (count == 0), 
but the context isn't marked as having stale entries. In this case, 
assertion in DependencyContext::expunge_stale_entries() fires.

Best regards,
Vladimir Ivanov