<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 Per,<br>
      <br>
      On 10/21/15 6:42 AM, Per Liden wrote:<br>
    </div>
    <blockquote cite="mid:56276C18.2040102@oracle.com" type="cite">Hi
      Derek,
      <br>
      <br>
      On 2015-10-20 21:56, Derek White wrote:
      <br>
      <blockquote type="cite">4th Webrev for review please:
        <br>
        <br>
        This version includes the cleanups suggested by Per.
        <br>
        <br>
        RFE: JDK-8138920
        <a class="moz-txt-link-rfc2396E" href="https://bugs.openjdk.java.net/browse/JDK-8138920"><https://bugs.openjdk.java.net/browse/JDK-8138920></a>
        <br>
        Refactor the sampling thread from ConcurrentG1RefineThread
        <br>
        <br>
        Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8138920/webrev.04/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.04/</a>
        <br>
      </blockquote>
      <br>
      <br>
      Looks good over all. Just a few minor things I noticed.
      <br>
      <br>
      concurrentG1Refine.cpp
      <br>
      ----------------------
      <br>
      <br>
       92     vm_shutdown_during_initialization("Could not create
      ConcurrentSampleThread");
      <br>
      <br>
      Should be "G1YoungListSampleThread" now.
      <br>
    </blockquote>
    <br>
    OK.<br>
    <blockquote cite="mid:56276C18.2040102@oracle.com" type="cite">
      <br>
       160 void
      ConcurrentG1Refine::print_worker_threads_on(outputStream* st)
      const {
      <br>
       161   for (uint i = 0; i < _n_worker_threads; ++i) {
      <br>
       162     _threads[i]->print_on(st);
      <br>
       163     st->cr();
      <br>
       164   }
      <br>
       165   _sample_thread->print_on(st);
      <br>
       166   st->cr();
      <br>
      <br>
      You didn't introduce this, but shouldn't there be a if (_threads
      != NULL) here around this, similar to the other functions which
      access _threads?
      <br>
    </blockquote>
    Deleted tests as suggested by Thomas.<br>
    <blockquote cite="mid:56276C18.2040102@oracle.com" type="cite">
      concurrentG1RefineThread.cpp
      <br>
      ----------------------------
      <br>
      <br>
       199 void ConcurrentG1RefineThread::stop_service() {
      <br>
       200   {
      <br>
       201     MutexLockerEx x(_monitor,
      Mutex::_no_safepoint_check_flag);
      <br>
       202     _monitor->notify();
      <br>
       203   }
      <br>
       204 }
      <br>
      <br>
      The extra scope here seems superfluous.
      <br>
    </blockquote>
    <br>
    OK.<br>
    <blockquote cite="mid:56276C18.2040102@oracle.com" type="cite">
      concurrentG1RefineThread.hpp
      <br>
      ----------------------------
      <br>
      <br>
        71   virtual void run_service();
      <br>
        72   virtual void stop_service();
      <br>
      <br>
      Please make these non-virtual.
      <br>
    </blockquote>
    <br>
    OK. I'll revirtualize in
    <meta http-equiv="content-type" content="text/html; charset=utf-8">
    <a class="issue-link" data-issue-key="JDK-8140257"
      href="https://bugs.openjdk.java.net/browse/JDK-8140257"
      id="key-val" rel="4850322">8140257</a>.
    <blockquote cite="mid:56276C18.2040102@oracle.com" type="cite">
      <br>
      <br>
      g1YoungListSampleThread.hpp
      <br>
      ---------------------------
      <br>
      <br>
        30 class G1YoungListSampleThread: public ConcurrentGCThread {
      <br>
        31   Monitor* _monitor;
      <br>
      <br>
      Please add a "private:" here.<br>
    </blockquote>
    <br>
    OK. BTW, I noticed pretty inconsistent indentation of "private:" in
    the G1 code, especially right after the class name. Some cases are
    zero indent and some are one (1!). I'm assuming that zero is
    correct.<br>
    <blockquote cite="mid:56276C18.2040102@oracle.com" type="cite">
      g1YoungListSampleThread.cpp
      <br>
      ---------------------------
      <br>
      <br>
        59   _monitor = new Monitor(Mutex::nonleaf,
      <br>
        60                          "Sample thread monitor",
      <br>
        61                          true,
      <br>
        62                          Monitor::_safepoint_check_never);
      <br>
      <br>
      It looks like _monitor could be a value member instead of a
      pointer, to avoid the extra allocation here.
      <br>
      <br>
        66   set_name("G1 Sample");
      <br>
      <br>
      This should probably be updated to reflect the new name.
      <br>
    </blockquote>
    <br>
    OK.<br>
    <blockquote cite="mid:56276C18.2040102@oracle.com" type="cite">
      <br>
      cheers,
      <br>
      /Per
      <br>
      <br>
    </blockquote>
    Thanks Per!<br>
    <br>
     - Derek<br>
    <blockquote cite="mid:56276C18.2040102@oracle.com" type="cite">
      <blockquote type="cite">
        <br>
        jprt: waiting for results (which might take a long time given
        current
        <br>
        jprt status).
        <br>
        <br>
        Thanks for looking!
        <br>
        <br>
          - Derek
        <br>
        <br>
        On 10/20/15 1:41 PM, Derek White wrote:
        <br>
        <blockquote type="cite">Thanks for the review Per! Comments
          below...
          <br>
          <br>
          On 10/20/15 2:56 AM, Per Liden wrote:
          <br>
          <blockquote type="cite">Hi Derek,
            <br>
            <br>
            On 2015-10-19 22:08, Derek White wrote:
            <br>
            <blockquote type="cite">Thanks Bengt (and Per indirectly).
              Comments below.
              <br>
              <br>
              On 10/19/15 3:17 PM, Bengt Rutisson wrote:
              <br>
              <blockquote type="cite">Hi Derek,
                <br>
                <br>
                On 2015-10-17 03:32, Derek White wrote:
                <br>
                <blockquote type="cite">3rd Webrev for review please:
                  <br>
                  <br>
                  This version does away with the common abstract base
                  case
                  <br>
                  ConcurrentServiceThread, and pushes the code down to
                  the concrete
                  <br>
                  classes. This may get cleaned up in a later fix to
                  <br>
                  ConcurrentGCThread.
                  <br>
                  <br>
                  <br>
                  RFE: JDK-8138920
                  <a class="moz-txt-link-rfc2396E" href="https://bugs.openjdk.java.net/browse/JDK-8138920"><https://bugs.openjdk.java.net/browse/JDK-8138920></a>
                  <br>
                  Refactor the sampling thread from
                  ConcurrentG1RefineThread
                  <br>
                  <br>
                  Webrev:
                  <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8138920/webrev.03/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.03/</a>
                  <br>
                </blockquote>
                <br>
                I think this version is much good.
                <br>
                <br>
                I talked a bit to Per about the latest webrev and he
                pointed out that
                <br>
                we don't really need the run_service()/stop_service()
                abstraction now
                <br>
                that the threads implement all this themselves. The
                change would be
                <br>
                smaller if we left that change out.
                <br>
                <br>
                I'll leave it up to you if you want to keep it in there
                or not. Do you
                <br>
                plan on following this work up by starting to work on
                JDK-8138920
                <br>
                pretty soon? In that case I think the
                run_service()/stop_service()
                <br>
                abstraction will have to be handled there anyway which
                makes it a
                <br>
                smaller issue now in my opinion.
                <br>
              </blockquote>
              I went back and forth about including
              run_service()/stop_service(), but
              <br>
              decided that the refactoring had already been done, and
              I'd like to
              <br>
              push
              <br>
              run()/stop() up to ConcurrentGCThread like we talked about
              soon. There
              <br>
              are a few extra complications that will also need to be
              handled (like
              <br>
              G1StringDedupThread already has a static stop() method).
              <br>
            </blockquote>
            <br>
            I'm ok with keeping it like this if the ConcurrentGCThread
            cleanup is
            <br>
            something we'll get to soon-ish.
            <br>
            <br>
            concurrentSampleThread.hpp/cpp
            <br>
            ------------------------------
            <br>
            <br>
            * I'd suggest we name the new thread it G1SampleThread (or
            maybe
            <br>
            G1YoungListSampleThread, or something similar). The fact
            that it's
            <br>
            concurrent doesn't seem important enough to make it part of
            the name,
            <br>
            better to try to say something about what it does.
            <br>
          </blockquote>
          OK.
          <br>
          <br>
          <blockquote type="cite">* sleepBeforeNextCycle() should be
            sleep_before_next_cycle(). I know
            <br>
            concurrentMarkThread and maybe some other thread also has a
            <br>
            sleepBeforeNextCycle() and I think that should be changed
            too at some
            <br>
            point (not saying you need to do that in this change).
            <br>
            <br>
          </blockquote>
          OK
          <br>
          <blockquote type="cite">* The protected members should be
            private, i.e. :
            <br>
            <br>
              30 class ConcurrentSampleThread: public ConcurrentGCThread
            {
            <br>
              31 protected:
            <br>
              32   Monitor* _monitor;
            <br>
              33
            <br>
              34   void sample_young_list_rs_lengths();
            <br>
              35
            <br>
              36   void run_service();
            <br>
              37   void stop_service();
            <br>
              38
            <br>
              39   void sleepBeforeNextCycle();
            <br>
              40
            <br>
              41   double _vtime_accum;  // Accumulated virtual time.
            <br>
          </blockquote>
          OK
          <br>
          <blockquote type="cite">* I could be wrong here, but I can see
            any immediate need to include
            <br>
            these, or?
            <br>
            <br>
              30 #include "memory/resourceArea.hpp"
            <br>
              31 #include "runtime/handles.inline.hpp"
            <br>
            <br>
          </blockquote>
          Not likely. I'll Check.
          <br>
          <br>
          <blockquote type="cite">concurrentG1Refine.hpp/cpp
            <br>
            --------------------------
            <br>
            <br>
            * As Thomas mentioned, I think the sample thread ownership
            should
            <br>
            move from the ConcurrentG1Refine to to G1CollectedHeap. That
            seems to
            <br>
            make more sense, no?
            <br>
          </blockquote>
          <br>
          Yes, but I'd like to leave that as a separate fix. One that
          actually
          <br>
          allows concurrent refinement to be disabled. It would take me
          a bit to
          <br>
          figure out how to that correctly (and test).
          <br>
          <br>
          Thanks again, new webrev in a bit...
          <br>
          <br>
           - Derek
          <br>
          <blockquote type="cite">
            <br>
            cheers,
            <br>
            /Per
            <br>
            <br>
            <blockquote type="cite">
              <br>
                - Derek
              <br>
              <blockquote type="cite">
                <br>
                Thanks,
                <br>
                Bengt
                <br>
                <br>
                <blockquote type="cite">
                  <br>
                  Passed jprt (finally!).
                  <br>
                  <br>
                  Thanks for looking!
                  <br>
                  <br>
                   - Derek
                  <br>
                  <br>
                  On 10/9/15 12:01 PM, Bengt Rutisson wrote:
                  <br>
                  <blockquote type="cite">
                    <br>
                    Hi Derek,
                    <br>
                    <br>
                    Comments inlined.
                    <br>
                    <br>
                    On 2015-10-09 00:29, Derek White wrote:
                    <br>
                    <blockquote type="cite">Another call for review:
                      <br>
                      <br>
                      2nd webrev:
                      <br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8138920/webrev.02/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.02/</a>
                      <br>
                      <br>
                      See changes and comments below:
                      <br>
                      <br>
                      On 10/8/15 2:47 AM, Bengt Rutisson wrote:
                      <br>
                      <blockquote type="cite">
                        <br>
                        Hi Derek,
                        <br>
                        <br>
                        On 2015-10-07 17:19, Derek White wrote:
                        <br>
                        <blockquote type="cite">Hi Bengt,
                          <br>
                          <br>
                          On 10/7/15 4:02 AM, Bengt Rutisson wrote:
                          <br>
                          <blockquote type="cite">Hi Derek,
                            <br>
                            <br>
                            Thanks for fixing this!
                            <br>
                            <br>
                            On 2015-10-06 19:51, Derek White wrote:
                            <br>
                            <blockquote type="cite">Refactor and cleanup
                              the G1 concurrent thread classes:
                              <br>
                               - Pull out a sampling thread class (now
                              <br>
                              ConcurrentG1SampleThread) from
                              ConcurrentG1RefineThread.
                              <br>
                               - Factor out an abstract base class
                              ConcurrentG1ServiceThread
                              <br>
                              that is used by:
                              <br>
                                  - ConcurrentG1RefineThread
                              <br>
                                  - ConcurrentG1SampleThread
                              <br>
                                  - ConcurrentMarkThread
                              <br>
                               - Made the handling of the "primary"
                              refinement thread more
                              <br>
                              explicit.
                              <br>
                               - Updated obsolete and confusing comments
                              <br>
                              <br>
                              This is tech debt that also will allow
                              disabling concurrent
                              <br>
                              refinement (if desired) and also fixes a
                              P4 bug.
                              <br>
                              Patch started by Thomas and improved
                              and/or mangled myself.
                              <br>
                              <br>
                              RFE: JDK-8138920
                              <br>
                              <a class="moz-txt-link-rfc2396E" href="https://bugs.openjdk.java.net/browse/JDK-8138920"><https://bugs.openjdk.java.net/browse/JDK-8138920></a>
                              Refactor the
                              <br>
                              sampling thread from
                              ConcurrentG1RefineThread
                              <br>
                              <br>
                              Webrev:
                              <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8138920/webrev.01/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.01/</a>
                              <br>
                            </blockquote>
                            <br>
                            Overall this looks really good to me.
                            <br>
                            <br>
                            Some comments:
                            <br>
                            <br>
                            No one seems to depend on
                            <br>
                            ConcurrentG1ServiceThread::vtime_accum().
                            All uses have the
                            <br>
                            specific subclass available. So, I don't
                            think the pure virtual
                            <br>
                            declaration in ConcurrentG1ServiceThread is
                            needed. I'd just
                            <br>
                            remove that and also make the corresponding
                            methods in the
                            <br>
                            subclasses non-virtual.
                            <br>
                          </blockquote>
                          <br>
                          OK. At some point we need to systematic
                          rewrite of timing, but
                          <br>
                          that can wait.
                          <br>
                        </blockquote>
                        <br>
                        Quite agree.
                        <br>
                        <br>
                        <blockquote type="cite">
                          <blockquote type="cite">That more or less only
                            leaves the run() and stop() methods in
                            <br>
                            ConcurrentG1ServiceThread. It is kind of
                            nice for the subclasses
                            <br>
                            to get help with this, but I wonder if it is
                            not possible to
                            <br>
                            improve the ConcurrentGCThread class to help
                            with this instead.
                            <br>
                            Basically I guess I am a little unsure if
                            the extra class
                            <br>
                            ConcurrentG1ServiceThread is really needed.
                            <br>
                            <br>
                          </blockquote>
                          I'll look at ConcurrentGCThread to see how
                          well it could cover
                          <br>
                          these cases.
                          <br>
                        </blockquote>
                        <br>
                        Thanks. I think it is worth a try. If it doesn't
                        turn out well we
                        <br>
                        can keep the intermediate class, but I think it
                        is worth
                        <br>
                        exploring.
                        <br>
                      </blockquote>
                      <br>
                      I looked at pushing ConcurrentServiceThread up
                      into
                      <br>
                      ConcurrentGCThread, but things got a little
                      complicated.
                      <br>
                      ConcurrentG1RefineThread, ConcurrentMarkThread,
                      and
                      <br>
                      ConcurrentSampleThread have a very "regularized"
                      implementation of
                      <br>
                      the "termination protocol". G1StringDedupThread is
                      slightly off
                      <br>
                      from this, and ConcurrentMarkSweepThread more so.
                      Pushing the
                      <br>
                      shared code up into ConcurrentGCThread but not
                      using it in
                      <br>
                      G1StringDedupThread and ConcurrentMarkSweepThread
                      seems confusing.
                      <br>
                      <br>
                      There's a tension between providing a framework
                      that handles all
                      <br>
                      shared factorizable code, but can become a
                      straight jacket for
                      <br>
                      future code, and everyone doing everything
                      separately and
                      <br>
                      differently. This webrev is somewhere in the
                      middle. Some of the
                      <br>
                      changes between webrev.01 and .02 are to make the
                      duplicated code
                      <br>
                      more similar, even though it's not shared.
                      <br>
                    </blockquote>
                    <br>
                    I see. Thanks for investigating it!
                    <br>
                    <br>
                    I think I agree with Per, though. The value of
                    <br>
                    ConcurrentServiceThread in its current form is IMHO
                    not really worth
                    <br>
                    the extra complexity of having it. I would prefer to
                    just duplicate
                    <br>
                    the code in ConcurrentG1RefineThread,
                    ConcurrentMarkThread, and
                    <br>
                    ConcurrentSampleThread for now and then have a
                    separate change to
                    <br>
                    try to clean this part up.
                    <br>
                    <br>
                    <blockquote type="cite">
                      <br>
                      <blockquote type="cite">
                        <blockquote type="cite">
                          <blockquote type="cite">Naming. The naming in
                            G1 is a bit inconsistent. Some files and
                            <br>
                            classes are prefixed with G1 and some are
                            not. But if they are
                            <br>
                            called something with G1 it is normally a
                            prefix. So, I would
                            <br>
                            prefer the new classes to be called
                            G1Concurrent* instead of
                            <br>
                            ConcurrentG1*.
                            <br>
                          </blockquote>
                          <br>
                          So we have:
                          <br>
                              - ConcurrentG1RefineThread
                          <br>
                              - ConcurrentMarkThread
                          <br>
                          <br>
                          And I added:
                          <br>
                              - ConcurrentG1SampleThread
                          <br>
                              - ConcurrentG1ServiceThread
                          <br>
                          <br>
                          And maybe I'm removing
                          ConcurrentG1ServiceThread. In that case
                          <br>
                          I'd be inclined to rename:
                          <br>
                               ConcurrentG1SampleThread =>
                          ConcurrentSampleThread
                          <br>
                        </blockquote>
                        <br>
                        Sounds good. The ConcurrentG1Refine* classes are
                        really the only
                        <br>
                        oddly named G1 classes, so I think it is better
                        not to let that
                        <br>
                        naming spread.
                        <br>
                      </blockquote>
                      This version includes the class renaming.
                      <br>
                    </blockquote>
                    <br>
                    Thanks for fixing this.
                    <br>
                    <br>
                    If we do decide to keep ConcurrentServiceThread
                    around I think it
                    <br>
                    would be better to move ConcurrentSampleThread into
                    its own file. It
                    <br>
                    is a separate enough entity to warrant its own file
                    I think.
                    <br>
                    <br>
                    <blockquote type="cite">
                      <blockquote type="cite">
                        <br>
                        <blockquote type="cite">
                          <blockquote type="cite">You write "and also
                            fixes a P4 bug". Which bug is that?
                            <br>
                          </blockquote>
                          JDK-8136856
                          <a class="moz-txt-link-rfc2396E" href="https://bugs.openjdk.java.net/browse/JDK-8136856"><https://bugs.openjdk.java.net/browse/JDK-8136856></a>
                          G1
                          <br>
                          makes two concurrent refinement threads with
                          <br>
                          -XX:G1ConcRefinementThreads=1
                          <br>
                          (because the sampling thread "is-a" concurrent
                          refinement thread.
                          <br>
                        </blockquote>
                        <br>
                        Ah. I see. Makes sense. Thanks.
                        <br>
                        <br>
                        But it is still not possible to turn refinement
                        off by setting
                        <br>
                        -XX:G1ConcRefinementThreads=0 since that is
                        considered the
                        <br>
                        default, right?
                        <br>
                      </blockquote>
                      <br>
                      I'm not sure about this. If I recall correctly,
                      Thomas implied that
                      <br>
                      it was hard to disable concurrent refinement
                      without disabling the
                      <br>
                      sampling thread too.
                      <br>
                    </blockquote>
                    <br>
                    I'm thinking about this code in
                    Arguments::set_g1_gc_flags():
                    <br>
                    <br>
                      if (G1ConcRefinementThreads == 0) {
                    <br>
                        FLAG_SET_DEFAULT(G1ConcRefinementThreads,
                    ParallelGCThreads);
                    <br>
                      }
                    <br>
                    <br>
                    Could we change that, so that you could turn off
                    refinement by
                    <br>
                    setting -XX:G1ConcRefinementThreads=0 ? It should
                    probably be
                    <br>
                    handled as a separate fix though.
                    <br>
                    <br>
                    Thanks,
                    <br>
                    Bengt
                    <br>
                    <br>
                    <blockquote type="cite">
                      <br>
                      <blockquote type="cite">
                        <blockquote type="cite">I cant' recall how to
                          mark a bug as blocking another bug.
                          <br>
                        </blockquote>
                        <br>
                        You add a link (More->Link) to the other bug
                        and choose "block" or
                        <br>
                        "is blocked by".
                        <br>
                        <br>
                        Thanks,
                        <br>
                        Bengt
                        <br>
                      </blockquote>
                      Thanks for the tip!
                      <br>
                      <br>
                      - Derek
                      <br>
                      <br>
                    </blockquote>
                    <br>
                  </blockquote>
                  <br>
                </blockquote>
                <br>
              </blockquote>
              <br>
            </blockquote>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>