JDK-8171119: Low-Overhead Heap Profiling

[JC Beyler]

(Change of subject to contain the bug number :)).

New webrev is here:
Incremental:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.03_04/

Full webrev:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.04/

Inlined are my answers again :).

On Fri, Jan 26, 2018 at 5:51 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
> 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());

This is strange but I'm assuming it is because we are not working on
the same repo?

I used:
hg clone http://hg.openjdk.java.net/jdk/hs jdkhs-heap

I'll try a new clone on Monday and see. My new version moved hard_end
back to public so it should work now.



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

Fair enough, hopefully Thomas will chime in. Are you saying that this
first version could go in and we can work on a refinement? Or are you
saying I should work on this now at the same time and fix it before
this V1 goes in? (Just so I know :))

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

Looks like it yes, I removed it, ran my tests and they work so the new
webrev no longer has that mutex at all.

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

I'll look at this on Monday then!

Thanks for the reply and have a great weekend!
Jc

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