RFR: 8248391: Unify handling of all OopStorage instances in weak root processing
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Jul 14 21:20:37 UTC 2020
I really like the runtime changes in this patch, and the addition of the
StringTable clearing test. Components that wish to have their own
OopStorages will create and own them. I didn't review the GC changes.
On 7/7/20 9:47 AM, Kim Barrett wrote:
>> 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.
> 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/
> 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
> 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
> 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.
More information about the hotspot-gc-dev