RFR: 8248216: JFR: Unify handling of all OopStorage instances in LeakProfiler root processing

[coleen.phillimore at oracle.com]

On 6/25/20 8:10 AM, Markus Gronlund wrote:
> Hi Erik,
>
> If I understand this correctly, there exist some classification of the different OopStorages, maintained by OopStoragesSet? Where 'strong' is the class for "real" roots (non-weak), for example "JNI Global"?
> And it is the selected iterator that predicates which OopStorages you access, so using OopStorageSet::strong_iterator() delivers only the storages classified as roots? There are only two strong OopStorages at this point: "JNI Global" which I assume is for JNI Global Handles and "VM Global" which I don’t really know what it is, probably a VM internal equivalent of not using a Global JNI Handle? Looks like it's currently being used only by JVMTI?

The vm_global() OopStorage is also being used for the SystemDictionary 
roots, and I have patches to add JVMTI, Management and  Universe roots.  
I also have a patch for JFR but it doesn't work at all.   The purpose is 
to not make GC walk through runtime and serviceability data structures 
in a pause.  OopStorage can be nicely parallel and concurrent.

More below:
>
> Also a new Type is defined "_global_oop_handle" ? I assume this is what you get to access an oop in the "VM Global" OopStorage? If so, this is not accessible to the end user.
>
> Implementation looks reasonable, but for Leak Profiler, I am a bit concerned that users will get too much internals, detailing the VM's view of the world, not necessarily what the user expects and can work with at their level of abstraction. We already have a bit much of that already (Type: "_handle_area" and several internal subsystems, for example).

So one of the motivators of having multiple global strong OopStorage 
areas is so that the storage can be walked together by the GC but 
there'll be different logging and accounting, so that a user (and the 
GC) can see how much time or how many oops are being used by various 
subsystems that use OopStorage.

The patches I have above, will eventually add dedicated OopStorages for 
them: like vm_service_globals() for JVMTI/Management oops.  The name 
being very negotiable.  Part of this work in this patch is to add the 
capability to add these in only one place.

Universe and SystemDictionary roots are global, and are not released, so 
they should still belong to the vm_global() OopStorage.

Thanks,
Coleen
>
> Leak Profiler will have a dependency on the internal names defined for the different OopStorages, which are no longer opaque but will be exposed externally. And this exposure can be stronger than a "debug" message, which is again VM internal, but it could potentially be tied to an API.
>
> Thanks
> Markus
>
>
>
>
> -----Original Message-----
> From: Erik Österlund
> Sent: den 24 juni 2020 15:29
> To: Stefan Karlsson <stefan.karlsson at oracle.com>; Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
> Cc: Markus Gronlund <markus.gronlund at oracle.com>
> Subject: Re: RFR: 8248216: JFR: Unify handling of all OopStorage instances in LeakProfiler root processing
>
> Hi Stefan,
>
> Thanks for the review.
>
> Updated to use your new API:
> http://cr.openjdk.java.net/~eosterlund/8248216/webrev.01/
>
> Also added Markus to the conversation.
>
> Thanks,
> /Erik
>
> On 2020-06-24 14:36, Stefan Karlsson wrote:
>> Hi Erik,
>>
>> On 2020-06-24 11:13, Erik Österlund wrote:
>>> Hi,
>>>
>>> We want it to be as convenient as possible for the runtime to add new
>>> OopStorage instances. Therefore, rather than picking specific
>>> OopStorages during LeakProfiler root processing, the
>>> OopStorageSet::strong_iterator() should be used instead. That way,
>>> once a new OopStorage is added to the OopStorageSet, it is
>>> automatically plugged in to the JFR LeakProfiler.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8248216
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8248216/webrev.00/
>> https://cr.openjdk.java.net/~eosterlund/8248216/webrev.00/src/hotspot/
>> share/jfr/leakprofiler/chains/rootSetClosure.cpp.udiff.html
>>
>>
>> I've now pushed a change so that you can simplify:
>>
>> + for (OopStorageSet::Iterator it = OopStorageSet::strong_iterator();
>> !it.is_end(); ++it) {
>> + it->oops_do(this);
>> + } to: OopStorageSet::strong_oops_do(this); The rest looks fine, but
>> should maybe be reviewed on hotspot-jfr-dev. Thanks, StefanK
>>
>>> Thanks,
>>> /Erik