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

[Markus Gronlund]

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?

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).

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
>