RFR: 8248391: Unify handling of all OopStorage instances in weak root processing
stefan.karlsson at oracle.com
Tue Jun 30 16:35:31 UTC 2020
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.
To see the effect of this patch, take a look at the StringTable changes.
The StringTable now creates it's own OopStorage by calling the
OopStorageSet::create_weak() factory function, that both creates the
OopStorage and registers it so that the GCs will visit it during their
iterations over OopStorages:
+ _oop_storage = OopStorageSet::create_weak("StringTable Weak");
The associated WeakHandles use the StringTable internal/private
reference to its OopStorage:
// Callers have already looked up the String using the jchar* name, so just go to add.
- WeakHandle wh(OopStorageSet::string_table_weak(), string_h);
+ WeakHandle wh(_oop_storage, string_h);
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.
On 2020-06-30 09:10, Kim Barrett wrote:
>> On Jun 26, 2020, at 9:06 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>> Today, when a weak OopStorage is added, you have to plug it in explicitly to ZGC, Shenandoah and the WeakProcessor, used by Shenandoah, Serial, Parallel and G1. Especially when the runtime data structure associated with an OopStorage needs a notification when oops die. Then you have to explicitly plug in notification code in various places in GC code.
>> It would be ideal if this process could be completely automated.
>> This patch allows each OopStorage to have an associated notification function. This is a callback function into the runtime, stating how many oops have died this GC cycle. This allows runtime data structures to perform accounting for how large part of the data structure needs cleaning, and whether to trigger such cleaning or not.
>> So the interface between the GCs to the OopStorage is that during weak processing, the GC promises to call the callback function with how many oops died. Some shared infrastructure makes this very easy for the GCs.
>> Weak processing now uses the OopStorageSet iterators across all GCs, so that adding a new weak OopStorage (even with notification functions) does not require touching any GC code.
>> Kudos to Zhengyu for providing some Shenandoah code for this, and StefanK for pre-reviewing it. Also, I am about to go out of office now, so StefanK promised to take it from here. Big thanks for that!
> While I approve of the goal for this change, I have a couple of issues
> of some significance with the approach taken.
> I think the way the gc notifications are being set up is backward.
> The GC (in particular, OopStorage) shouldn't know about client
> subsystems in order to record the notification function. That seems
> like a dependency inversion. Rather, I think client subsystems should
> be registering callbacks with the GC.
> I've never liked the existing mechanism used to gather iteration
> statistics (number of entries cleared, skipped, processed...). It
> always seemed rather bolted on after the fact. This change continues
> that. I'd rather take the opportunity to improve that, by improving
> OopStorage in this area.
> I'm working on a reworked version of Erik's changes. So far it's
> looking pretty good. Some of Erik's work I'm using as-is, but some
> parts not so much. I'm not yet ready to send anything out for review,
> but mentioning it here so others hopefully don't spend too much time
> reviewing the existing change.
> I've discussed this with Stefan, and he's agreed to hand off
> responsibility for this project to me.
More information about the hotspot-gc-dev