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

[John Rose]

On Nov 12, 2015, at 12:53 PM, John Rose <john.r.rose at oracle.com> wrote:
> 
> On Nov 11, 2015, at 7:43 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com <mailto:vladimir.x.ivanov at oracle.com>> wrote:
>> 
>> 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.
> 
> Indeed, that's the tough part.

Pending the updated webrev, I have to say that it looks pretty good.

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

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

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.

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

— John