<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi JC,<br>
    <br>
    Comments are inlined below.<br>
    <br>
    <div class="moz-cite-prefix">On 2018-02-13 06:18, JC Beyler wrote:<br>
    </div>
    <blockquote
cite="mid:CAF9BGByDrKLNu9A4Rx41TWRrYHjrBmEbRYH_614HwgRBNnsECA@mail.gmail.com"
      type="cite">
      <div dir="ltr">Hi Erik,
        <div><br>
        </div>
        <div>Thanks for your answers, I've now inlined my own
          answers/comments.</div>
        <div><br>
        </div>
        <div>I've done a new webrev here:</div>
        <div><a moz-do-not-send="true"
            href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.08/"
            target="_blank">http://cr.openjdk.java.net/~<wbr>jcbeyler/8171119/webrev.08/</a><br>
        </div>
        <div><br>
        </div>
        <div>The incremental is here:</div>
        <div><a moz-do-not-send="true"
            href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.07_08/"
            target="_blank">http://cr.openjdk.java.net/~<wbr>jcbeyler/8171119/webrev.07_08/</a><br>
        </div>
        <div><br>
        </div>
        <div>Note to all:</div>
        <div>  - I've been integrating changes from Erin/Serguei/David
          comments so this webrev incremental is a bit an answer to all
          comments in one. I apologize for that :)</div>
        <div><br>
        </div>
        <div class="gmail_extra"><br>
          <div class="gmail_quote">On Mon, Feb 12, 2018 at 6:05 AM, Erik
            Österlund <span dir="ltr"><<a moz-do-not-send="true"
                href="mailto:erik.osterlund@oracle.com" target="_blank">erik.osterlund@oracle.com</a>></span>
            wrote:<br>
            <blockquote class="gmail_quote" style="margin:0px 0px 0px
              0.8ex;border-left:1px solid
              rgb(204,204,204);padding-left:1ex">Hi JC,<br>
              <br>
              Sorry for the delayed reply.<br>
              <br>
              Inlined answers:
              <div>
                <div
                  class="m_-3432361388973563659gmail-m_2261932697128216379h5"><br>
                  <br>
                  On 2018-02-06 00:04, JC Beyler wrote:<br>
                  <blockquote class="gmail_quote" style="margin:0px 0px
                    0px 0.8ex;border-left:1px solid
                    rgb(204,204,204);padding-left:1ex">
                    Hi Erik,<br>
                    <br>
                    (Renaming this to be folded into the newly renamed
                    thread :))<br>
                    <br>
                    First off, thanks a lot for reviewing the webrev! I
                    appreciate it!<br>
                    <br>
                    I updated the webrev to:<br>
                    <a moz-do-not-send="true"
                      href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.05a/"
                      rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jc<wbr>beyler/8171119/webrev.05a/</a><br>
                    <br>
                    And the incremental one is here:<br>
                    <a moz-do-not-send="true"
                      href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.04_05a/"
                      rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jc<wbr>beyler/8171119/webrev.04_05a/</a><br>
                    <br>
                    It contains:<br>
                    - The change for since from 9 to 11 for the
                    jvmti.xml<br>
                    - The use of the OrderAccess for initialized<br>
                    - Clearing the oop<br>
                    <br>
                    I also have inlined my answers to your comments. The
                    biggest question<br>
                    will come from the multiple *_end variables. A bit
                    of the logic there<br>
                    is due to handling the slow path refill vs fast path
                    refill and<br>
                    checking that the rug was not pulled underneath the
                    slowpath. I<br>
                    believe that a previous comment was that
                    TlabFastRefill was going to<br>
                    be deprecated.<br>
                    <br>
                    If this is true, we could revert this code a bit and
                    just do a : if<br>
                    TlabFastRefill is enabled, disable this. And then
                    deprecate that when<br>
                    TlabFastRefill is deprecated.<br>
                    <br>
                    This might simplify this webrev and I can work on a
                    follow-up that<br>
                    either: removes TlabFastRefill if Robbin does not
                    have the time to do<br>
                    it or add the support to the assembly side to handle
                    this correctly.<br>
                    What do you think?<br>
                  </blockquote>
                  <br>
                </div>
              </div>
              I support removing TlabFastRefill, but I think it is good
              to not depend on that happening first.<span><br>
                <br>
              </span></blockquote>
            <div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br>
                </span></div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br
class="m_-3432361388973563659gmail-m_2261932697128216379gmail-Apple-interchange-newline">
                  I'm slowly pushing on the FastTLABRefill (<a
                    moz-do-not-send="true"
                    href="https://bugs.openjdk.java.net/browse/JDK-8194084"
                    target="_blank"><a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net">https://bugs.openjdk.java.net</a><wbr>/browse/JDK-8194084</a>),
                  I agree on keeping both separate for now though so
                  that we can think of both differently</span><br>
              </div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br>
              </div>
               </div>
            <blockquote class="gmail_quote" style="margin:0px 0px 0px
              0.8ex;border-left:1px solid
              rgb(204,204,204);padding-left:1ex"><span>
                <blockquote class="gmail_quote" style="margin:0px 0px
                  0px 0.8ex;border-left:1px solid
                  rgb(204,204,204);padding-left:1ex">
                  Now, below, inlined are my answers:<br>
                  <br>
                  On Fri, Feb 2, 2018 at 8:44 AM, Erik Österlund<br>
                  <<a moz-do-not-send="true"
                    href="mailto:erik.osterlund@oracle.com"
                    target="_blank">erik.osterlund@oracle.com</a>>
                  wrote:<br>
                  <blockquote class="gmail_quote" style="margin:0px 0px
                    0px 0.8ex;border-left:1px solid
                    rgb(204,204,204);padding-left:1ex">
                    Hi JC,<br>
                    <br>
                    Hope I am reviewing the right version of your work.
                    Here goes...<br>
                    <br>
                    src/hotspot/share/gc/shared/co<wbr>llectedHeap.inline.hpp:<br>
                    <br>
                      159     AllocTracer::send_allocation_<wbr>outside_tlab(klass,
                    result, size *<br>
                    HeapWordSize, THREAD);<br>
                      160<br>
                      161     THREAD->tlab().handle_sample(<wbr>THREAD,
                    result, size);<br>
                      162     return result;<br>
                      163   }<br>
                    <br>
                    Should not call tlab()->X without checking if
                    (UseTLAB) IMO.<br>
                    <br>
                  </blockquote>
                  Done!<br>
                </blockquote>
                <br>
              </span>
              More about this later.
              <div>
                <div
                  class="m_-3432361388973563659gmail-m_2261932697128216379h5"><br>
                  <br>
                  <blockquote class="gmail_quote" style="margin:0px 0px
                    0px 0.8ex;border-left:1px solid
                    rgb(204,204,204);padding-left:1ex">
                    <br>
                    <blockquote class="gmail_quote" style="margin:0px
                      0px 0px 0.8ex;border-left:1px solid
                      rgb(204,204,204);padding-left:1ex">
                      src/hotspot/share/gc/shared/th<wbr>readLocalAllocBuffer.cpp:<br>
                      <br>
                      So first of all, there seems to quite a few ends.
                      There is an "end", a "hard<br>
                      end", a "slow path end", and an "actual end".
                      Moreover, it seems like the<br>
                      "hard end" is actually further away than the
                      "actual end". So the "hard end"<br>
                      seems like more of a "really definitely actual
                      end" or something. I don't<br>
                      know about you, but I think it looks kind of
                      messy. In particular, I don't<br>
                      feel like the name "actual end" reflects what it
                      represents, especially when<br>
                      there is another end that is behind the "actual
                      end".<br>
                      <br>
                        413 HeapWord* ThreadLocalAllocBuffer::hard_e<wbr>nd()
                      {<br>
                        414   // Did a fast TLAB refill occur?<br>
                        415   if (_slow_path_end != _end) {<br>
                        416     // Fix up the actual end to be now the
                      end of this TLAB.<br>
                        417     _slow_path_end = _end;<br>
                        418     _actual_end = _end;<br>
                        419   }<br>
                        420<br>
                        421   return _actual_end + alignment_reserve();<br>
                        422 }<br>
                      <br>
                      I really do not like making getters unexpectedly
                      have these kind of side<br>
                      effects. It is not expected that when you ask for
                      the "hard end", you<br>
                      implicitly update the "slow path end" and "actual
                      end" to new values.<br>
                      <br>
                    </blockquote>
                    As I said, a lot of this is due to the
                    FastTlabRefill. If I make this<br>
                    not supporting FastTlabRefill, this goes away. The
                    reason the system<br>
                    needs to update itself at the get is that you only
                    know at that get if<br>
                    things have shifted underneath the tlab slow path. I
                    am not sure of<br>
                    really better names (naming is hard!), perhaps we
                    could do these<br>
                    names:<br>
                    <br>
                    - current_tlab_end       // Either the allocated
                    tlab end or a sampling point<br>
                    - last_allocation_address  // The end of the tlab
                    allocation<br>
                    - last_slowpath_allocated_end  // In case a fast
                    refill occurred the<br>
                    end might have changed, this is to remember slow vs
                    fast past refills<br>
                    <br>
                    the hard_end method can be renamed to something
                    like:<br>
                    tlab_end_pointer()        // The end of the lab
                    including a bit of<br>
                    alignment reserved bytes<br>
                  </blockquote>
                  <br>
                </div>
              </div>
              Those names sound better to me. Could you please provide a
              mapping from the old names to the new names so I
              understand which one is which please?<br>
              <br>
              This is my current guess of what you are proposing:<br>
              <br>
              end -> current_tlab_end<br>
              actual_end -> last_allocation_address<br>
              slow_path_end -> last_slowpath_allocated_end<br>
              hard_end -> tlab_end_pointer<br>
              <br>
            </blockquote>
            <div><br>
            </div>
            <div>Yes that is correct, that was what I was proposing.</div>
            <div> </div>
            <blockquote class="gmail_quote" style="margin:0px 0px 0px
              0.8ex;border-left:1px solid
              rgb(204,204,204);padding-left:1ex">
              I would prefer this naming:<br>
              <br>
              end -> slow_path_end // the end for taking a slow path;
              either due to sampling or refilling<br>
              actual_end -> allocation_end // the end for allocations<br>
              slow_path_end -> last_slow_path_end // last address for
              slow_path_end (as opposed to allocation_end)<br>
              hard_end -> reserved_end // the end of the reserved
              space of the TLAB<br>
              <br>
              About setting things in the getter... that still seems
              like a very unpleasant thing to me. It would be better to
              inspect the call hierarchy and explicitly update the ends
              where they need updating, and assert in the getter that
              they are in sync, rather than implicitly setting various
              ends as a surprising side effect in a getter. It looks
              like the call hierarchy is very small. With my new naming
              convention, reserved_end() would presumably return
              _allocation_end + alignment_reserve(), and have an assert
              checking that _allocation_end ==
              _last_slow_path_allocation_end<wbr>, complaining that this
              invariant must hold, and that a caller to this function,
              such as make_parsable(), must first explicitly synchronize
              the ends as required, to honor that invariant.
              <div>
                <div
                  class="m_-3432361388973563659gmail-m_2261932697128216379h5"><br>
                </div>
              </div>
            </blockquote>
            <div><br>
            </div>
            <div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><br
class="m_-3432361388973563659gmail-m_2261932697128216379gmail-Apple-interchange-newline">
                I've renamed the variables to how you preferred it
                except for the _end one. I did:</div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">current_end</div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">last_allocation_address</span><br>
              </div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">tlab_end_ptr</span><br>
              </div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br>
                </span></div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><span
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">The
                  reason is that the architecture dependent code use the
                  thread.hpp API and it already has tlab included into
                  the name so it becomes tlab_current_end (which is
                  better that tlab_current_tlab_end in my opinion).</span></div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><br>
              </div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">I
                also moved the update into a separate method with a TODO
                that says to remove it when FastTLABRefill is deprecated<br>
              </div>
            </div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    This looks a lot better now. Thanks.<br>
    <br>
    Note that the following comment now needs updating accordingly in
    threadLocalAllocBuffer.hpp:<br>
    <br>
    <pre><span class="new">  41 //            Heap sampling is performed via the end/actual_end fields.</span>
<span class="new">  42 //            actual_end contains the real end of the tlab allocation,</span>
<span class="new">  43 //            whereas end can be set to an arbitrary spot in the tlab to</span>
<span class="new">  44 //            trip the return and sample the allocation.</span>
<span class="new">  45 //            slow_path_end is used to track if a fast tlab refill occured</span>
<span class="new">  46 //            between slowpath calls.</span></pre>
    There might be other comments too, I have not looked in detail.<br>
    <br>
    <blockquote
cite="mid:CAF9BGByDrKLNu9A4Rx41TWRrYHjrBmEbRYH_614HwgRBNnsECA@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"><br>
              </div>
            </div>
            <div> </div>
            <blockquote class="gmail_quote" style="margin:0px 0px 0px
              0.8ex;border-left:1px solid
              rgb(204,204,204);padding-left:1ex">
              <div>
                <div
                  class="m_-3432361388973563659gmail-m_2261932697128216379h5">
                  <br>
                  <blockquote class="gmail_quote" style="margin:0px 0px
                    0px 0.8ex;border-left:1px solid
                    rgb(204,204,204);padding-left:1ex">
                    Not sure it's better but before updating the webrev,
                    I wanted to try<br>
                    to get input/consensus :)<br>
                    <br>
                    (Note hard_end was always further off than end).<br>
                    <br>
                    <blockquote class="gmail_quote" style="margin:0px
                      0px 0px 0.8ex;border-left:1px solid
                      rgb(204,204,204);padding-left:1ex">
                      src/hotspot/share/prims/jvmti.<wbr>xml:<br>
                      <br>
                      10357       <capabilityfield
                      id="can_sample_heap" since="9"><br>
                      10358         <description><br>
                      10359           Can sample the heap.<br>
                      10360           If this capability is enabled then
                      the heap sampling methods<br>
                      can be called.<br>
                      10361         </description><br>
                      10362       </capabilityfield><br>
                      <br>
                      Looks like this capability should not be "since 9"
                      if it gets integrated<br>
                      now.<br>
                    </blockquote>
                    Updated now to 11, crossing my fingers :)<br>
                    <br>
                    <br>
                    <blockquote class="gmail_quote" style="margin:0px
                      0px 0px 0.8ex;border-left:1px solid
                      rgb(204,204,204);padding-left:1ex">
                      src/hotspot/share/runtime/heap<wbr>Monitoring.cpp:<br>
                      <br>
                        448       if (is_alive->do_object_b(value)) {<br>
                        449         // Update the oop to point to the
                      new object if it is still<br>
                      alive.<br>
                        450         f->do_oop(&(trace.obj));<br>
                        451<br>
                        452         // Copy the old trace, if it is
                      still live.<br>
                        453         _allocated_traces->at_put(cur<wbr>r_pos++,
                      trace);<br>
                        454<br>
                        455         // Store the live trace in a cache,
                      to be served up on /heapz.<br>
                        456         _traces_on_last_full_gc->appe<wbr>nd(trace);<br>
                        457<br>
                        458         count++;<br>
                        459       } else {<br>
                        460         // If the old trace is no longer
                      live, add it to the list of<br>
                        461         // recently collected garbage.<br>
                        462         store_garbage_trace(trace);<br>
                        463       }<br>
                      <br>
                      In the case where the oop was not live, I would
                      like it to be explicitly<br>
                      cleared.<br>
                    </blockquote>
                    Done I think how you wanted it. Let me know because
                    I'm not familiar<br>
                    with the RootAccess API. I'm unclear if I'm doing
                    this right or not so<br>
                    reviews of these parts are highly appreciated.
                    Robbin had talked of<br>
                    perhaps later pushing this all into a OopStorage,
                    should I do this now<br>
                    do you think? Or can that wait a second webrev later
                    down the road?<br>
                  </blockquote>
                  <br>
                </div>
              </div>
              I think using handles can and should be done later. You
              can use the Access API now.<br>
              I noticed that you are missing an #include
              "oops/access.inline.hpp" in your heapMonitoring.cpp file.<span><br>
                <br>
              </span></blockquote>
            <div><br>
            </div>
            <div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial;background-color:rgb(255,255,255)"><span
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">The
                  missing header is there for me so I don't know, I made
                  sure it is present in the latest webrev. Sorry about
                  that.</span><br
class="m_-3432361388973563659gmail-m_2261932697128216379gmail-Apple-interchange-newline">
                <br>
              </div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial;background-color:rgb(255,255,255)"> <br>
              </div>
            </div>
            <blockquote class="gmail_quote" style="margin:0px 0px 0px
              0.8ex;border-left:1px solid
              rgb(204,204,204);padding-left:1ex"><span>
                <blockquote class="gmail_quote" style="margin:0px 0px
                  0px 0.8ex;border-left:1px solid
                  rgb(204,204,204);padding-left:1ex">
                  + Did I clear it the way you wanted me to or were you
                  thinking of<br>
                  something else?<br>
                </blockquote>
                <br>
              </span>
              That is precisely how I wanted it to be cleared. Thanks.<span><br>
                <br>
                <blockquote class="gmail_quote" style="margin:0px 0px
                  0px 0.8ex;border-left:1px solid
                  rgb(204,204,204);padding-left:1ex">
                  + Final question here, seems like if I were to want to
                  not do the<br>
                  f->do_oop directly on the trace.obj, I'd need to do
                  something like:<br>
                  <br>
                      f->do_oop(&value);<br>
                      ...<br>
                      trace->store_oop(value);<br>
                  <br>
                  to update the oop internally. Is that right/is that
                  one of the<br>
                  advantages of going to the Oopstorage sooner than
                  later?<br>
                </blockquote>
                <br>
              </span>
              I think you really want to do the do_oop on the root
              directly. Is there a particular reason why you would not
              want to do that?<br>
              Otherwise, yes - the benefit with using the handle
              approach is that you do not need to call do_oop explicitly
              in your code.<span><br>
                <br>
              </span></blockquote>
            <div><br>
            </div>
            <div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial;background-color:rgb(255,255,255)">There
                is no reason except that now we have a load_oop and a
                get_oop_addr, I was not sure what you would think of
                that.</div>
              <br>
            </div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    That's fine.<br>
    <br>
    <blockquote
cite="mid:CAF9BGByDrKLNu9A4Rx41TWRrYHjrBmEbRYH_614HwgRBNnsECA@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div> </div>
            <blockquote class="gmail_quote" style="margin:0px 0px 0px
              0.8ex;border-left:1px solid
              rgb(204,204,204);padding-left:1ex"><span>
                <blockquote class="gmail_quote" style="margin:0px 0px
                  0px 0.8ex;border-left:1px solid
                  rgb(204,204,204);padding-left:1ex">
                  <br>
                  <blockquote class="gmail_quote" style="margin:0px 0px
                    0px 0.8ex;border-left:1px solid
                    rgb(204,204,204);padding-left:1ex">
                    Also I see a lot of concurrent-looking use of the
                    following field:<br>
                      267   volatile bool _initialized;<br>
                    <br>
                    Please note that the "volatile" qualifier does not
                    help with reordering<br>
                    here. Reordering between volatile and non-volatile
                    fields is completely free<br>
                    for both compiler and hardware, except for windows
                    with MSVC, where volatile<br>
                    semantics is defined to use acquire/release
                    semantics, and the hardware is<br>
                    TSO. But for the general case, I would expect this
                    field to be stored with<br>
                    OrderAccess::release_store and loaded with
                    OrderAccess::load_acquire.<br>
                    Otherwise it is not thread safe.<br>
                  </blockquote>
                  Because everything is behind a mutex, I wasn't really
                  worried about<br>
                  this. I have a test that has multiple threads trying
                  to hit this<br>
                  corner case and it passes.<br>
                  <br>
                  However, to be paranoid, I updated it to using the
                  OrderAccess API<br>
                  now, thanks! Let me know what you think there too!<br>
                </blockquote>
                <br>
              </span>
              If it is indeed always supposed to be read and written
              under a mutex, then I would strongly prefer to have it
              accessed as a normal non-volatile member, and have an
              assertion that given lock is held or we are in a
              safepoint, as we do in many other places. Something like
              this:<br>
              <br>
              assert(HeapMonitorStorage_lock<wbr>->owned_by_self() ||
              (SafepointSynchronize::is_at_s<wbr>afepoint() &&
              Thread::current()->is_VM_threa<wbr>d()), "this should
              not be accessed concurrently");<br>
              <br>
              It would be confusing to people reading the code if there
              are uses of OrderAccess that are actually always protected
              under a mutex.<span><br>
                <br>
              </span></blockquote>
            <div><br>
            </div>
            <div>Thank you for the exact example to be put in the code!
              I put it around each access/assignment of the _initialized
              method and found one case where yes you can touch it and
              not have the lock. It actually is "ok" because you don't
              act on the storage until later and only when you really
              want to modify the storage (see the object_alloc_do_sample
              method which calls the add_trace method).</div>
            <div><br>
            </div>
            <div>But, because of this, I'm going to put the OrderAccess
              here, I'll do some performance numbers later and if there
              are issues, I might add a "unsafe" read and a "safe" one
              to make it explicit to the reader. But I don't think it
              will come to that.</div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    Okay. This double return in heapMonitoring.cpp looks wrong:<br>
    <br>
     283   bool initialized() {<br>
     284     return OrderAccess::load_acquire(&_initialized) != 0;<br>
     285     return _initialized;<br>
     286   }<br>
    <br>
    Since you said object_alloc_do_sample() is the only place where you
    do not hold the mutex while reading initialized(), I had a closer
    look at that. It looks like in its current shape, the lack of a
    mutex may lead to a memory leak. In particular, it first checks if
    (initialized()). Let's assume this is now true. It then allocates a
    bunch of stuff, and checks if the number of frames were over 0. If
    they were, it calls StackTraceStorage::storage()->add_trace()
    seemingly hoping that after grabbing the lock in there,
    initialized() will still return true. But it could now return false
    and skip doing anything, in which case the allocated stuff will
    never be freed.<br>
    <br>
    So the analysis seems to be that _initialized is only used outside
    of the mutex in once instance, where it is used to perform
    double-checked locking, that actually causes a memory leak.<br>
    <br>
    I am not proposing how to fix that, just raising the issue. If you
    still want to perform this double-checked locking somehow, then the
    use of acquire/release still seems odd. Because the memory ordering
    restrictions of it never comes into play in this particular case. If
    it ever did, then the use of destroy_stuff();
    release_store(_initialized, 0) would be broken anyway as that would
    imply that whatever concurrent reader there ever was would after
    reading _initialized with load_acquire() could *never* read the data
    that is concurrently destroyed anyway. I would be biased to think
    that RawAccess<MO_RELAXED>::load/store looks like a more
    appropriate solution, given that the memory leak issue is resolved.
    I do not know how painful it would be to not perform this
    double-checked locking.<br>
    <br>
    <blockquote
cite="mid:CAF9BGByDrKLNu9A4Rx41TWRrYHjrBmEbRYH_614HwgRBNnsECA@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div>
              <div
style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial;background-color:rgb(255,255,255)"><br>
              </div>
            </div>
            <div> </div>
            <blockquote class="gmail_quote" style="margin:0px 0px 0px
              0.8ex;border-left:1px solid
              rgb(204,204,204);padding-left:1ex"><span>
                <blockquote class="gmail_quote" style="margin:0px 0px
                  0px 0.8ex;border-left:1px solid
                  rgb(204,204,204);padding-left:1ex">
                  <blockquote class="gmail_quote" style="margin:0px 0px
                    0px 0.8ex;border-left:1px solid
                    rgb(204,204,204);padding-left:1ex">
                    As a kind of meta comment, I wonder if it would make
                    sense to add sampling<br>
                    for non-TLAB allocations. Seems like if someone is
                    rapidly allocating a<br>
                    whole bunch of 1 MB objects that never fit in a
                    TLAB, I might still be<br>
                    interested in seeing that in my traces, and not get
                    surprised that the<br>
                    allocation rate is very high yet not showing up in
                    any profiles.<br>
                    <br>
                  </blockquote>
                  That is handled by the handle_sample where you wanted
                  me to put a<br>
                  UseTlab because you hit that case if the allocation is
                  too big.<br>
                </blockquote>
                <br>
              </span>
              I see. It was not obvious to me that non-TLAB sampling is
              done in the TLAB class. That seems like an abstraction
              crime.<br>
              What I wanted in my previous comment was that we do not
              call into the TLAB when we are not using TLABs. If there
              is sampling logic in the TLAB that is used for something
              else than TLABs, then it seems like that logic simply does
              not belong inside of the TLAB. It should be moved out of
              the TLAB, and instead have the TLAB call this common
              abstraction that makes sense.<br>
              <br>
            </blockquote>
            <div><br>
            </div>
            <div>So in the incremental version:</div>
            <div><a moz-do-not-send="true"
                href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.07_08/"
                target="_blank">http://cr.openjdk.java.net/~<wbr>jcbeyler/8171119/webrev.07_08/</a><wbr>,
              this is still a "crime". The reason is that the system has
              to have the bytes_until_sample on a per-thread level and
              it made "sense" to have it with the TLAB implementation.
              Also, I was not sure how people felt about adding
              something to the thread instance instead.<br>
            </div>
            <div><br>
            </div>
            <div>Do you think it fits better at the Thread level? I can
              see how difficult it is to make it happen there and add
              some logic there. Let me know what you think.</div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    We have an unfortunate situation where everyone that has some fields
    that are thread local tend to dump them right into Thread, making
    the size and complexity of Thread grow as it becomes tightly coupled
    with various unrelated subsystems. It would be desirable to have a
    separate class for this instead that encapsulates the sampling
    logic. That class could possibly reside in Thread though as a value
    object of Thread.<br>
    <br>
    <blockquote
cite="mid:CAF9BGByDrKLNu9A4Rx41TWRrYHjrBmEbRYH_614HwgRBNnsECA@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div><br>
            </div>
            <div> </div>
            <blockquote class="gmail_quote" style="margin:0px 0px 0px
              0.8ex;border-left:1px solid
              rgb(204,204,204);padding-left:1ex">
              Hope I have answered your questions and that my feedback
              makes sense to you.<br>
              <br>
            </blockquote>
            <div><br>
            </div>
            <div>You have and thank you for them, I think we are getting
              to a cleaner implementation and things are getting better
              and more readable :)</div>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    Yes it is getting better.<br>
    <br>
    Thanks,<br>
    /Erik<br>
    <br>
    <blockquote
cite="mid:CAF9BGByDrKLNu9A4Rx41TWRrYHjrBmEbRYH_614HwgRBNnsECA@mail.gmail.com"
      type="cite">
      <div dir="ltr">
        <div class="gmail_extra">
          <div class="gmail_quote">
            <div>Thanks for your help!</div>
            <div>Jc</div>
            <div><br>
            </div>
            <div> </div>
            <blockquote class="gmail_quote" style="margin:0px 0px 0px
              0.8ex;border-left:1px solid
              rgb(204,204,204);padding-left:1ex">
              Thanks,<br>
              /Erik
              <div
                class="m_-3432361388973563659gmail-m_2261932697128216379HOEnZb">
                <div
                  class="m_-3432361388973563659gmail-m_2261932697128216379h5"><br>
                  <br>
                  <blockquote class="gmail_quote" style="margin:0px 0px
                    0px 0.8ex;border-left:1px solid
                    rgb(204,204,204);padding-left:1ex">
                    I double checked by changing the test<br>
                    <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.05a/raw_files/new/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatObjectCorrectnessTest.java"
                      rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jc<wbr>beyler/8171119/webrev.05a/raw_<wbr>files/new/test/hotspot/jtreg/s<wbr>erviceability/jvmti/HeapMonito<wbr>r/MyPackage/HeapMonitorStatObj<wbr>ectCorrectnessTest.java</a><br>
                    <br>
                    to use a smaller Tlab (2048) and made the object
                    bigger and it goes<br>
                    through that and passes.<br>
                    <br>
                    Thanks again for your review and I look forward to
                    your pointers for<br>
                    the questions I now have raised!<br>
                    Jc<br>
                    <br>
                    <br>
                    <br>
                    <br>
                    <br>
                    <br>
                    <br>
                    <br>
                    <br>
                    <blockquote class="gmail_quote" style="margin:0px
                      0px 0px 0.8ex;border-left:1px solid
                      rgb(204,204,204);padding-left:1ex">
                      Thanks,<br>
                      /Erik<br>
                      <br>
                      <br>
                      On 2018-01-26 06:45, JC Beyler wrote:<br>
                      <blockquote class="gmail_quote" style="margin:0px
                        0px 0px 0.8ex;border-left:1px solid
                        rgb(204,204,204);padding-left:1ex">
                        Thanks Robbin for the reviews :)<br>
                        <br>
                        The new full webrev is here:<br>
                        <a moz-do-not-send="true"
                          href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.03/"
                          rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jc<wbr>beyler/8171119/webrev.03/</a><br>
                        The incremental webrev is here:<br>
                        <a moz-do-not-send="true"
                          href="http://cr.openjdk.java.net/%7Ejcbeyler/8171119/webrev.02_03/"
                          rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jc<wbr>beyler/8171119/webrev.02_03/</a><br>
                        <br>
                        I inlined my answers:<br>
                        <br>
                        On Thu, Jan 25, 2018 at 1:15 AM, Robbin Ehn <<a
                          moz-do-not-send="true"
                          href="mailto:robbin.ehn@oracle.com"
                          target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:robbin.ehn@oracle.com">robbin.ehn@oracle.com</a></a>>
                        wrote:<br>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">
                          Hi JC, great to see another revision!<br>
                          <br>
                          ####<br>
                          heapMonitoring.cpp<br>
                          <br>
                          StackTraceData should not contain the oop for
                          'safety' reasons.<br>
                          When StackTraceData is moved from
                          _allocated_traces:<br>
                          L452 store_garbage_trace(trace);<br>
                          it contains a dead oop.<br>
                          _allocated_traces could instead be a tupel of
                          oop and StackTraceData thus<br>
                          dead oops are not kept.<br>
                        </blockquote>
                        Done I used inheritance to make the copier work
                        regardless but the<br>
                        idea is the same.<br>
                        <br>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">
                          You should use the new Access API for loading
                          the oop, something like<br>
                          this:<br>
                          RootAccess<ON_PHANTOM_OOP_REF |
                          AS_NO_KEEPALIVE>::load(...)<br>
                          I don't think you need to use Access API for
                          clearing the oop, but it<br>
                          would<br>
                          look nicer. And you shouldn't probably be
                          using:<br>
                          Universe::heap()->is_in_reserv<wbr>ed(value)<br>
                        </blockquote>
                        I am unfamiliar with this but I think I did do
                        it like you wanted me<br>
                        to (all tests pass so that's a start). I'm not
                        sure how to clear the<br>
                        oop exactly, is there somewhere that does that,
                        which I can use to do<br>
                        the same?<br>
                        <br>
                        I removed the is_in_reserved, this came from our
                        internal version, I<br>
                        don't know why it was there but my tests work
                        without so I removed it<br>
                        :)<br>
                        <br>
                        <br>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">
                          The lock:<br>
                          L424   MutexLocker
                          mu(HeapMonitorStorage_lock);<br>
                          Is not needed as far as I can see.<br>
                          weak_oops_do is called in a safepoint, no TLAB
                          allocation can happen and<br>
                          JVMTI thread can't access these
                          data-structures. Is there something more<br>
                          to<br>
                          this lock that I'm missing?<br>
                        </blockquote>
                        Since a thread can call the JVMTI getLiveTraces
                        (or any of the other<br>
                        ones), it can get to the point of trying to
                        copying the<br>
                        _allocated_traces. I imagine it is possible that
                        this is happening<br>
                        during a GC or that it can be started and a GC
                        happens afterwards.<br>
                        Therefore, it seems to me that you want this
                        protected, no?<br>
                        <br>
                        <br>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">
                          ####<br>
                          You have 6 files without any changes in them
                          (any more):<br>
                          g1CollectedHeap.cpp<br>
                          psMarkSweep.cpp<br>
                          psParallelCompact.cpp<br>
                          genCollectedHeap.cpp<br>
                          referenceProcessor.cpp<br>
                          thread.hpp<br>
                          <br>
                        </blockquote>
                        Done.<br>
                        <br>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">
                          ####<br>
                          I have not looked closely, but is it possible
                          to hide heap sampling in<br>
                          AllocTracer ? (with some minor changes to the
                          AllocTracer API)<br>
                          <br>
                        </blockquote>
                        I am imagining that you are saying to move the
                        code that does the<br>
                        sampling code (change the tlab end, do the call
                        to HeapMonitoring,<br>
                        etc.) into the AllocTracer code itself? I think
                        that is right and I'll<br>
                        look if that is possible and prepare a webrev to
                        show what would be<br>
                        needed to make that happen.<br>
                        <br>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">
                          ####<br>
                          Minor nit, when declaring pointer there is a
                          little mix of having the<br>
                          pointer adjacent by type name and data name.
                          (Most hotspot code is by<br>
                          type<br>
                          name)<br>
                          E.g.<br>
                          heapMonitoring.cpp:711     jvmtiStackTrace
                          *trace = ....<br>
                          heapMonitoring.cpp:733         Method* m =
                          vfst.method();<br>
                          (not just this file)<br>
                          <br>
                        </blockquote>
                        Done!<br>
                        <br>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">
                          ####<br>
                          HeapMonitorThreadOnOffTest.jav<wbr>a:77<br>
                          I would make g_tmp volatile, otherwise the
                          assignment in loop may<br>
                          theoretical be skipped.<br>
                          <br>
                        </blockquote>
                        Also done!<br>
                        <br>
                        Thanks again!<br>
                        Jc<br>
                      </blockquote>
                      <br>
                    </blockquote>
                  </blockquote>
                  <br>
                </div>
              </div>
            </blockquote>
          </div>
          <br>
        </div>
      </div>
    </blockquote>
    <br>
  </body>
</html>