RFR: ZGC: Add support for JFR leak profiler

[Erik Österlund]

Hi Erik,

Thanks for the review!

/Erik

> On 4 Dec 2019, at 18:25, Erik Gahlin <erik.gahlin at oracle.com> wrote:
> 
> Looks good.
> 
> Erik
> 
>> On 2019-12-04 17:16, Erik Österlund wrote:
>> Hi Stefan,
>> 
>> Thanks for the review.
>> 
>>> On 2019-12-04 14:55, Stefan Karlsson wrote:
>>> Hi Erik,
>>> 
>>> This looks good to me!
>> 
>> Thanks!
>> 
>>> I think it would be good for the code to limit the number of UnifedOopRef::addr calls, since those are unsafe ways into the the addr and _could_ be abused to go around the UnifiedOopRef encapsulation. Maybe something for the future.
>> 
>> Yeah. I wrote that function because I noticed that the address was used with literally every single
>> address looking type I could imagine, in different places (uintptr_t, address, char*, oop*, void*,
>> you name it). So I just wrote the addr<T> function to be able to perform this change as mechanically
>> as possible.
>> 
>>> Just a small nit:
>>> 
>>> https://cr.openjdk.java.net/~eosterlund/8235174/webrev.00/src/hotspot/share/jfr/leakprofiler/checkpoint/eventEmitter.cpp.frames.html 
>>> 
>>> Now that you've limited the use of object_addr, maybe you could also limit its scope by moving
>>> 
>>> 113   const oop* object_addr = sample->object_addr();
>>> 
>>> to around:
>>> 
>>> 123 edge = edge_store->put(UnifiedOopRef::encode_in_native(object_addr));
>> 
>> Absolutely.
>> 
>> Here is a new webrev with your proposed change, and with some changes from Markus (big thanks to Markus for looking at this in great detail):
>> http://cr.openjdk.java.net/~eosterlund/8235174/webrev.01/
>> 
>> Incremental:
>> http://cr.openjdk.java.net/~eosterlund/8235174/webrev.00_01/
>> 
>> His changes mostly massage some types and order some includes. For example Markus prefers passing around UnifiedOopRef
>> as non-const (now that it is a POD, it's like passing around an integer).
>> 
>> The most important change though, was fixing a bug in my patch by making sure that ObjectSampler::weak_oops_do
>> actually clears the oops that are dead. I thought it did, like the other weak_oops_do functions. Turns out
>> that it has not been needed until now. The oops were not cleared, yet the ObjectSample is reused as some
>> kind of cached object (presumably to call malloc less). The consequence was that when the ObjectSample gets subsequently reused with a dead oop in it, set_object() now using NativeAccess<ON_PHANTOM_OOP_REF>::oop_store
>> with my patch crashes things, because the G1 barrier backend SATB enqueues what was there in memory before
>> (which is dead garbage memory).
>> 
>> Now one might argue that it's really silly of G1 barriers to SATB enqueue on non-strong stores (because the very
>> snapshot that SATB tracks is the strong graph, which is the graph being marked concurrently, nothing else). But I
>> would like to change that heuristic weirdness in a separate change. In this patch I am happy with the object simply
>> being cleared in weak_oops_do, like it is in all other weak_oops_do functions. That fixes the issue.
>> 
>> Before there was a separate _dead boolean to track that the ObjectSample is dead. Now that the oop is cleared
>> by the GC when it dies, the boolean is no longer needed, because the is_dead() function can simply look at the oop.
>> 
>> Thanks,
>> /Erik
>> 
>>> Thanks,
>>> StefanK
>>> 
>>> On 2019-12-02 11:09, erik.osterlund at oracle.com wrote:
>>>> Hi,
>>>> 
>>>> The JFR leak profiler is currently not supported on ZGC, because it has not been accessorized appropriately.
>>>> This patch adds the required Access API sprinkling necessary, to enable this for ZGC. I did the following:
>>>> 
>>>> 1) Renamed UnifiedOop to UnifiedOopRef, because it describes the reference to an oop, not the oop itself.
>>>> 2) Changed UnifiedOopRef to be a POD, instead of an AllStatic that passes around its encoded data as
>>>>    const oop* (even though it might be a tagged pointer to a narrow oop), improving type safty.
>>>> 3) Added another tag bit to UnifiedOopRef, allowing it to keep track of whether the described location is
>>>>    IN_NATIVE or IN_HEAP. The dereference function performs the appropriate NativeAccess/HeapAccess with the
>>>>    AS_NO_KEEPALIVE decotator, which is fine because the leak profiler does not create any edges to oops
>>>>    found during traversal.
>>>> 4) Removed code blob oop iteration, because it can't be supported appropriately by the current UnifiedOopRef
>>>>    mechanism (because oops are misaligned, and UnifiedOopRef requires tag bits - I believe this has never
>>>>    worked properly)
>>>> 6) Accessorized loads on strong roots to NativeAccess<AS_NO_KEEPALIVE>::oop_load. JFR leak profiler never
>>>>    creates new references to oops that are visited, so it does not need to keep things alive
>>>> 7) Explicitly told the oop iteration closures to not visit referents. The default referent strategy used
>>>>    before is DO_DISCOVERY, which doesn't seem right.
>>>> 8) Fixed a pre-existing issue where the array index reported in the leak chain was calculated incorrectly
>>>>    as the byte offset from the array base. It should be scaled by the element length of the array.
>>>> 
>>>> Ran some JFR leak profiler tests, and some ad-hoc leak profiling in various applications. Also checked that
>>>> there were no observable perf regressions in the iterations, and there were not.
>>>> 
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8235174
>>>> 
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8235174/webrev.00/
>>>> 
>>>> Thanks,
>>>> /Erik
>>> 
>>