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

Thu Jun 25 13:55:29 UTC 2020

Hi Markus,

Thanks for reviewing this.

On 2020-06-25 14:10, 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"?

Something like that.

> 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?

Right. The reason I am proposing this patch is that this is about to 
change. Pretty much all global roots (non-thread roots) are moving into 
different OopStorages with appropriate names. So it won't be two for 
long. There will be only different OopStorages denoting the different 
root sets + thread roots + maybe CLDs depending on whether we fix them 
or not. To make the conversion easier, we are making code that visits 
the OopStorages automatic, so that adding more OopStorage objects is 
easier, without having to go through every single GC + leak profiler to 
update their root processing code to include yet another OopStorage and 
again do the same thing with that one that is done for all the other ones.

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

Right. These things used to be stray oops in the C++ code, reachable 
through oops_do functions. Now they are being moved to OopStorage 
instances and are accessed through global handles, very similar to JNI 
handles, but are not part of JNI. And like all those roots, they are 
indeed not accessible to the end user (unless you count heap traversal 
APIs).

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

I am not exposing any information that is not already exposed. You 
already have a set of roots and give them names (including the today 2 
OopStorage objects). All I changed is so the OopStorage roots get their 
human readable names from their OopStorage instead, which defines such a 
human readable name. And these are not internal names for us, they are 
meant to be user facing, and will be consistent with what you see in GC 
log output, etc, so that users can match them in a consistent way. If 
the human readable names defined for our OopStorage objects are bad, 
then it sounds like what you really want is to change their name strings 
(defined in oopStorageSet.cpp).

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

Again, the names I print are not internal names, they are strings that 
have been selected as user facing, just like the strings you had defined 
in JFR before. The difference is only in the exact name string chosen, 
but they are both supposed to be human readable. And now they can also 
be consistent.

Having said that, I have strong reservations about tying any root set 
name to an API. JVMTI does that and it is a nightmare.

1) The set of VM roots are constantly changing, and any code relying on 
HotSpot having the same set of roots, set in stone, is bound to break. 
For example, I am about to remove the ObjectMonitor roots, because they 
will simply not be strong roots any more. Anyone expecting them to be 
around will fail. A whole subsystem could move its roots out to Java for 
whatever reason. We can't know how roots will change, only that they 
definitely can and will change. The root sets of HotSpot are and always 
will be, an internal implementation detail of our JVM.

2) Even if you still want to tie root sets to an API, this should not be 
done with human readable strings that you are not allowed to alter. It 
should be done with some kind of enumeration or something, that is meant 
for code and not humans. JVMTI root sets do this, and I still think it 
really should not be done as explained above.

Thanks,
/Erik

> 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