<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#ffffff" text="#000000">
    <br>
    Hi again,<br>
    <br>
    Here is an updated webrev:<br>
    <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/7178361/webrev.02/">http://cr.openjdk.java.net/~brutisso/7178361/webrev.02/</a><br>
    <br>
    It includes the assert to check that we are set up active_workers to
    be less than ParallelGCThreads that Vitaly suggested. <br>
    <br>
    Also, I will have to change my earlier statement about using signed
    values to index C++ arrays. It is fine with unsigned values too
    (thanks Mikael Vidstedt for making me reconsider this statement).
    So, Vitaly, I went ahead and implemented your first suggestion to
    use uint for the worker indexes.<br>
    <br>
    Thanks again for the review Vitaly. I know John Cuthbertson is
    looking at this too. <br>
    <br>
    Benbgt<br>
    <br>
    <br>
    On 2012-06-25 16:23, Vitaly Davidovich wrote:
    <blockquote
cite="mid:CAHjP37FS-Ht8YVnnq5Fz07ZirHOnDi7MrS8pJaMvLdGAvmD4Tg@mail.gmail.com"
      type="cite">
      <p>Thanks for the explanation Bengt - all good from me, FWIW :).</p>
      <p>Sent from my phone</p>
      <div class="gmail_quote">On Jun 25, 2012 9:24 AM, "Bengt Rutisson"
        <<a moz-do-not-send="true"
          href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>>
        wrote:<br type="attribution">
        <blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
          0.8ex; border-left: 1px solid rgb(204, 204, 204);
          padding-left: 1ex;">
          <div bgcolor="#FFFFFF" text="#000000">
            <div><br>
              Hi Vitaly,<br>
              <br>
              On 2012-06-21 14:36, Vitaly Davidovich wrote:<br>
            </div>
            <blockquote type="cite">Hi Bengt,
              <div><br>
              </div>
              <div>Looks good.  One question: the asserts you added
                check against _active_gc_threads, but the arrays are
                sized with parallel_gc_threads.  The assumption then is
                that active_gc_threads <= parallel_gc_threads? If so,
                maybe assert that piece as well. </div>
            </blockquote>
            <br>
            I see your point. Currently _active_gc_threads is set up to
            be the same as parallel_gc_threads, but I can add an assert
            to that effect in the constructor of G1GCPhaseTimes.<br>
            <br>
            <blockquote type="cite">
              <div>Also, maybe consider moving the range asserts into a
                macro or helper function so that you don't have to
                repeat the exact same 2 lines?</div>
            </blockquote>
            <br>
            Somehow I find the inlined asserts more readable. I'll think
            about it. Thanks for the suggestion.<br>
            <br>
            <blockquote type="cite">
              <div><br>
              </div>
              <div>Finally (forgot to mention this in my initial email),
                a minor point -- should the sentinel -1234.0 value that
                you set the arrays to be defined as a constant so if
                you, for some reason, decide to change it, you just
                update 1 place? Very minor though :).</div>
            </blockquote>
            <br>
            Yes, this should be fixed. However, I just moved this code
            from one place to anther and I plan on revisiting this code
            and cleaning it up a bit with my next change. That change
            should remove the serial special case in this code. Is it ok
            if I leave this cleanup for that change?<br>
            <br>
            <blockquote type="cite">
              <div><br>
              </div>
              <div>As for worker_i being unsigned, I was thinking the
                method would take unsigned which perhaps better
                expresses the range/intent of the value, but can cast
                internally to signed to do array lookup.  Anyhow, not a
                big deal and what you have is fine, obviously.</div>
            </blockquote>
            <br>
            Yes, I tend to agree, but I think I'll leave them as int for
            now. Maybe I'll change this too as part of my next cleanup.<br>
            <br>
            <blockquote type="cite">
              <div>The output looks nice with your changes -- I wonder
                though if even whitespace changes are deemed too risky
                in terms of possibly breaking client parsers (would have
                to be fairly brittle ones, but nonetheless).</div>
            </blockquote>
            <br>
            Right. However, it looks to me like this output has been
            changing its indentation levels over time, so if any parsers
            break due to white space changes they are probably already
            broken ;-)<br>
            <br>
            Thanks again for looking at this!<br>
            Bengt<br>
            <br>
            <blockquote type="cite">
              <div><br>
              </div>
              <div>Thanks,</div>
              <div><br>
              </div>
              <div>Vitaly<br>
                <br>
                <div class="gmail_quote">On Thu, Jun 21, 2012 at 7:54
                  AM, Bengt Rutisson <span dir="ltr"><<a
                      moz-do-not-send="true"
                      href="mailto:bengt.rutisson@oracle.com"
                      target="_blank">bengt.rutisson@oracle.com</a>></span>
                  wrote:<br>
                  <blockquote class="gmail_quote" style="margin: 0pt 0pt
                    0pt 0.8ex; border-left: 1px solid rgb(204, 204,
                    204); padding-left: 1ex;"><br>
                    Hi again,<br>
                    <br>
                    Updated webrev:<br>
                    <a moz-do-not-send="true"
                      href="http://cr.openjdk.java.net/%7Ebrutisso/7178361/webrev.01/"
                      target="_blank">http://cr.openjdk.java.net/~brutisso/7178361/webrev.01/</a><br>
                    <br>
                    I added some asserts as suggested by Vitaly and I
                    did some white space changes to the TraceGen0Time
                    logging. I hope this will not break any parsers. It
                    is just intended to align the output up a bit better
                    to be more readable.<br>
                    <br>
                    Here is a webrev with just the change I made
                    compared to my previous webrev:<br>
                    <a moz-do-not-send="true"
                      href="http://cr.openjdk.java.net/%7Ebrutisso/7178361/webrev.00-01-diff/"
                      target="_blank">http://cr.openjdk.java.net/~brutisso/7178361/webrev.00-01-diff/</a><br>
                    <br>
                    Here is an example of what the TraceGen0Time output
                    looks like after my change:<br>
                    <br>
                    ALL PAUSES<br>
                      Total                    =     0.95 s (avg =  
                     63.44 ms)<br>
                                                            (num =  
                     15, std dev =    47.84 ms, max =   150.30 ms)<br>
                    <br>
                    <br>
                      Young GC Pauses:       14<br>
                      Mixed GC Pauses:        1<br>
                    <br>
                    EVACUATION PAUSES<br>
                      Evacuation Pauses        =     0.95 s (avg =  
                     63.44 ms)<br>
                                                            (num =  
                     15, std dev =    47.84 ms, max =   150.30 ms)<br>
                         Root Region Scan Wait =     0.00 s (avg =    
                    0.00 ms)<br>
                         Parallel Time         =     0.94 s (avg =  
                     62.39 ms)<br>
                            Ext Root Scanning  =     0.11 s (avg =    
                    7.22 ms)<br>
                            SATB Filtering     =     0.00 s (avg =    
                    0.00 ms)<br>
                            Update RS          =     0.04 s (avg =    
                    2.81 ms)<br>
                            Scan RS            =     0.03 s (avg =    
                    2.07 ms)<br>
                            Object Copy        =     0.75 s (avg =  
                     49.75 ms)<br>
                            Termination        =     0.00 s (avg =    
                    0.02 ms)<br>
                            Parallel Other     =     0.01 s (avg =    
                    0.51 ms)<br>
                         Clear CT              =     0.00 s (avg =    
                    0.09 ms)<br>
                         Other                 =     0.01 s (avg =    
                    0.90 ms)<br>
                    <br>
                    MISC<br>
                      Stop World               =     0.01 s (avg =    
                    0.48 ms)<br>
                                                            (num =  
                     15, std dev =     0.19 ms, max =     0.79 ms)<br>
                      Yields                   =     0.00 s (avg =    
                    0.27 ms)<br>
                                                            (num =    
                    2, std dev =     0.05 ms, max =     0.32 ms)<br>
                    <br>
                    Thanks,<br>
                    Bengt
                    <div>
                      <div><br>
                        <br>
                        <br>
                        On 2012-06-20 15:15, Bengt Rutisson wrote:<br>
                        <blockquote class="gmail_quote" style="margin:
                          0pt 0pt 0pt 0.8ex; border-left: 1px solid
                          rgb(204, 204, 204); padding-left: 1ex;"> <br>
                          Hi everyone,<br>
                          <br>
                          Could I please have some reviews for this
                          change:<br>
                          <a moz-do-not-send="true"
                            href="http://cr.openjdk.java.net/%7Ebrutisso/7178361/webrev.00/"
                            target="_blank">http://cr.openjdk.java.net/~brutisso/7178361/webrev.00/</a><br>
                          <br>
                          Background<br>
                          As part of the PrintGC and PrintGCDetails
                          logging there is information about how long
                          the GC pause was. The timing of the pause was
                          done differently in G1 depending on whether
                          PrintGC or PrintGCDetails were enabled. It
                          turns out that PrintGCDetails was just timing
                          part of the pause.<br>
                          <br>
                          This change will make both PrintGC and
                          PrintGCDetails use the same timing. To achieve
                          this I had to refactor the code a bit. I moved
                          all the timing data out of G1CollectorPolicy
                          into a new class called G1GCPhaseTimes.<br>
                          <br>
                          My intention is that this change should not
                          alter the format of the output of PrintGC or
                          PrintGCDetails. It should just correct the
                          timing data.<br>
                          <br>
                          However, I did find that we are collecting
                          timing information about card counts, under an
                          #ifdef. I moved this to the finest log level
                          instead. This does not change the existing
                          format for normal usage of PrintGC or
                          PrintGCDetails.<br>
                          <br>
                          Also, I had to update how the TraceGen0Time
                          data is logged. I will have another look at
                          this, but my idea was to leave the format
                          exactly as it was. However, I think the format
                          has decayed over time so maybe I'll try to
                          clean it up.<br>
                          <br>
                          I intend to follow this change up with a
                          change to remove the special treatment of the
                          single threaded case for PrintGCDetails
                          (tracked in CR 7178363).<br>
                          <br>
                          Finally, this work revealed an issue with how
                          the ergonomics in G1 measure the collection
                          pauses. I did not want to change this behavior
                          now so I filed a separate RFE for that
                          (7178365: G1: Ergonomics only count part of
                          the collection pause).<br>
                          <br>
                          Bengt<br>
                        </blockquote>
                        <br>
                      </div>
                    </div>
                  </blockquote>
                </div>
                <br>
              </div>
            </blockquote>
            <br>
            <br>
          </div>
        </blockquote>
      </div>
    </blockquote>
    <br>
  </body>
</html>