<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">RFR 3rd version.<br>
      <br>
      Cleans up ConcurrentMarkSweepThread implementation a little more,
      adds getters for should_terminate() and has_terminated() to
      ConcurrentGCThread (and made fields private).<br>
      <br>
      <b>Bug</b>: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8140257">https://bugs.openjdk.java.net/browse/JDK-8140257</a><br>
      <b>Webrev</b>:
      <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8140257/webrev.04/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.04/</a> <br>
      <b>Incremental webrev:</b>
      <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8140257/webrev.v02.v04/webrev/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.v02.v04/webrev/</a><br>
      <br>
      Thanks!<br>
      <br>
       - Derek<br>
      <br>
      On 3/8/16 10:35 AM, Derek White wrote:<br>
    </div>
    <blockquote cite="mid:56DEF124.9030004@oracle.com" type="cite">Hi
      Per,
      <br>
      <br>
      On 3/8/16 5:48 AM, Per Liden wrote:
      <br>
      <blockquote type="cite">Hi Derek,
        <br>
        <br>
        On 2016-03-07 20:36, Derek White wrote:
        <br>
        <blockquote type="cite">Hi Per,
          <br>
          <br>
          Thanks for the comments. More below...
          <br>
          <br>
          On 3/7/16 5:25 AM, Per Liden wrote:
          <br>
          <blockquote type="cite">Hi Derek,
            <br>
            <br>
            On 2016-03-03 18:14, Derek White wrote:
            <br>
            <blockquote type="cite">RFR 2nd version.
              <br>
              <br>
              New version is focused on making ConcurrentMarkSweepThread
              a proper
              <br>
              subclass of ConcurrentGCThread, especially related to
              sharing the same
              <br>
              initialization and termination protocols. See incremental
              webrev
              <br>
              <a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.01.v.02/"><http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.01.v.02/></a>
              for
              <br>
              details.
              <br>
              <br>
                * Move CMS-specific code to
              run_service()/stop_service(), inherit
              <br>
                  run()/stop() methods.
              <br>
                * Removed ConcurrentMarkSweepThread::_should_terminate,
              use inherited
              <br>
                  _should_terminate instead.
              <br>
                * Use the inherited _has_terminated flag instead of
              _cmst to denote
              <br>
                  termination. Users call cmst() instead of reading
              _cmst.
              <br>
                * Change ConcurrentMarkSweepThread::start() and stop()
              to match G1's
              <br>
                  handling - assume stop() only called after start(), so
              <br>
                  ConcurrentGCThread objects have been created.
              <br>
                    o CMS and G1 start() methods called in same place:
              <br>
                      Universe::Initialize_heap(), and the stop()
              methods are called
              <br>
                      in same place: before_exit(), so they have the
              same lifetimes
              <br>
                      for their ConcurrentGCThreads.
              <br>
              <br>
              <br>
              *Bug*: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8140257">https://bugs.openjdk.java.net/browse/JDK-8140257</a>
              <br>
              *Webrev*:
              <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8140257/webrev.02/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.02/</a>
              <br>
            </blockquote>
            <br>
            Thanks for doing this cleanup. Looks good overall, just two
            minor
            <br>
            comments:
            <br>
            <br>
            concurrentMarkSweepThread.hpp
            <br>
            -----------------------------
            <br>
            <br>
              88   inline static ConcurrentMarkSweepThread* cmst() {
            <br>
              89     if (_cmst != NULL &&
            !_cmst->_has_terminated) {
            <br>
              90       return _cmst;
            <br>
              91     }
            <br>
              92     return NULL;
            <br>
              93   }
            <br>
            <br>
            Checking _has_terminated here seems a bit strange. The use
            case where
            <br>
            _cmst == NULL indicated that termination had completed seems
            to be
            <br>
            gone now from ConcurrentMarkSweepThread::stop(), and I don't
            see any
            <br>
            other uses of this.
            <br>
          </blockquote>
          The "_cmst != NULL" on line 89 isn't to catch termination, but
          to
          <br>
          protect the dereference "_cmst->_has_terminated". _cmst
          could be NULL if
          <br>
          not initialized.
          <br>
          <br>
          What I was trying to do here is preserve the existing behavior
          for
          <br>
          readers of the _cmst flag. Originally "_cmst != NULL" meant
          that the
          <br>
          cmst thread had started and had not terminated. In the new
          code, _cmst
          <br>
          never reverts to NULL at termination, so I changed the cmst()
          function
          <br>
          to catch both conditions.
          <br>
        </blockquote>
        <br>
        I'm questioning if adding logic to cmst() to cater for the
        termination condition is a good idea. I think callers which want
        to check is the thread has terminated should be doing
        cmst()->has_terminated() instead.
        <br>
      </blockquote>
      <br>
      OK, I'll make cmst() a normal trivial getter. I think most of the
      callers will end up doing "if (cmst() != NULL &&
      !cmst()->has_terminated())", but that's OK.
      <br>
      <blockquote type="cite">
        <blockquote type="cite">Maybe "cmst()" is a poor name - the
          usual convention is that a getter is
          <br>
          a trivial wrapper around a private/protected field. Maybe
          <br>
          "active_cmst()" or "running_cmst()" would be better?
          <br>
        </blockquote>
        <br>
        Given my comment above I think you should just leave it as
        cmst().
        <br>
        <br>
        <blockquote type="cite">
          <blockquote type="cite">Also, the above code looks potentially
            racy, if the thread terminates
            <br>
            at the same time (not sure about all contexts where this
            could be
            <br>
            called).
            <br>
          </blockquote>
          I don't follow this. Can you give some more detail? I don't
          see a /new/
          <br>
          race here - _has_terminated is set in the same place as as
          _cmst used to
          <br>
          be cleared.
          <br>
          <br>
          I can't promise there weren't any old races. For example
          cmst() could go
          <br>
          NULL between lines 166 and 167 below in the new code, or _cmst
          could go
          <br>
          NULL between lines 191 and 192 in the old code below.
          <br>
        </blockquote>
        <br>
        Right, this is the race I'm referring to. Your change didn't
        introduce it, but we have the opportunity to remove it if cmst()
        just return _cmst as I suggest.
        <br>
      </blockquote>
      OK, I see your point. With the change, the race might still print
      on a terminated cmst, but at least it won't segfault!
      <br>
      <br>
      <blockquote type="cite">
        <blockquote type="cite">
          <blockquote type="cite"> 165 void
            ConcurrentMarkSweepThread::print_all_on(outputStream* st) {
            <br>
             166   if (cmst() != NULL) {
            <br>
             167     cmst()->print_on(st);
            <br>
             168     st->cr();
            <br>
             169   }
            <br>
            <br>
             189 void
            ConcurrentMarkSweepThread::threads_do(ThreadClosure* tc) {
            <br>
             190   assert(tc != NULL, "Null ThreadClosure");
            <br>
             191   if (_cmst != NULL) {
            <br>
             192     tc->do_thread(_cmst);
            <br>
             193   }
            <br>
          </blockquote>
          <br>
          BTW, the lines 189-193 are old code, but not old versions of
          the new
          <br>
          code at 165-169. So I'm not sure if you were just showing the
          new code
          <br>
          or comparing old vs. new code here?
          <br>
        </blockquote>
        <br>
        Sorry, I cut-n-pasted the wrong version. It doesn't really
        matter since the new version has the same potential race.
        <br>
      </blockquote>
      NP, I just wanted to make sure I wasn't missing something subtle.
      <br>
      <blockquote type="cite">
        <br>
        <blockquote type="cite">
          <blockquote type="cite">I'd suggest we keep:
            <br>
            <br>
              89   static ConcurrentMarkSweepThread* cmst()    { return
            _cmst; }
            <br>
            <br>
            and never set _cmst to NULL, unless there's some very good
            reason I'm
            <br>
            missing here.
            <br>
          </blockquote>
          <br>
          The other callers of cmst() are in assertions. Arguably code
          like the
          <br>
          following is really trying to ensure that the cmst thread has
          started
          <br>
          and has not terminated.
          <br>
                  assert(ConcurrentMarkSweepThread::cmst() != NULL,
          <br>
                        "CMS thread must be running");
          <br>
        </blockquote>
        <br>
        I see, but I would suggest that we instead change the assert to
        check cmst()->has_terminated() instead. Wouldn't that make
        more sense?
        <br>
      </blockquote>
      <br>
      OK, as long as we check both cmst() != NULL and
      cmst()->has_terminated() (unless I can ensure that the caller
      runs after initialization is complete).
      <br>
      <br>
      I'll get a new webrev out soon.
      <br>
      <br>
      Thanks again!
      <br>
      <br>
       - Derek
      <br>
      <blockquote type="cite">
        <br>
        thanks,
        <br>
        Per
        <br>
        <br>
        <blockquote type="cite">
          <br>
          <blockquote type="cite">concurrentGCThread.hpp
            <br>
            ----------------------
            <br>
            <br>
              34 protected:
            <br>
              35   bool volatile _should_terminate;
            <br>
              36   bool _has_terminated;
            <br>
            <br>
            Please make these private and add a protected getter for
            <br>
            _should_terminate. _has_terminated shouldn't need a getter
            after the
            <br>
            changes related to my other comment.
            <br>
          </blockquote>
          <br>
          OK, that sounds good. I think a "has_terminated()" getter
          would be
          <br>
          useful though.
          <br>
          <br>
          Thanks for the review!
          <br>
          <br>
            - Derek
          <br>
          <blockquote type="cite">
            <br>
            cheers.
            <br>
            /Per
            <br>
            <br>
            <blockquote type="cite">*Incremental 1 vs 2*:
              <br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8140257/webrev.01.v.02/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.01.v.02/</a>
              <br>
              <br>
              *Tests*:
              <br>
              - jprt
              <br>
              - Aurora Perf (including Startup3-Server-CMS,
              Footprint3-Server-CMS)
              <br>
              - Aurora Test "hs-nightly-gc-cms".
              <br>
              <br>
              Thanks for looking!
              <br>
                - Derek
              <br>
              <br>
              On 2/26/16 11:51 PM, Derek White wrote:
              <br>
              <blockquote type="cite">I'm working on a new webrev, so
                please hold off on reviewing.
                <br>
                <br>
                Some offline comments from Kim suggest trying another
                approach. Any
                <br>
                other approach :-) He rightly pointed out the poor match
                between the
                <br>
                new code and ConcurrentMarkSweepThread is pretty ugly.
                <br>
                <br>
                Two other options I'm looking at are either having
                <br>
                ConcurrentMarkSweepThread not subclass from
                ConcurrentGCThread, or
                <br>
                (more likely) refactor ConcurrentMarkSweepThread to use
                the common
                <br>
                termination protocol instead of doing it's own thing.
                The approach of
                <br>
                adding an intermediate class that handles the common
                code being
                <br>
                factored out was rejected in review comments for
                "8138920".
                <br>
                <br>
                 - Derek
                <br>
                <br>
                On 2/26/16 11:56 AM, Derek White wrote:
                <br>
                <blockquote type="cite">*Background*:
                  <br>
                  ConcurrentGCThread provides incomplete support for an
                  initialization
                  <br>
                  and termination protocol for GC threads, so missing
                  parts are
                  <br>
                  duplicated in almost all subclasses.
                  <br>
                  <br>
                  *Fix*:
                  <br>
                  Move duplicated run(), and stop() methods up from
                  subclasses
                  <br>
                  ConcurrentG1RefineThread, ConcurrentMarkThread,
                  G1StringDedupThread,
                  <br>
                  and G1YoungRemSetSamplingThread to ConcurrentGCThread,
                  as well as
                  <br>
                  declare virtual methods run_service() and
                  stop_service.
                  <br>
                  <br>
                  Note that ConcurrentMarkSweepThread is the odd-ball
                  subclass. It
                  <br>
                  implements it's own termination protocol, it provides
                  it's own run()
                  <br>
                  and stop() methods, and does not use run_service() and
                  stop_service().
                  <br>
                  <br>
                  *Bug*:
                  <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8140257">https://bugs.openjdk.java.net/browse/JDK-8140257</a>
                  <br>
                  *Webrev*:
                  <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8140257/webrev.01/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.01/</a>
                  <br>
                  <br>
                  *Tests*: jprt, Aurora "nightly" run (I think this is
                  OK)
                  <br>
                  <a class="moz-txt-link-freetext" href="http://aurora.ru.oracle.com/faces/Batch.xhtml?batchName=1325690.VMSQE.adhoc.JPRT">http://aurora.ru.oracle.com/faces/Batch.xhtml?batchName=1325690.VMSQE.adhoc.JPRT</a>
                  <br>
                  <br>
                </blockquote>
                <br>
                <br>
              </blockquote>
              <br>
            </blockquote>
          </blockquote>
          <br>
        </blockquote>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>