Low-Overhead Heap Profiling
jcbeyler at google.com
Fri Jan 26 05:45:27 UTC 2018
Thanks Robbin for the reviews :)
The new full webrev is here:
The incremental webrev is here:
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!
> 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:
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
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):
> 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
> heapMonitoring.cpp:711 jvmtiStackTrace *trace = ....
> heapMonitoring.cpp:733 Method* m = vfst.method();
> (not just this file)
> I would make g_tmp volatile, otherwise the assignment in loop may
> theoretical be skipped.
More information about the hotspot-compiler-dev