<div dir="ltr">Dear all,<div><br></div><div>I've updated the webrev to:</div><div><a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.02/" target="_blank">http://cr.openjdk.java.net/~<wbr>rasbold/8171119/webrev.02/</a><br></div><div><br></div><div>Robbin,</div><div>I believe I have addressed most of your items with webrev 02:</div><div>  - I added a JTreg test to show how it works:</div><div>      <a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.02/raw_files/new/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c" target="_blank">http://cr.openjdk.java.net/~<wbr>rasbold/8171119/webrev.02/raw_<wbr>files/new/test/serviceability/<wbr>jvmti/HeapMonitor/<wbr>libHeapMonitor.c</a></div><div> - I've modified the code to use its own data structures both internally and externally, this will make it easier to move out of AsyncGetCallTrace as we move forward, that is still on my TODOs</div><div> - I cleaned up the JVMTI API by passing a structure that handles the num_traces and put in a ReleaseTraces as well</div><div> - I cleaned up other issues as well.</div><div><br></div><div>However, I have three questions, which are probably because I'm new in this community:</div><div> 1) My previous webrevs were based off of JDK9 by mistake. When I took JDK10 via : hg clone <a href="http://hg.openjdk.java.net/jdk10/jdk10" target="_blank">http://hg.openjdk.java.net/<wbr>jdk10/jdk10</a> jdk10</div><div>     - I don't see code compatible with what you were showing (ie your patches don't make sense for that code base; ex: klass is still accessed via klass() for example in <span style="color:rgb(0,0,0);white-space:pre-wrap">collectedHeap.inline.hpp</span>)</div><div>     - Would you know what is the right hg clone command so we are working on the same code base?</div><div><br></div><div> 2) You mentioned I was using os::malloc, new, NEW_C_HEAP_ARRAY; I cleaned out the os::malloc but which of the new vs NEW_C_HEAP_ARRAY should I use. It might be that I don't understand when one uses one or the other but I see both used around the code base?</div><div>   - Is it that new is to be used for anything internal and NEW_C_HEAP_ARRAY anything provided to the JVMTI users outside of the JVM?</div><div><br></div><div> 3) Casts: same kind question: which should I use. The code was using a bit of everything, I'll refactor it entirely but I was not clear if I should go to C casts or C++ casts as I see both in the codebase. What is the convention I should use?</div><div><br></div><div>Final notes on this webrev:<br></div><div>  - I am still missing:</div><div>    - Putting a TLAB implementation so that we can compare both webrevs</div><div>    - Have not tried to circumvent AsyncGetCallTrace</div><div>    - Putting in the handling of GC'd objects</div><div>    - Fix a stack walker issue I have seen, I think I know the problem and will test that theory out for the next webrev</div><div><br></div><div>I will work on integrating those items for the next webrev!</div><div><br></div><div>Thanks for your help,</div><div>Jc</div><div><br></div><div>Ps:  I tested this on a new repo:</div><div><br></div><div>hg clone <a href="http://hg.openjdk.java.net/jdk10/jdk10" target="_blank">http://hg.openjdk.java.net/<wbr>jdk10/jdk10</a> jdk10<br></div><div>... building it</div><div>cd test</div><div><div>jtreg -nativepath:<path-to-jdk10>/build/linux-x86_64-normal-server-release/support/test/hotspot/jtreg/native/lib/ -jdk <path-to-jdk10>/linux-x86_64-normal-server-release/images/jdk ../hotspot/test/serviceability/jvmti/HeapMonitor/</div></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 4, 2017 at 11:21 PM, <a href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> <span dir="ltr"><<a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Robbin,<br>
<br>
Thank you for forwarding!<br>
I will review it.<br>
<br>
Thanks,<br>
Serguei<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On 5/4/17 02:13, Robbin Ehn wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
To me the compiler changes looks what is expected.<br>
It would be good if someone from compiler could take a look at that.<br>
Added compiler to mail thread.<br>
<br>
Also adding Serguei, It would be good with his view also.<br>
<br>
My initial take on it, read through most of the code and took it for a ride.<br>
<br>
##############################<br>
- Regarding the compiler changes: I think we need the 'TLAB end' trickery (mentioned by Tony P)<br>
instead of a separate check for sampling in fast path for the final version.<br>
<br>
##############################<br>
- This patch I had to apply to get it compile on JDK 10:<br>
<br>
diff -r ac3ded340b35 src/share/vm/gc/shared/collect<wbr>edHeap.inline.hpp<br>
--- a/src/share/vm/gc/shared/colle<wbr>ctedHeap.inline.hpp    Fri Apr 28 14:31:38 2017 +0200<br>
+++ b/src/share/vm/gc/shared/colle<wbr>ctedHeap.inline.hpp    Thu May 04 10:22:56 2017 +0200<br>
@@ -87,3 +87,3 @@<br>
     // support for object alloc event (no-op most of the time)<br>
-    if (klass() != NULL && klass()->name() != NULL) {<br>
+    if (klass != NULL && klass->name() != NULL) {<br>
       Thread *base_thread = Thread::current();<br>
diff -r ac3ded340b35 src/share/vm/runtime/heapMonit<wbr>oring.cpp<br>
--- a/src/share/vm/runtime/heapMon<wbr>itoring.cpp    Fri Apr 28 14:31:38 2017 +0200<br>
+++ b/src/share/vm/runtime/heapMon<wbr>itoring.cpp    Thu May 04 10:22:56 2017 +0200<br>
@@ -316,3 +316,3 @@<br>
   JavaThread *thread = reinterpret_cast<JavaThread *>(Thread::current());<br>
-  assert(o->size() << LogHeapWordSize == byte_size,<br>
+  assert(o->size() << LogHeapWordSize == (long)byte_size,<br>
          "Object size is incorrect.");<br>
<br>
##############################<br>
- This patch I had to apply to get it not asserting during slowdebug:<br>
<br>
--- a/src/share/vm/runtime/heapMon<wbr>itoring.cpp    Fri Apr 28 15:15:16 2017 +0200<br>
+++ b/src/share/vm/runtime/heapMon<wbr>itoring.cpp    Thu May 04 10:24:25 2017 +0200<br>
@@ -32,3 +32,3 @@<br>
 // TODO(jcbeyler): should we make this into a JVMTI structure?<br>
-struct StackTraceData {<br>
+struct StackTraceData : CHeapObj<mtInternal> {<br>
   ASGCT_CallTrace *trace;<br>
@@ -143,3 +143,2 @@<br>
 StackTraceStorage::StackTrace<wbr>Storage() :<br>
-    _allocated_traces(new StackTraceData*[MaxHeapTraces]<wbr>),<br>
     _allocated_traces_size(MaxHea<wbr>pTraces),<br>
@@ -147,2 +146,3 @@<br>
     _allocated_count(0) {<br>
+  _allocated_traces = NEW_C_HEAP_ARRAY(StackTraceDat<wbr>a*, MaxHeapTraces, mtInternal);<br>
   memset(_allocated_traces, 0, sizeof(*_allocated_traces) * MaxHeapTraces);<br>
@@ -152,3 +152,3 @@<br>
 StackTraceStorage::~StackTrac<wbr>eStorage() {<br>
-  delete[] _allocated_traces;<br>
+  FREE_C_HEAP_ARRAY(StackTraceDa<wbr>ta*, _allocated_traces);<br>
 }<br>
<br>
- Classes should extend correct base class for which type of memory is used for it e.g.: CHeapObj<mt????> or StackObj or AllStatic<br>
- The style in heapMonitoring.cpp is a bit different from normal vm-style, e.g. using C++ casts instead of C. You mix NEW_C_HEAP_ARRAY, os::malloc and new.<br>
- In jvmtiHeapTransition.hpp you use C cast instead.<br>
<br>
##############################<br>
- This patch I had apply to get traces without setting an ‘unrelated’ capability<br>
- Should this not be a new capability?<br>
<br>
diff -r c02a5d8785bf src/share/vm/prims/forte.cpp<br>
--- a/src/share/vm/prims/forte.<wbr>cpp    Fri Apr 28 15:15:16 2017 +0200<br>
+++ b/src/share/vm/prims/forte.<wbr>cpp    Thu May 04 10:24:25 2017 +0200<br>
@@ -530,6 +530,6 @@<br>
<br>
-  if (!JvmtiExport::should_post_cla<wbr>ss_load()) {<br>
+/*  if (!JvmtiExport::should_post_cla<wbr>ss_load()) {<br>
     trace->num_frames = ticks_no_class_load; // -1<br>
     return;<br>
-  }<br>
+  }*/<br>
<br>
##############################<br>
- forte.cpp: (I know this is not part of your changes but)<br>
find_jmethod_id_or_null give me NULL for my test.<br>
It looks like we actually want the regular jmethod_id() ?<br>
<br>
Since we are the thread we are talking about (and in same ucontext) and thread is in vm and have a last java frame,<br>
I think most of the checks done in AsyncGetCallTrace is irrelevant, so you should be-able to call forte_fill_call_trace_given_to<wbr>p directly.<br>
But since we might need jmethod_id() if possible to avoid getting method id NULL,<br>
we need some fixes in forte code, or just do the vframStream loop inside heapMonitoring.cpp and not use forte.cpp.<br>
<br>
Something like:<br>
<br>
  if (jthread->has_last_Java_frame(<wbr>)) { // just to be safe<br>
    vframeStream vfst(jthread);<br>
    while (!vfst.at_end()) {<br>
      Method* m = vfst.method();<br>
      m->jmethod_id();<br>
      m->line_number_from_bci(vfst.b<wbr>ci());<br>
      vfst.next();<br>
    }<br>
<br>
- This is a bit confusing in forte.cpp, trace->frames[count].lineno = bci.<br>
Line number should be m->line_number_from_bci(bci);<br>
Do the heapMonitoring suppose to trace with bci or line number?<br>
I would say bci, meaning we should either rename ASGCT_CallFrame→lineno or use another data structure which says bci.<br>
<br>
##############################<br>
- // TODO(jcbeyler): remove this extra code handling the extra trace for<br>
Please fix all these TODO's :)<br>
<br>
##############################<br>
- heapMonitoring.hpp:<br>
// TODO(jcbeyler): is this algorithm acceptable in open source?<br>
<br>
Why is this comment here? What is the implication?<br>
Have you tested any simpler algorithm?<br>
<br>
##############################<br>
- Create a sanity jtreg test. (./hotspot/make/test/JtregNati<wbr>ve.gmk for building the agent)<br>
<br>
##############################<br>
- monitoring_period vs HeapMonitorRate, pick rate or period.<br>
<br>
##############################<br>
- globals.hpp<br>
Why is MaxHeapTraces not settable/overridable from jvmti interface? That would be handy.<br>
<br>
##############################<br>
- jvmtiStackTraceData + ASGCT_CallFrame memory<br>
Are the agent suppose to loop through and free all ASGCT_CallFrame?<br>
Wouldn't it be better with some kinda protocol, like:<br>
(*jvmti)->GetLiveTraces(jvmti, &stack_traces, &num_traces);<br>
(*jvmti)->ReleaseTraces(jvmti, stack_traces, num_traces);<br>
<br>
Also using another data structure that have num_traces inside it simplifies things.<br>
So I'm not convinced using the async structure is the best way forward.<br>
<br>
<br>
I have more questions, but I think it's better if you respond and update the code first.<br>
<br>
Thanks!<br>
<br>
/Robbin<br>
<br>
<br>
On 04/21/2017 11:34 PM, JC Beyler wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi all,<br>
<br>
I've added size information to the allocation sampling system. This allows the callback to remember the size of each sampled allocation.<br>
<a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.01/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/8171119/webrev.01/</a><br>
<br>
The new webrev.01 also adds the actual heap monitoring sampling system in files:<br>
<a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.01/src/share/vm/runtime/heapMonitoring.cpp.patch" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/8171119/webrev.01/src/sh<wbr>are/vm/runtime/heapMonitoring.<wbr>cpp.patch</a> <br>
and<br>
<a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.01/src/share/vm/runtime/heapMonitoring.hpp.patch" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/8171119/webrev.01/src/sh<wbr>are/vm/runtime/heapMonitoring.<wbr>hpp.patch</a> <br>
<br>
My next step is to add the GC part to the webrev, which will allow users to determine what objects are live and what are garbage.<br>
<br>
Thanks for your attention and let me know if there are any questions!<br>
<br>
Have a wonderful Friday!<br>
Jc<br>
<br>
On Mon, Apr 17, 2017 at 12:37 PM, JC Beyler <<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a> <mailto:<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>>> wrote:<br>
<br>
    Hi all,<br>
<br>
    I worked on getting a few numbers for overhead and accuracy for my feature. I'm unsure if here is the right place to provide the full data, so I am just summarizing<br>
    here for now.<br>
<br>
    - Overhead of the feature<br>
<br>
    Using the Dacapo benchmark (<a href="http://dacapobench.org/" rel="noreferrer" target="_blank">http://dacapobench.org/</a>). My initial results are that sampling provides 2.4% with a 512k sampling, 512k being our default setting.<br>
<br>
    - Note: this was without the tradesoap, tradebeans and tomcat benchmarks since they did not work with my JDK9 (issue between Dacapo and JDK9 it seems)<br>
    - I want to rerun next week to ensure number stability<br>
<br>
    - Accuracy of the feature<br>
<br>
    I wrote a small microbenchmark that allocates from two different stacktraces at a given ratio. For example, 10% of stacktrace S1 and 90% from stacktrace S2. The<br>
    microbenchmark was run 20 times, I averaged the results and looked for accuracy. It seems that statistically it is sound since if I allocated10% S1 and 90% S2, with a<br>
    sampling rate of 512k, I obtained 9.61% S1 and 90.49% S2.<br>
<br>
    Let me know if there are any questions on the numbers and if you'd like to see some more data.<br>
<br>
    Note: this was done using our internal JDK8 implementation since the webrev provided by <a href="http://cr.openjdk.java.net/~rasbold/heapz/webrev.00/index.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/heapz/webrev.00/index.ht<wbr>ml</a><br>
<<a href="http://cr.openjdk.java.net/~rasbold/heapz/webrev.00/index.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~r<wbr>asbold/heapz/webrev.00/index.h<wbr>tml</a>> does not yet contain the whole implementation and therefore would have been misleading.<br>
<br>
    Thanks,<br>
    Jc<br>
<br>
<br>
    On Tue, Apr 4, 2017 at 3:55 PM, JC Beyler <<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a> <mailto:<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>>> wrote:<br>
<br>
        Hi all,<br>
<br>
        To move the discussion forward, with Chuck Rasbold's help to make a webrev, we pushed this:<br>
<a href="http://cr.openjdk.java.net/~rasbold/heapz/webrev.00/index.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/heapz/webrev.00/index.ht<wbr>ml</a> <<a href="http://cr.openjdk.java.net/~rasbold/heapz/webrev.00/index.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~r<wbr>asbold/heapz/webrev.00/index.h<wbr>tml</a>><br>
        415 lines changed: 399 ins; 13 del; 3 mod; 51122 unchg<br>
<br>
        This is not a final change that does the whole proposition from the JBS entry: <a href="https://bugs.openjdk.java.net/browse/JDK-8177374" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/<wbr>browse/JDK-8177374</a><br>
<<a href="https://bugs.openjdk.java.net/browse/JDK-8177374" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net<wbr>/browse/JDK-8177374</a>>; what it does show is parts of the implementation that is proposed and hopefully can start the conversation going<br>
        as I work through the details.<br>
<br>
        For example, the changes to C2 are done here for the allocations: <a href="http://cr.openjdk.java.net/~rasbold/heapz/webrev.00/src/share/vm/opto/macro.cpp.patch" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/heapz/webrev.00/src/shar<wbr>e/vm/opto/macro.cpp.patch</a><br>
<<a href="http://cr.openjdk.java.net/~rasbold/heapz/webrev.00/src/share/vm/opto/macro.cpp.patch" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~r<wbr>asbold/heapz/webrev.00/src/sha<wbr>re/vm/opto/macro.cpp.patch</a>><br>
<br>
        Hopefully this all makes sense and thank you for all your future comments!<br>
        Jc<br>
<br>
<br>
        On Tue, Dec 13, 2016 at 1:11 PM, JC Beyler <<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a> <mailto:<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>>> wrote:<br>
<br>
            Hello all,<br>
<br>
            This is a follow-up from Jeremy's initial email from last year:<br>
<a href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2015-June/017543.html" rel="noreferrer" target="_blank">http://mail.openjdk.java.net/p<wbr>ipermail/serviceability-dev/20<wbr>15-June/017543.html</a> <<a href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2015-June/017543.html" rel="noreferrer" target="_blank">http://mail.openjdk.java.net/<wbr>pipermail/serviceability-dev/2<wbr>015-June/017543.html</a>> <br>
<br>
            I've gone ahead and started working on preparing this and Jeremy and I went down the route of actually writing it up in JEP form:<br>
            <a href="https://bugs.openjdk.java.net/browse/JDK-8171119" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/<wbr>browse/JDK-8171119</a><br>
<br>
            I think original conversation that happened last year in that thread still holds true:<br>
<br>
              - We have a patch at Google that we think others might be interested in<br>
                 - It provides a means to understand where the allocation hotspots are at a very low overhead<br>
                 - Since it is at a low overhead, we can leave it on by default<br>
<br>
            So I come to the mailing list with Jeremy's initial question:<br>
            "I thought I would ask if there is any interest / if I should write a JEP / if I should just forget it."<br>
<br>
            A year ago, it seemed some thought it was a good idea, is this still true?<br>
<br>
            Thanks,<br>
            Jc<br>
<br>
<br>
<br>
<br>
<br>
</blockquote></blockquote>
<br>
</div></div></blockquote></div><br></div>