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

Thu Jun 25 15:39:08 UTC 2020

Hi Markus,

On 2020-06-25 16:55, Markus Gronlund wrote:
> Hi again Erik, thanks for the context and explanations.
>
> Inline.
>
> Markus
>
> -----Original Message-----
> From: Erik Österlund
> Sent: den 25 juni 2020 15:55
> To: Markus Gronlund <markus.gronlund at oracle.com>; Stefan Karlsson <stefan.karlsson at oracle.com>; Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
> Subject: Re: RFR: 8248216: JFR: Unify handling of all OopStorage instances in LeakProfiler root processing
>
> 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.
> [MG] If JVMTI could store its global oops into an OopStorage with a name detailed something with "JVMTI", instead of "VM Global" that would help (I take it that you are about to specialize the generic "VM Global" class).

Yes, that's right. And this patch is part of how JVMTI will get its own 
OopStorage with a better description.

>> 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).
> [
> [MG] I think "accessible" was not the best choice here - what I meant was something like "semantically related to a user API", i.e. traceable back to something for the end user - "JNI Global (Handles)" being a good example.

Right, these handles are never exposed to users. If you want to, we can 
call this root type something else. Finding a good name for something 
never exposed to users is tricky.I did my best!

>> 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).
>
> [MG] Thanks, my main concern was the stability of the names selected.

Okay.

>> 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.
>
> [MG] I know, that is why I raised it. It is not that we will create an API for a specific root set tied to a specific name, which would be counter to what I am trying to argue for here, but rather that the names of these OopStorages, now exposed, will become expected as part of user code, when processing events. Sounds like you have thought about that as part of GC log output.

Right. As you say, that is a situation we already have; when they change 
name, GC log parsers etc need updating.So now at least we will break 
things at precisely the same time, in a consistent way. ;)

> 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.
>
> [MG] Could not agree more, hence reason for asking about stability of the OopStorage names / classification. If we (JFR) need a higher abstraction, we might introduce a translation map later.

Sounds good.

> [MG] Ok Erik, looks good.

Thanks Markus!

/Erik

> 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