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