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

[Stefan Karlsson]

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

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");
+ _oop_storage->register_notification_function(&gc_notification);

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.

Thanks,
StefanK

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:
>>
>> Hi,
>>
>> 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!
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8248391
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8248391/webrev.00/
>>
>> Thanks,
>> /Erik
> 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.
>