<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-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
cite="mid:CAF9BGBzZCjZedDKfFwX9DmqsQ04_hxsHwZ6UpB1i_r1F-=rT-A@mail.gmail.com"
      type="cite">
      <meta http-equiv="Context-Type" content="text/html; charset=UTF-8">
      <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
              moz-do-not-send="true"
              href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
            <span dir="ltr"><<a moz-do-not-send="true"
                href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a>></span>
            wrote:<br>
            <blockquote class="gmail_quote">
              <div>
                <div class="gmail-m_4523822924146464101moz-cite-prefix">Hi
                  JC,<br>
                  <br>
                  > Some initial JVMTI-specific questions/comments.<br>
                  <br>
                  > I was not able to apply your patch as the are
                  many merge conflicts.<br>
                  > Could you, please, re-base it on the latest JDK
                  10 sources?<br>
                </div>
              </div>
            </blockquote>
            <div><br>
            </div>
            <div>This is weird, I did a pull right now and it worked. I
              hesitated I was using hg correctly so I just did:</div>
            <div>hg clone <a moz-do-not-send="true"
                href="http://hg.openjdk.java.net/jdk10/hs">http://hg.openjdk.java.net/jdk10/hs</a>
              jdk10hs<br>
            </div>
            <div>bash ./get_source.sh<br>
            </div>
            <div>wget <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.06/hotspot.patch">http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/hotspot.patch</a><br>
            </div>
            <div>patch -p1 < hotspot.patch<br>
            </div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    Thanks.<br>
    The following seems to work:<br>
     % cd hotspot; patch -p1 < ../hotspot.patch<br>
    <br>
    Hopefully, I'll be able to build and play with it in order to
    understand better.<br>
    <br>
    <blockquote
cite="mid:CAF9BGBzZCjZedDKfFwX9DmqsQ04_hxsHwZ6UpB1i_r1F-=rT-A@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div>(Maybe the patch is not the right way of doing things,
              there might be a hg equivalent?).</div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    Yes.<br>
    I normally do this:<br>
    <br>
    cd <repo>/hotspot<br>
    hg import ../hotspot.patch<br>
    <br>
    It returns the following errors:<br>
    <br>
    applying ../hotspot.patch<br>
    patching file src/cpu/x86/vm/macroAssembler_x86.cpp<br>
    Hunk #1 succeeded at 5662 with fuzz 2 (offset 60 lines).<br>
    patching file src/share/vm/gc/g1/g1CollectedHeap.cpp<br>
    Hunk #1 FAILED at 74<br>
    Hunk #2 succeeded at 4316 with fuzz 1 (offset 33 lines).<br>
    Hunk #3 FAILED at 4506<br>
    2 out of 3 hunks FAILED -- saving rejects to file
    src/share/vm/gc/g1/g1CollectedHeap.cpp.rej<br>
    patching file src/share/vm/gc/g1/g1CollectedHeap.hpp<br>
    Hunk #1 FAILED at 302<br>
    1 out of 1 hunks FAILED -- saving rejects to file
    src/share/vm/gc/g1/g1CollectedHeap.hpp.rej<br>
    patching file src/share/vm/gc/g1/g1MarkSweep.cpp<br>
    Hunk #1 FAILED at 47<br>
    Hunk #2 FAILED at 249<br>
    2 out of 2 hunks FAILED -- saving rejects to file
    src/share/vm/gc/g1/g1MarkSweep.cpp.rej<br>
    patching file src/share/vm/gc/parallel/psMarkSweep.cpp<br>
    Hunk #1 FAILED at 50<br>
    Hunk #2 FAILED at 609<br>
    2 out of 2 hunks FAILED -- saving rejects to file
    src/share/vm/gc/parallel/psMarkSweep.cpp.rej<br>
    patching file src/share/vm/gc/parallel/psParallelCompact.cpp<br>
    Hunk #1 FAILED at 59<br>
    Hunk #2 FAILED at 2167<br>
    2 out of 2 hunks FAILED -- saving rejects to file
    src/share/vm/gc/parallel/psParallelCompact.cpp.rej<br>
    patching file src/share/vm/gc/shared/collectedHeap.cpp<br>
    Hunk #1 FAILED at 37<br>
    Hunk #2 FAILED at 294<br>
    Hunk #3 FAILED at 314<br>
    Hunk #4 FAILED at 335<br>
    4 out of 4 hunks FAILED -- saving rejects to file
    src/share/vm/gc/shared/collectedHeap.cpp.rej<br>
    patching file src/share/vm/gc/shared/collectedHeap.hpp<br>
    Hunk #1 succeeded at 146 with fuzz 2 (offset 3 lines).<br>
    patching file src/share/vm/gc/shared/collectedHeap.inline.hpp<br>
    Hunk #1 FAILED at 156<br>
    1 out of 1 hunks FAILED -- saving rejects to file
    src/share/vm/gc/shared/collectedHeap.inline.hpp.rej<br>
    patching file src/share/vm/gc/shared/genCollectedHeap.cpp<br>
    Hunk #1 FAILED at 48<br>
    Hunk #2 FAILED at 721<br>
    2 out of 2 hunks FAILED -- saving rejects to file
    src/share/vm/gc/shared/genCollectedHeap.cpp.rej<br>
    patching file src/share/vm/gc/shared/referenceProcessor.cpp<br>
    Hunk #1 FAILED at 34<br>
    Hunk #2 FAILED at 256<br>
    2 out of 2 hunks FAILED -- saving rejects to file
    src/share/vm/gc/shared/referenceProcessor.cpp.rej<br>
    patching file src/share/vm/gc/shared/threadLocalAllocBuffer.cpp<br>
    Hunk #1 FAILED at 28<br>
    Hunk #2 FAILED at 120<br>
    Hunk #3 FAILED at 182<br>
    Hunk #4 FAILED at 305<br>
    4 out of 4 hunks FAILED -- saving rejects to file
    src/share/vm/gc/shared/threadLocalAllocBuffer.cpp.rej<br>
    patching file src/share/vm/gc/shared/threadLocalAllocBuffer.hpp<br>
    Hunk #1 FAILED at 43<br>
    Hunk #2 FAILED at 51<br>
    Hunk #3 FAILED at 65<br>
    Hunk #4 FAILED at 114<br>
    Hunk #5 FAILED at 158<br>
    5 out of 5 hunks FAILED -- saving rejects to file
    src/share/vm/gc/shared/threadLocalAllocBuffer.hpp.rej<br>
    patching file src/share/vm/prims/jvmti.xml<br>
    Hunk #1 succeeded at 11719 with fuzz 1 (offset 190 lines).<br>
    patching file src/share/vm/prims/jvmtiEnv.cpp<br>
    Hunk #1 FAILED at 45<br>
    Hunk #2 FAILED at 54<br>
    Hunk #3 succeeded at 1932 with fuzz 2 (offset -17 lines).<br>
    2 out of 3 hunks FAILED -- saving rejects to file
    src/share/vm/prims/jvmtiEnv.cpp.rej<br>
    patching file src/share/vm/runtime/thread.hpp<br>
    Hunk #1 FAILED at 614<br>
    1 out of 1 hunks FAILED -- saving rejects to file
    src/share/vm/runtime/thread.hpp.rej<br>
    file src/share/vm/prims/jvmtiHeapTransition.hpp already exists<br>
    1 out of 1 hunks FAILED -- saving rejects to file
    src/share/vm/prims/jvmtiHeapTransition.hpp.rej<br>
    file src/share/vm/runtime/heapMonitoring.cpp already exists<br>
    1 out of 1 hunks FAILED -- saving rejects to file
    src/share/vm/runtime/heapMonitoring.cpp.rej<br>
    file src/share/vm/runtime/heapMonitoring.hpp already exists<br>
    1 out of 1 hunks FAILED -- saving rejects to file
    src/share/vm/runtime/heapMonitoring.hpp.rej<br>
    file test/serviceability/jvmti/HeapMonitor/MyPackage/Frame.java
    already exists<br>
    1 out of 1 hunks FAILED -- saving rejects to file
    test/serviceability/jvmti/HeapMonitor/MyPackage/Frame.java.rej<br>
    file
test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorFrequentTest.java
    already exists<br>
    1 out of 1 hunks FAILED -- saving rejects to file
test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorFrequentTest.java.rej<br>
    file
test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorOnOffTest.java
    already exists<br>
    1 out of 1 hunks FAILED -- saving rejects to file
test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorOnOffTest.java.rej<br>
    file
test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorRecentTest.java
    already exists<br>
    1 out of 1 hunks FAILED -- saving rejects to file
test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorRecentTest.java.rej<br>
    file
    test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorTest.java
    already exists<br>
    1 out of 1 hunks FAILED -- saving rejects to file
    test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorTest.java.rej<br>
    file test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c already
    exists<br>
    1 out of 1 hunks FAILED -- saving rejects to file
    test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c.rej<br>
    abort: patch failed to apply<br>
    <br>
    <br>
    <blockquote
cite="mid:CAF9BGBzZCjZedDKfFwX9DmqsQ04_hxsHwZ6UpB1i_r1F-=rT-A@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div><br>
            </div>
            <div>And it seemed to work:</div>
            <div>bash ./configure
              --with-boot-jdk=/path/to/jdk1.8.0-latest
              --with-debug-level=slowdebug<br>
            </div>
            <div>make all images JOBS=48</div>
            <div><br>
            </div>
            <div>Any idea why it is working for me and not you? I am
              assuming I am doing something wrong but I don't know what!</div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    It seems, we've found the reason (please, see above).<br>
    <br>
    <br>
    <blockquote
cite="mid:CAF9BGBzZCjZedDKfFwX9DmqsQ04_hxsHwZ6UpB1i_r1F-=rT-A@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div class="gmail-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
cite="mid:CAF9BGBzZCjZedDKfFwX9DmqsQ04_hxsHwZ6UpB1i_r1F-=rT-A@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div class="gmail-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="gmail-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
cite="mid:CAF9BGBzZCjZedDKfFwX9DmqsQ04_hxsHwZ6UpB1i_r1F-=rT-A@mail.gmail.com"
      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><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 moz-do-not-send="true"
href="https://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#environments">https://docs.oracle.com/javase/7/docs/platform/jvmti/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><br>
            </div>
            <div> </div>
            <blockquote class="gmail_quote">
              <div>
                <div class="gmail-m_4523822924146464101moz-cite-prefix">
                  <br>
                  > <a moz-do-not-send="true"
                    class="gmail-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/~<wbr>rasbold/8171119/webrev.06/src/<wbr>share/vm/runtime/<wbr>heapMonitoring.cpp.html</a><br>
                  <br>
                  > I do not see the variable
                  HeapMonitoring::_enabled is checked in the functions:<br>
                  >     HeapMonitoring::get_live_<wbr>traces(),<br>
                  >     HeapMonitoring::get_garbage_<wbr>traces(),<br>
                  >     HeapMonitoring::get_frequent_<wbr>garbage_traces(),<br>
                  >     HeapMonitoring::release_<wbr>traces()<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
cite="mid:CAF9BGBzZCjZedDKfFwX9DmqsQ04_hxsHwZ6UpB1i_r1F-=rT-A@mail.gmail.com"
      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
cite="mid:CAF9BGBzZCjZedDKfFwX9DmqsQ04_hxsHwZ6UpB1i_r1F-=rT-A@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <blockquote class="gmail_quote">
              <div>
                <div class="gmail-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
cite="mid:CAF9BGBzZCjZedDKfFwX9DmqsQ04_hxsHwZ6UpB1i_r1F-=rT-A@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div class="gmail-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="gmail-m_4523822924146464101moz-cite-prefix">
                  <div>
                    <div class="gmail-h5"><br>
                      <br>
                      <br>
                      <br>
                      On 6/21/17 13:45, JC Beyler wrote:<br>
                    </div>
                  </div>
                </div>
                <div>
                  <div class="gmail-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 moz-do-not-send="true"
                            href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.06/"
                            target="_blank">http://cr.openjdk.java.net/~<wbr>rasbold/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 moz-do-not-send="true"
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/~<wbr>rasbold/8171119/webrev.06/<wbr>test/serviceability/jvmti/<wbr>HeapMonitor/MyPackage/<wbr>HeapMonitorRecentTest.java.<wbr>patch</a> </div>
                        <div>   and</div>
                        <div>   <a moz-do-not-send="true"
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/~<wbr>rasbold/8171119/webrev.06/<wbr>test/serviceability/jvmti/<wbr>HeapMonitor/MyPackage/<wbr>HeapMonitorFrequentTest.java.<wbr>patch</a></div>
                        <div><br>
                        </div>
                        <div>I've also refactored the JNI library a bit:
                          <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Erasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c.patch"
                            target="_blank">http://cr.openjdk.java.net/~<wbr>rasbold/8171119/webrev.06/<wbr>test/serviceability/jvmti/<wbr>HeapMonitor/libHeapMonitor.c.<wbr>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 moz-do-not-send="true"
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="gmail-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="gmail-m_4523822924146464101gmail-m_-1455798157861946755gmail-m_7132783609556290314gmail-">>
                                  I've continued working on this and
                                  have done the following webrev:<br>
                                  > <a moz-do-not-send="true"
                                    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="gmail-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
                                  moz-do-not-send="true"
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/~<wbr>rasbold/8171119/webrev.06/src/<wbr>share/vm/gc/shared/<wbr>collectedHeap.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>
  </body>
</html>