<div dir="ltr">Hi Serguei,<div><br></div><div>I'm answering your emails in one spot to make it easier for everyone to follow. I think I've gathered all your remarks in one spot:</div><div><br></div><div>Before I start: No worries for going on vacation, I am also soon going for a few weeks :)</div><div><br></div><div>1) hg import:</div><div><br></div><div>Strange, I tried this to be sure I was not doing something wrong:</div><div><div>hg clone <a href="http://hg.openjdk.java.net/jdk10/hs">http://hg.openjdk.java.net/jdk10/hs</a> jdk10hs-testpatch</div><div>bash ./get_source.sh</div><div>wget <a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.07/hotspot.patch">http://cr.openjdk.java.net/~rasbold/8171119/webrev.07/hotspot.patch</a></div><div>hg import ../hotspot.patch</div><div>hg import ../hotspot.patch </div><div>applying ../hotspot.patch</div><div>abort: no username supplied (see "hg help config")</div><div>hg status</div><div><br></div></div><div>This worked for me.</div><div><br></div><div>Is it by any chance that I recently moved to the hs branch?</div><div><br></div><div><br></div><div>2) JVMTI System</div><div><br></div><div>Yes this system seems slightly different than the turn on/set your callbacks and event notifications...</div><div><br></div><div>  - We talked about having a callback system but it is for a different use case:</div><div>      - Have a callback for each sampled allocation</div><div>         - Theoretically I suppose we could do this here and move the code from heapMonitoring into a library outside of the JVM to offer the same effect. I'm not sure on the overhead though but I can do a comparison. I've had conversations about this and Jeremy was worried that a callback system would add more overhead instead of a full sampler inside the JVM that you can poll like we do here</div><div><br></div><div>  - Start/stop and initialization/finalization can definitely be separated, I've just left it as the code was written originally but could move it, it might simplify a bit the code too</div><div>      - Because of our way of doing it, it seems that we keep the capability on always, though you could imagine that you would only need the capability for the initialization and all other methods fail/do nothing if not initialized. That would allow us to disable the capability later.</div><div>         - What do you think?</div><div><br></div><div>  - I think I will go for single agent, I don't think we care about multi-agent right now, so I'll stay on this path and leave it on the onload_solo</div><div><br></div><div><br></div><div>3) Capabilities:<br></div><div>  - I had already added the can_sample_heap capability (not sure if you saw it) and had seen that it does work as I thought it would. I added a JTreg test to check each method here:</div><div>       Search for <span style="color:rgb(0,0,0);white-space:pre-wrap">Java_MyPackage_HeapMonitorNoCapabilityTest_allSamplingMethodsFail in </span><font color="#000000"><span style="white-space:pre-wrap"><a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.07/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c.patch">http://cr.openjdk.java.net/~rasbold/8171119/webrev.07/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c.patch</a></span></font></div><div><font color="#000000"><span style="white-space:pre-wrap"><br></span></font></div><div><font color="#000000"><span style="white-space:pre-wrap">  - I had not put it as onload_solo and had not put the print out for debugging, so I did do that by following the can_generate_breakpoint_events as you recommended</span></font></div><div><font color="#000000"><span style="white-space:pre-wrap">     - Currently I don't have an associated event because I'm not sure it applies to here but if you want me to consider it and try it out to see the overhead of such a change, I'm happy to try</span></font></div><div><font color="#000000"><span style="white-space:pre-wrap"><br></span></font></div><div><font color="#000000"><span style="white-space:pre-wrap">4) Changing structures:</span></font></div><div><font color="#000000"><span style="white-space:pre-wrap">   - I've moved jvmtiCallFrame to jvmtiFrameInfo</span></font></div><div><font color="#000000"><span style="white-space:pre-wrap">   - For </span></font>jvmtiStackTrace, it seems that for historical reasons we were using env_id but I don't in the current code so I removed it. So the only change between jvmtiStackTrace and jvmtiStackInfo is the size of the object allocated. Before doing that work, any issues with me adding it in your opinion?</div><div>       - If not but there is more work to make it consistent across the board, perhaps that should be a first change and then do this on top.</div><div><br></div><div>Thanks for all your comments!<br></div><div>Jc</div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 28, 2017 at 12:26 AM, <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">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000">
    <div class="m_5531443816811676102moz-cite-prefix">One more comment on the JVMTI spec
      update (jvmti.xml).<br>
      <br>
      In the Heap Monitoring section new typedefs are defined:<br>
      <h4 id="m_5531443816811676102jvmtiCallFrame">Call Frame</h4>
      <blockquote>
        <pre>typedef struct {
    jint bci;
    jmethodID method_id;
} jvmtiCallFrame;</pre>
      </blockquote>
      <pre></pre>
      <h4 id="m_5531443816811676102jvmtiStackTrace">Stack Trace</h4>
      <blockquote>
        <pre>typedef struct {
    JNIEnv* env_id;
    jvmtiCallFrame* frames;
    jint frame_count;
    jint size;
    jlong thread_id;
} jvmtiStackTrace;</pre>
      </blockquote>
      <br>
      I think, it'd make sense to use similar typedefs defined in the
      Stack Frame section:<br>
        jvmtiFrameInfo and jvmtiStackInfo<br>
      <br>
      <br>
      Thanks,<br>
      Serguei<div><div class="h5"><br>
      <br>
      <br>
      On 6/27/17 23:58, <a class="m_5531443816811676102moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a> wrote:<br>
    </div></div></div><div><div class="h5">
    <blockquote type="cite">
      
      <div class="m_5531443816811676102moz-cite-prefix">Hi JC,<br>
        <br>
        Please, find more comments below.<br>
        <br>
        <br>
        On 6/27/17 21:26, <a class="m_5531443816811676102moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a>
        wrote:<br>
      </div>
      <blockquote type="cite">
        <div class="m_5531443816811676102moz-cite-prefix">Hi JC,<br>
          <br>
          Sorry for a latency in reply.<br>
          I'm going to a 4-week vacation.<br>
          But I'll try to help you occasionally when there will be
          access to the Internet.<br>
          <br>
          <br>
          <br>
          On 6/23/17 09:58, JC Beyler wrote:<br>
        </div>
        <blockquote type="cite">
          <div dir="ltr">Hi Serguei and all,
            <div><br>
            </div>
            <div>Thanks Serguei for your review. I believe I have not
              answered many questions but raised more but I hope this
              helps understand what I am trying to do.</div>
            <div><br>
            </div>
            <div>I have inlined my answers to make it easier to answer
              everything (and to be honest, ask questions).</div>
            <div class="gmail_extra"><br>
            </div>
            <div class="gmail_extra">@Thomas: Thank you for your
              thorough answer as well, I'll be working on it and will
              answer when I get the fixes and comments in a handled
              state, most likely beginning of next week :)</div>
            <div class="gmail_extra"><br>
              <div class="gmail_quote">On Fri, Jun 23, 2017 at 3:01 AM,
                <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">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">
                  <div>
                    <div class="m_5531443816811676102gmail-m_4523822924146464101moz-cite-prefix">Hi
                      JC,<br>
                      <br>
                      > Some initial JVMTI-specific
                      questions/comments.<br>
                    </div>
                  </div>
                </blockquote>
              </div>
            </div>
          </div>
        </blockquote>
      </blockquote>
      . . .<br>
      <blockquote type="cite">
        <blockquote type="cite">
          <div dir="ltr">
            <div class="gmail_extra">
              <div class="gmail_quote">
                <blockquote class="gmail_quote">
                  <div>
                    <div class="m_5531443816811676102gmail-m_4523822924146464101moz-cite-prefix">
                    </div>
                  </div>
                </blockquote>
              </div>
            </div>
          </div>
        </blockquote>
        <blockquote type="cite">
          <div dir="ltr">
            <div class="gmail_extra">
              <div class="gmail_quote">
                <div class="m_5531443816811676102gmail-m_4523822924146464101moz-cite-prefix">
                  I think, it would make sense to introduce new optional
                  capability, something like:<br>
                     can_sample_heap_allocations<br>
                </div>
                <div><br>
                </div>
                <div>I will work on adding that for the next webrev,
                  Robbin had mentioned I should and I dropped the ball
                  on that one.</div>
              </div>
            </div>
          </div>
        </blockquote>
        <br>
        I see a couple of more emails from you.<br>
        Will try to help when you have some problems or questions.<br>
        <br>
        <blockquote type="cite">
          <div dir="ltr">
            <div class="gmail_extra">
              <div class="gmail_quote">
                <div class="m_5531443816811676102gmail-m_4523822924146464101moz-cite-prefix">
                  > Do you consider this API to be used by a single
                  agent or it is a multi-agent feature?<br>
                  > What if one agent calls the <span class="m_5531443816811676102gmail-m_4523822924146464101new">StartHeapSampling()
                    but another calls one of the others.</span> <br>
                  > The API specs needs to clarify this.<br>
                  <br>
                  > I'm not sure you had a chance to carefully think
                  on what JVMTI phases are allowed for new functions.<br>
                </div>
                <div><br>
                </div>
                <div>- I am pretty sure I have NOT really thought about
                  this as carefully as you have because this is the
                  first time I am doing this and these questions already
                  make me go "hmmm" :)</div>
              </div>
            </div>
          </div>
        </blockquote>
        <br>
        Expectable. :)<br>
        <br>
        <blockquote type="cite">
          <div dir="ltr">
            <div class="gmail_extra">
              <div class="gmail_quote">
                <div><br>
                </div>
                <div>I have a tendency to work through issues and try to
                  keep the design simple first. Your question seems to
                  imply I can make this work for a single agent world
                  and have a means to explicitly disable it for
                  multi-agent for now.</div>
              </div>
            </div>
          </div>
        </blockquote>
      </blockquote>
      <br>
      Right.<br>
      I agree, this will allow to make a progress and get more
      experience first.<br>
      <br>
      <br>
      <blockquote type="cite">
        <blockquote type="cite">
          <div dir="ltr">
            <div class="gmail_extra">
              <div class="gmail_quote">
                <div><br>
                </div>
                <div>I looked quickly in the prims folder for anything
                  mentioning single agent vs multi agent but did not
                  find anything. Does this get handled s this more a
                  documentation thing and tough luck for the user.</div>
                <div><br>
                </div>
                <div>This page: <a href="https://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#environments" target="_blank">https://docs.oracle.com/<wbr>javase/7/docs/platform/jvmti/<wbr>jvmti.html#environments</a></div>
                <div><br>
                </div>
                <div>seems to say the latter. That the documentation
                  should say that this is not a multiple agent support
                  (yet?) and there is nothing we can do. Is that right?</div>
              </div>
            </div>
          </div>
        </blockquote>
      </blockquote>
      <br>
      <br>
      You can look at these two functions in the
      jvmtiManageCapabilities.cpp:<br>
      <br>
       144 jvmtiCapabilities
      JvmtiManageCapabilities::init_<wbr>always_solo_capabilities() {<br>
       145   jvmtiCapabilities jc;<br>
       146 <br>
       147   memset(&jc, 0, sizeof(jc));<br>
       148   jc.can_suspend = 1;<br>
       149   return jc;<br>
       150 }<br>
       151 <br>
       152 <br>
       153 jvmtiCapabilities
      JvmtiManageCapabilities::init_<wbr>onload_solo_capabilities() {<br>
       154   jvmtiCapabilities jc;<br>
       155 <br>
       156   memset(&jc, 0, sizeof(jc));<br>
       157   jc.can_generate_field_<wbr>modification_events = 1;<br>
       158   jc.can_generate_field_access_<wbr>events = 1;<br>
       159   jc.can_generate_breakpoint_<wbr>events = 1;<br>
       160   return jc;<br>
       161 }<br>
      <br>
      <br>
      The can_suspend is always solo but other 3 capabilities are onload
      solo.<br>
      Probably, onload solo would work well for your purpose.<br>
      So that it is possible to follow one of these pattern, for
      example, can_generate_breakpoint_<wbr>events.<br>
      You can check all uses of it in the JVMTI spec and the hotspot
      sources.<br>
      <br>
      Also, look at the generated file:<br>
       
build/linux-x86_64-normal-<wbr>server-release/hotspot/<wbr>variant-server/gensrc/<wbr>jvmtifiles/jvmtiEnter.cpp<br>
      <br>
        (You may have different kind of build so, just replace the
      suffix "linux-x86_64-normal-server-<wbr>release" with yours.<br>
      <br>
      You will find a couple of conditions like this:<br>
      <br>
      2873   if
      (jvmti_env->get_capabilities()<wbr>->can_generate_breakpoint_<wbr>events
      == 0) {<br>
      2874     return JVMTI_ERROR_MUST_POSSESS_<wbr>CAPABILITY;<br>
      2875   }<br>
      <br>
      In your case such checks will be also auto-generated from your
      version of the jvmti.xml.<br>
      The newly generated jvmti.html can be found in the same build
      folder too.  <br>
      <br>
      You may need to look at this JVMTI spec section:<br>
       
      <a class="m_5531443816811676102moz-txt-link-freetext" href="http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#capability" target="_blank">http://docs.oracle.com/javase/<wbr>8/docs/platform/jvmti/jvmti.<wbr>html#capability</a><br>
      <br>
      on how to add the needed capability.<br>
      <br>
      <br>
      Please, ask questions if you have.<br>
      I hope, somebody will be able to answer, or I'll do it when get a
      chance.<br>
      <br>
      <br>
      Thanks,<br>
      Serguei<br>
      <br>
      <br>
      <blockquote type="cite">
        <blockquote type="cite">
          <div dir="ltr">
            <div class="gmail_extra">
              <div class="gmail_quote">
                <blockquote class="gmail_quote"><br>
                  <div>
                    <div class="m_5531443816811676102gmail-m_4523822924146464101moz-cite-prefix">
                      > <a class="m_5531443816811676102gmail-m_4523822924146464101moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.06/src/share/vm/runtime/heapMonitoring.cpp.html" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/8171119/webrev.06/src/sh<wbr>are/vm/runtime/heapMonitoring.<wbr>cpp.html</a><br>
                      <br>
                      > I do not see the variable
                      HeapMonitoring::_enabled is checked in the
                      functions:<br>
                      >     HeapMonitoring::get_live_trace<wbr>s(),<br>
                      >     HeapMonitoring::get_garbage_tr<wbr>aces(),<br>
                      >     HeapMonitoring::get_frequent_g<wbr>arbage_traces(),<br>
                      >     HeapMonitoring::release_traces<wbr>()<br>
                      <br>
                    </div>
                  </div>
                </blockquote>
                <div>So it is not checked there because the API allows
                  you to:</div>
                <div>  - Start sampling (which flips the enabled
                  variable)</div>
                <div>  - Stop sampling (which flips back the enabled
                  variable)</div>
                <div>  - Get the sampled traces at that point</div>
                <div><br>
                </div>
                <div>I think the API documentation is not clear for this
                  right now, so I'll change it for the next webrev. Let
                  me also know if you think this is a bad idea.</div>
              </div>
            </div>
          </div>
        </blockquote>
        <br>
        I agree, it is better at least to document this behavior.<br>
        <br>
        <br>
        <blockquote type="cite">
          <div dir="ltr">
            <div class="gmail_extra">
              <div class="gmail_quote">
                <div> I thought it would be useful because it would
                  allow you to start/stop the sampling and then later on
                  go back and see what was sampled at a later time.<br>
                </div>
              </div>
            </div>
          </div>
        </blockquote>
        <br>
        I need to think more on this.<br>
        The whole style is different than that used in the JVMTI.<br>
        Normally, each agent does some steps like the following:<br>
        <br>
          - add the capabilities required by the agent (normally in the
        ONLOAD phase)<br>
          - set the agent event callbacks<br>
          - enable/disable event notification mode<br>
          - relinquish the capabilities that are not needed anymore<br>
        <br>
        In your approach there is no tight association with any agent
        yet.<br>
        Also, it seems, the heap monitoring initialization/finalization<br>
        are not separated from sampling starting/stopping.<br>
        The heap allocation events are processed by the VM itself (in GC
        or JVMTI).<br>
        <br>
        It is not clear yet if it could be somehow adjusted to JVMTI
        style<br>
        and if it is really necessary.<br>
        <br>
        <br>
        <blockquote type="cite">
          <div dir="ltr">
            <div class="gmail_extra">
              <div class="gmail_quote">
                <blockquote class="gmail_quote">
                  <div>
                    <div class="m_5531443816811676102gmail-m_4523822924146464101moz-cite-prefix">
                      A question about a multi-agent feature from above
                      applies here as well.<br>
                    </div>
                  </div>
                </blockquote>
                <div><br>
                </div>
                <div>I'm open to either supporting multi-agent if you
                  think it is important or doing whatever can be done
                  to, as a first step, make it a single agent support.
                  Let me know how to do that.</div>
              </div>
            </div>
          </div>
        </blockquote>
        <br>
        I do not think it is really important to support multi-agents
        here.<br>
        However, it is important to make a choice sooner rather than
        later as it impacts the design.<br>
        <br>
        <br>
        Thanks,<br>
        Serguei<br>
        <br>
        <blockquote type="cite">
          <div dir="ltr">
            <div class="gmail_extra">
              <div class="gmail_quote">
                <div class="m_5531443816811676102gmail-m_4523822924146464101moz-cite-prefix"> 
                  > Thanks,<br>
                    > Serguei</div>
                <div><br>
                </div>
                <div>Thank you!</div>
                <div>Jc</div>
                <div> </div>
                <blockquote class="gmail_quote">
                  <div>
                    <div class="m_5531443816811676102gmail-m_4523822924146464101moz-cite-prefix">
                      <div>
                        <div class="m_5531443816811676102gmail-h5"><br>
                          <br>
                          <br>
                          <br>
                          On 6/21/17 13:45, JC Beyler wrote:<br>
                        </div>
                      </div>
                    </div>
                    <div>
                      <div class="m_5531443816811676102gmail-h5">
                        <blockquote type="cite">
                          <div dir="ltr">Hi all,
                            <div><br>
                            </div>
                            <div>First off: Thanks again to Robbin and
                              Thomas for their reviews :)</div>
                            <div><br>
                            </div>
                            <div>Next, I've uploaded a new webrev:</div>
                            <div><a href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.06/" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/8171119/webrev.06/</a></div>
                            <div><br>
                            </div>
                            <div>Here is an update:</div>
                            <div><br>
                            </div>
                            <div>- @Robbin, I forgot to say that yes I
                              need to look at implementing this for the
                              other architectures and testing it before
                              it is all ready to go. Is it common to
                              have it working on all possible
                              combinations or is there a subset that I
                              should be doing first and we can do the
                              others later?</div>
                            <div>- I've tested slowdebug, built and ran
                              the JTreg tests I wrote with slowdebug and
                              fixed a few more issues<br>
                            </div>
                            <div>- I've refactored a bit of the code
                              following Thomas' comments<br>
                            </div>
                            <div>   - I think I've handled all the
                              comments from Thomas (I put comments
                              inline below for the specifics)</div>
                            <div><br>
                            </div>
                            <div>The biggest addition to this webrev is
                              that there is more testing, I've added two
                              tests for looking specifically at the two
                              garbage sampling data Recent vs Frequent:</div>
                            <div>   <a href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorRecentTest.java.patch" target="_blank">http://cr.openjdk.java.net/~r<wbr>asbold/8171119/webrev.06/test/<wbr>serviceability/jvmti/HeapMonit<wbr>or/MyPackage/HeapMonitorRecent<wbr>Test.java.patch</a> </div>
                            <div>   and</div>
                            <div>   <a href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorFrequentTest.java.patch" target="_blank">http://cr.openjdk.java.net/~r<wbr>asbold/8171119/webrev.06/test/<wbr>serviceability/jvmti/HeapMonit<wbr>or/MyPackage/HeapMonitorFreque<wbr>ntTest.java.patch</a></div>
                            <div><br>
                            </div>
                            <div>I've also refactored the JNI library a
                              bit: <a href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c.patch" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/8171119/webrev.06/test/<wbr>serviceability/jvmti/HeapMonit<wbr>or/libHeapMonitor.c.patch</a>.</div>
                            <div>   - Side question: I was looking at
                              trying to make this a multi-file library
                              so you would not have all the code in one
                              spot. It seems this is not really done?</div>
                            <div>      - Is there a solution to this?
                              What I really wanted was:</div>
                            <div>        - The core library that will
                              help testing be easier</div>
                            <div>        - The JNI code for each Java
                              class separate and calling into the core
                              library</div>
                            <div><br>
                            </div>
                            <div>- Following Thomas' comments on
                              statistics, I want to add some quality
                              assurance tests and find that the easiest
                              way would be to have a few counters of
                              what is happening in the sampler and
                              expose that to the user.</div>
                            <div>   - I'll be adding that in the next
                              version if no one sees any objections to
                              that.</div>
                            <div>   - This will allow me to add a sanity
                              test in JTreg about number of samples and
                              average of sampling rate</div>
                            <div><br>
                            </div>
                            <div>@Thomas: I had a few questions that I
                              inlined below but I will summarize the
                              "bigger ones" here:</div>
                            <div>   - You mentioned constants are not
                              using the right conventions, I looked
                              around and didn't see any convention
                              except normal naming then for static
                              constants. Is that right?</div>
                            <div>   - You mentioned putting the
                              weak_oops_do in a separate method and
                              logging, I inlined my answer below and
                              would appreciate your feedback there too.</div>
                            <div><br>
                            </div>
                            <div>Thanks again for your reviews and
                              patience!<br>
                            </div>
                            <div>Jc</div>
                            <div><br>
                            </div>
                            <div>PS: I've also inlined my answers to
                              Thomas below:</div>
                            <div><br>
                            </div>
                            <div>
                              <div class="gmail_extra">
                                <div class="gmail_quote">On Tue, Jun 13,
                                  2017 at 8:03 AM, Thomas Schatzl <span dir="ltr"><<a href="mailto:thomas.schatzl@oracle.com" target="_blank">thomas.schatzl@oracle.com</a>></span>
                                  wrote:<br>
                                  <blockquote class="gmail_quote">Hi
                                    all,<br>
                                    <span class="m_5531443816811676102gmail-m_4523822924146464101gmail-m_-1455798157861946755gmail-m_7132783609556290314gmail-"><br>
                                      On Mon, 2017-06-12 at 11:11 -0700,
                                      JC Beyler wrote:<br>
                                      > Dear all,<br>
                                      ><br>
                                    </span><span class="m_5531443816811676102gmail-m_4523822924146464101gmail-m_-1455798157861946755gmail-m_7132783609556290314gmail-">>
                                      I've continued working on this and
                                      have done the following webrev:<br>
                                      > <a href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.05/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/8171119/webrev.05/</a><br>
                                      <br>
                                    </span>[...]<br>
                                    <span class="m_5531443816811676102gmail-m_4523822924146464101gmail-m_-1455798157861946755gmail-m_7132783609556290314gmail-">>
                                      Things I still need to do:<br>
                                      >    - Have to fix that TLAB
                                      case for the FastTLABRefill<br>
                                      >    - Have to start looking at
                                      the data to see that it is
                                      consistent<br>
                                      > and does gather the right
                                      samples, right frequency, etc.<br>
                                      >    - Have to check the GC
                                      elements and what that produces<br>
                                      >    - Run a slowdebug run and
                                      ensure I fixed all those issues
                                      you saw<br>
                                      > Robbin<br>
                                      ><br>
                                      > Thanks for looking at the
                                      webrev and have a great week!<br>
                                      <br>
                                    </span>  scratching a bit on the
                                    surface of this change, so apologies
                                    for<br>
                                    rather shallow comments:<br>
                                    <br>
                                    - macroAssembler_x86.cpp:5604: while
                                    this is compiler code, and I am<br>
                                    not sure this is final, please avoid
                                    littering the code with TODO<br>
                                    remarks :) They tend to be
                                    candidates for later wtf moments
                                    only.<br>
                                    <br>
                                    Just file a CR for that.<br>
                                    <br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Newcomer question: what is a CR
                                    and not sure I have the rights to do
                                    that yet ? :)</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> -
                                    calling HeapMonitoring::do_wea<wbr>k_oops()
                                    (which should probably be<br>
                                    called weak_oops_do() like other
                                    similar methods) only if string<br>
                                    deduplication is enabled (in
                                    g1CollectedHeap.cpp:4511) seems
                                    wrong.</blockquote>
                                  <blockquote class="gmail_quote"> <br>
                                    The call should be at least around 6
                                    lines up outside the if.<br>
                                    <br>
                                    Preferentially in a method like
                                    process_weak_jni_handles(),
                                    including<br>
                                    additional logging. (No new (G1) gc
                                    phase without minimal logging :)).<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Done but really not sure because:<br>
                                  </div>
                                  <div><br>
                                  </div>
                                  <div>I put for logging:</div>
                                  <div>
                                    <div>  log_develop_trace(gc,
                                      freelist)("G1ConcRegionFreeing
                                      [other] : heap monitoring");</div>
                                  </div>
                                  <div><br>
                                  </div>
                                  <div>Since weak_jni_handles didn't
                                    have logging for me to be inspired
                                    from, I did that but unconvinced
                                    this is what should be done.</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    Seeing the changes in
                                    referenceProcess.cpp, you need to
                                    add the call to<br>
                                    HeapMonitoring::do_weak_oops()
                                    exactly similar to<br>
                                    process_weak_jni_handles() in case
                                    there is no reference processing<br>
                                    done.<br>
                                    <br>
                                    - psParallelCompact.cpp:2172 similar
                                    to other places where the change<br>
                                    adds the call to
                                    HeapMonitoring::do_weak_oops()<wbr>,
                                    remove the empty line.<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Done from what I can tell in the
                                    whole webrev. (The only empty lines
                                    I still see are when I maintain an
                                    empty line, exception being <a href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.06/src/share/vm/gc/shared/collectedHeap.cpp.patch" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/8171119/webrev.06/src/sh<wbr>are/vm/gc/shared/collectedHeap<wbr>.cpp.patch</a>
                                    where I think it was "nicer")</div>
                                  <div> <br>
                                  </div>
                                  <blockquote class="gmail_quote"> <br>
                                    - the change doubles the size of<br>
                                    CollectedHeap::allocate_from_t<wbr>lab_slow()
                                    above the "small and nice"<br>
                                    threshold. Maybe it could be
                                    refactored a bit.<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Done I think, it looks better to
                                    me :).</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    - referenceProcessor.cpp:40: please
                                    make sure that the includes are<br>
                                    sorted alphabetically (I did not
                                    check the other files yet).<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Done and checked all other files
                                    normally.</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    - referenceProcessor.cpp:261: the
                                    change should add logging about the<br>
                                    number of references encountered,
                                    maybe after the corresponding "JNI<br>
                                    weak reference count" log message.<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Just to double check, are you
                                    saying that you'd like to have the
                                    heap sampler to keep in store how
                                    many sampled objects were
                                    encountered in the
                                    HeapMonitoring::weak_oops_do?</div>
                                  <div>   - Would a return of the method
                                    with the number of handled
                                    references and logging that work?</div>
                                  <div>   - Additionally, would you
                                    prefer it in a separate block with
                                    its GCTraceTime?</div>
                                  <div><br>
                                  </div>
                                  <blockquote class="gmail_quote"> <br>
                                    - threadLocalAllocBuffer.cpp:3<wbr>31:
                                    one more "TODO"<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Removed it and added it to my
                                    personal todos to look at.</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    - threadLocalAllocBuffer.hpp:
                                    ThreadLocalAllocBuffer class<br>
                                    documentation should be updated
                                    about the sampling additions. I
                                    would<br>
                                    have no clue what the difference
                                    between "actual_end" and "end" would<br>
                                    be from the given information.<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>If you are talking about the
                                    comments in this file, I made them
                                    more clear I hope in the new webrev.
                                    If it was somewhere else, let me
                                    know where to change.</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    - threadLocalAllocBuffer.hpp:130<wbr>:
                                    extra whitespace ;)<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Fixed :)</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    - some copyrights :)<br>
                                    <br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>I think I fixed all the issues,
                                    if you see specific issues, let me
                                    know.</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> - in
                                    heapMonitoring.hpp: there are some
                                    random comments about some code<br>
                                    that has been grabbed from
                                    "util/math/fastmath.[h|cc]". I can't
                                    tell<br>
                                    whether this is code that can be
                                    used but I assume that Noam Shazeer
                                    is<br>
                                    okay with that (i.e. that's all
                                    Google code).<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Jeremy and I double checked and
                                    we can release that as I thought. I
                                    removed the comment from that piece
                                    of code entirely.</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    - heapMonitoring.hpp/cpp static
                                    constant naming does not correspond
                                    to<br>
                                    Hotspot's. Additionally, in Hotspot
                                    static methods are cased like other<br>
                                    methods.<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>I think I fixed the methods to be
                                    cased the same way as all other
                                    methods. For static constants, I was
                                    not sure. I fixed a few other
                                    variables but I could not seem to
                                    really see a consistent trend for
                                    constants. I made them as variables
                                    but I'm not sure now.</div>
                                  <div><br>
                                  </div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    - in heapMonitoring.cpp there are a
                                    few cryptic comments at the top<br>
                                    that seem to refer to internal stuff
                                    that should probably be removed.<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>Sorry about that! My personal
                                    todos not cleared out.</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    I did not think through the impact
                                    of the TLAB changes on collector<br>
                                    behavior yet (if there are). Also I
                                    did not check for problems with<br>
                                    concurrent mark and SATB/G1 (if
                                    there are).<br>
                                  </blockquote>
                                  <div><br>
                                  </div>
                                  <div>I would love to know your
                                    thoughts on this, I think this is
                                    fine. I see issues with multiple
                                    threads right now hitting the stack
                                    storage instance. Previous webrevs
                                    had a mutex lock here but we took it
                                    out for simplificity (and only for
                                    now).</div>
                                  <div> </div>
                                  <blockquote class="gmail_quote"> <br>
                                    Thanks,<br>
                                      Thomas<br>
                                    <br>
                                  </blockquote>
                                </div>
                                <br>
                              </div>
                            </div>
                          </div>
                        </blockquote>
                        <br>
                      </div>
                    </div>
                  </div>
                </blockquote>
              </div>
              <br>
            </div>
          </div>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </div></div></div>

</blockquote></div><br></div>