Low-Overhead Heap Profiling

[JC Beyler]

Thanks Robbin for the reviews :)

The new full webrev is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.03/
The incremental webrev is here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.02_03/

I inlined my answers:

On Thu, Jan 25, 2018 at 1:15 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
> Hi JC, great to see another revision!
>
> ####
> heapMonitoring.cpp
>
> StackTraceData should not contain the oop for 'safety' reasons.
> When StackTraceData is moved from _allocated_traces:
> L452 store_garbage_trace(trace);
> it contains a dead oop.
> _allocated_traces could instead be a tupel of oop and StackTraceData thus
> dead oops are not kept.

Done I used inheritance to make the copier work regardless but the
idea is the same.

>
> You should use the new Access API for loading the oop, something like this:
> RootAccess<ON_PHANTOM_OOP_REF | AS_NO_KEEPALIVE>::load(...)
> I don't think you need to use Access API for clearing the oop, but it would
> look nicer. And you shouldn't probably be using:
> Universe::heap()->is_in_reserved(value)

I am unfamiliar with this but I think I did do it like you wanted me
to (all tests pass so that's a start). I'm not sure how to clear the
oop exactly, is there somewhere that does that, which I can use to do
the same?

I removed the is_in_reserved, this came from our internal version, I
don't know why it was there but my tests work without so I removed it
:)


>
> The lock:
> L424   MutexLocker mu(HeapMonitorStorage_lock);
> Is not needed as far as I can see.
> weak_oops_do is called in a safepoint, no TLAB allocation can happen and
> JVMTI thread can't access these data-structures. Is there something more to
> this lock that I'm missing?

Since a thread can call the JVMTI getLiveTraces (or any of the other
ones), it can get to the point of trying to copying the
_allocated_traces. I imagine it is possible that this is happening
during a GC or that it can be started and a GC happens afterwards.
Therefore, it seems to me that you want this protected, no?


>
> ####
> You have 6 files without any changes in them (any more):
> g1CollectedHeap.cpp
> psMarkSweep.cpp
> psParallelCompact.cpp
> genCollectedHeap.cpp
> referenceProcessor.cpp
> thread.hpp
>

Done.

> ####
> I have not looked closely, but is it possible to hide heap sampling in
> AllocTracer ? (with some minor changes to the AllocTracer API)
>

I am imagining that you are saying to move the code that does the
sampling code (change the tlab end, do the call to HeapMonitoring,
etc.) into the AllocTracer code itself? I think that is right and I'll
look if that is possible and prepare a webrev to show what would be
needed to make that happen.

> ####
> Minor nit, when declaring pointer there is a little mix of having the
> pointer adjacent by type name and data name. (Most hotspot code is by type
> name)
> E.g.
> heapMonitoring.cpp:711     jvmtiStackTrace *trace = ....
> heapMonitoring.cpp:733         Method* m = vfst.method();
> (not just this file)
>

Done!

> ####
> HeapMonitorThreadOnOffTest.java:77
> I would make g_tmp volatile, otherwise the assignment in loop may
> theoretical be skipped.
>

Also done!

Thanks again!
Jc