Low-Overhead Heap Profiling

[Robbin Ehn]

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.

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)

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?

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

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

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

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

Thanks!

/Robbin

On 01/24/2018 01:40 AM, JC Beyler wrote:
> And it has been exactly two months since I posted an update here :)
> 
> Thanksgiving, Christmas, and handling
> https://bugs.openjdk.java.net/browse/JDK-8190862 will do that to you
> :)
> 
> I have gotten back to this item now that JDK-8190862 is done and I
> have the following webrev ready:
> http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.02/
> 
> With the incremental here:
> http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.01_02/
> 
> The updates are:
> 
> a) GetCachedTraces
> 
> - I added a GC collection if the GetLiveTraces is called, this is
> because you really want to know what is currently live when you call
> that method
> - If you are worried about performance and only care about what was
> live at the last GC, you can now call GetCachedTraces, which does not
> provoke a GC
> 
> Let me know if there are any questions here or concerns. I'm happy to
> explain and defend the choices :).
> Note: I added a new test for this and updated other tests to double
> check the live call. (see for example
> http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.01_02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorCachedTest.java.html)
> 
> b) Disabling samples wipes out the data
> 
> I had originally implemented for OpenJdk a version that keeps the data
> in memory after disabling the sampler, allowing a user to get traces
> post-sampling. Because of this, we would always do the weak_oops_do
> method, whether enabled or disabled. This led to a slight regression
> in performance for GC reference processing time. I had initially fixed
> this with a small "was this ever enabled" flag. This would have
> allowed a program that never uses this to not have a regression but a
> program that enables the disabled the code for the rest of the
> execution would still pay the price after disabling the sampler.
> 
> Because of this, I have moved things back to where they probably
> should be: if you disable the sampler, you lose the data. But this
> allows a simpler code path: if the sampler is disabled, skip over the
> GC weak_oops_do method.
> 
> Let me know what you think and happy 2018 to all!
> Jc
> 
> On Thu, Nov 23, 2017 at 7:20 AM, Thomas Schatzl
> <thomas.schatzl at oracle.com> wrote:
>> On Tue, 2017-11-21 at 13:50 -0800, JC Beyler wrote:
>>> Hi all,
>>>
>>> I have a new webrev here:
>>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.15/
>>>
>>> With the incremental one here:
>>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.14a_15/
>>>
>>> I think I did most items from Thomans and Robbin. I've especially
>>> added a new test:
>>
>> Thanks. Looks good.
>>
>> Thanks,
>>    Thomas
>>