[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: Low-Overhead Heap Profiling
From:       Robbin Ehn <robbin.ehn () oracle ! com>
Date:       2018-01-26 13:51:08
Message-ID: 63a3a1ba-f566-8b9a-668c-f82e0479aef1 () oracle ! com
[Download RAW message or body]

Hi JC, (dropping compiler on mail thread)

On 01/26/2018 06:45 AM, JC Beyler wrote:
> 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/

Thanks!

I got this compile issue:
/home/rehn/source/jdk/small/open/src/hotspot/share/gc/shared/threadLocalAllocBuffer.hpp: 
In static member function 'static void 
G1ResManTLABCache::put(ThreadLocalAllocBuffer&, AllocationContext_t)':
/home/rehn/source/jdk/small/open/src/hotspot/share/gc/shared/threadLocalAllocBuffer.hpp:97:13: 
error: 'HeapWord* ThreadLocalAllocBuffer::hard_end()' is private
    HeapWord* hard_end();
              ^
/home/rehn/source/jdk/small/closed/src/hotspot/share/gc/g1/g1ResManTLABCache.cpp:52:50: 
error: within this context
    size_t remaining = pointer_delta(tlab.hard_end(), tlab.top());

> 
> I inlined my answers:
> 
> On Thu, Jan 25, 2018 at 1:15 AM, Robbin Ehn <robbin.ehn@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.

Looks good.

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

I talked a bit with GC folks about this today.
So actually the oop should be in the oopStorage and your should have a 
weakhandle to that oop (at least in the near future).
But this should work for now...
In the future when we have the oop in oppStorage you will not get called, so you 
will not know when the oops are dead, either GC must trigger a concurrent vm 
operation (e.g. _no_ safepoint operation) so we can clear dead oops or do 
periodic scanning.

It would be good with some input here from Thomas that knows what you are doing 
and knows GC.

> 
> 
>>
>> 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?

A thread calling getLiveTraces will be stopped in the HeapThreadTransition.
A safepoint don't allow any threads to be in_vm or to be in_java during 
safepoint. Only threads in native can execute, but they will be stopped on any 
transition. If a thread is in_vm the safepoint waits to a transition (locking a 
mutex is also transition to blocked). So if weak_oops is called you have an 
guarantee that no threads are executing inside the VM or executing Java code. 
(not counting GC threads of course)
This also means that the lock can never be contented when weak_oops is called, 
so it's not harmful.

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

Sounds good.

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

Looks good, thanks!

/Robbin

> 
> Thanks again!
> Jc
> 
[prev in list] [next in list] [prev in thread] [next in thread]