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
<[email protected]> 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
