[16] RFR: 8248391: Unify handling of all OopStorage instances in weak root processing

[Kim Barrett]

> On Jun 30, 2020, at 12:35 PM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> I talked to kim about this point:
> 
> "The GC (in particular, OopStorage) shouldn't know about client
> subsystems in order to record the notification function"
> 
> The current OopStorageSet implementation already knows about the different client subsystems, therefore I though that adding the notification registration there actually made sense. I've seen the alternative where the notification registration happens in the client subsystem, and that patch didn't look that great either, IMHO.
> 
> So, I decided to create a PoC to (almost) completely decouple the client subsystems and the OopStorageSet. With the patch, the OopStorageSet doesn't know anything about the existing subsystems. It only knows about the number of weak and strong OopStorages in the JVM. The need to know the exact numbers arise from the wish to statically know that number during compile time. I'm not sure it's important to keep this static nature of the number of OopStorages, instead of using a more dynamic approach, but I've left this as is for now.
> 
> https://cr.openjdk.java.net/~stefank/8248391/webrev.02.delta/
> https://cr.openjdk.java.net/~stefank/8248391/webrev.02

I have some comments, but because of vacations I'm providing them as a
further patch on top of Stefan's webrev.02, plus the description below.
(I'm abandoning the earlier never-published prototype derived from Erik's
changes in favor of using Stefan's additional PoC plus further changes based
on it.)  If this all gets reviewed before folks are back, I'll deal with
pushing, with Erik and Stefan as contributors.

full: https://cr.openjdk.java.net/~kbarrett/8248391/open.03/
incr: https://cr.openjdk.java.net/~kbarrett/8248391/open.03.inc/

Testing:
mach5 tier1- TBD
Locally ran new StringTableCleaningTest.java with a duration of 1/2 hour for
each collector (including Shenandoah) except Epsilon.

(Note that the baseline for the incremental patch is a slight modification
of Stefan's webrev.02, to deal with a recent graal update that moved JVMCI's
interface to OopStorage from jvmci.cpp to jvmciRuntime.cpp.)

> One problem with the current state of this patch is that during G1's heap initialization, it wants to eagerly figure out all OopStorage names. The problem is that with my patch OopStorages are initialized together with its subsystem, and the StringTable and ResolvedMethodTable are initialized after the heap. It's a bit unfortunate, and I would like to find a good solution for that, but for this PoC I've changed the G1Policy::phase_times to lazily created phase times instance. This moves the allocations to the first GC. I'm open for suggestions on what do do about that part.

I haven't come up with anything that seems better than Stefan's lazy
allocation of the G1GCPhaseTimes object to deal with the ordering problem
with subsystem registration of the OopStorage objects.  So I'm leaving that
as is, at least for now.

I think the proposed terminology for the GC -> OopStorage owner callback was
too vague and generic; "notify" and "notification" are relatively
content-free names.  So I've renamed the functions and types.  Also, I think
"can_notify" should be "should_notify" (and then adjusted for the generic
"notify").

I've also changed (and documented) the semantics; the number of dead
reported for an iteration is the number of entries skipped because they were
already NULL plus the number cleared by the iteration.  (So the total dead,
not just the recently deceased.)  That's the meaning in the old reporting
mechanism.  Earlier versions of this change instead invoked the callback
with just the number of newly cleared entries, with it being up to the
receiving subsystem to accumulate the total.  But there are race conditions
between GC and concurrent subsystem cleanup (like the ServiceThread's
concurrent cleaning of the StringTable) that make such accumulation by the
client potentially wildly inaccurate, leading to excess re-cleanings.  (Even
with the callback receiving the total dead there are races, but I think the
potential for significant inaccuracies leading to time wasting are much
less.)

A possible further change that I've left for later is that the
WeakProcessorPhaseTimeTracker gets just the cleared (newly dead) count.
(That hasn't been touched by any version of this change.)  I think the total
dead would be a more useful statistic for the associated logging.

The (anonymous) enum of OopStorageSet iterator indices no longer serves any
purpose; I've changed them to be ordinary in-class constants.  With the
original code the enum was used to automatically assign values to some of
the enumerators, but with the removal of named indices for each storage,
there was only one autmatically assigned enumerator.

I've changed WeakProcessor::Task to also use the new
OopStorageSetWeakParState.  This involved adding support for client access
to the states, similar to what's in the strong parstate.  (I'm not sure the
weak parstate should provide an oops_do that counts and reports the dead,
but I've left that for now.)

Fixed a "bug" that WeakProcessor::oops_do(OopClosure*) counted and reported
dead entries, even though it never clears entries.  It is used for
iterations that shouldn't be reporting dead, such as for the adjust pointers
phase of mark-sweep-compact.

Besides adapting to the above changes, I moved StringTable's clearing of the
_has_work flag to the end of its do_concurrent_work.  This avoids queuing
a new cleanup request in the middle of a concurrent cleanup that might
eliminate the need for further cleaning.  We could try to re-evaluate at the
end of the cleanup, but instead leave that to the next GC to trigger a new
round of cleanup if needed.  Also removed some inconsistencies between
ResolvedMethodTable and StringTable.

I've also added a test that beats on StringTable allocation and cleaning.