<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hi Bengt,<br>
    <br>
    Thanks. I'll make the change as you suggest. For some reason the
    push job failed on MacOS. It looks exactly like the problem these
    changes are supposed to fix.<br>
    <br>
    JohnC<br>
    <br>
    <div class="moz-cite-prefix">On 3/18/2013 4:40 AM, Bengt Rutisson
      wrote:<br>
    </div>
    <blockquote cite="mid:5146FD28.7070609@oracle.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      <div class="moz-cite-prefix"><br>
        Hi John,<br>
        <br>
        This looks good to me. Just like I thought ;)<br>
        <br>
        One nit if you feel like fixing it before you push:<br>
        <br>
        In a couple places you have this pattern:<br>
        <br>
        <meta http-equiv="content-type" content="text/html;
          charset=UTF-8">
        2240           _task-&gt;do_marking_step(mark_step_duration_ms,<br>
        2241                                  false      /*
        do_termination */,<br>
        2242                                  _is_serial /*
        is_serial      */);<br>
        <br>
        I think the comment /* is_serial */ is superfluous now that we
        pass a variable named _is_serial instead of just true or false.<br>
        <br>
        Bengt<br>
        <br>
        <br>
        On 3/14/13 6:19 PM, John Cuthbertson wrote:<br>
      </div>
      <blockquote cite="mid:51420692.10802@oracle.com" type="cite">Hi
        Everyone, <br>
        <br>
        Here's the final webrev based upon Bengt's comments: <br>
        <br>
        <a moz-do-not-send="true" class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/%7Ejohnc/8009536/webrev.2/">http://cr.openjdk.java.net/~johnc/8009536/webrev.2/</a>
        <br>
        <br>
        I think I'm all set after reviews from Bengt and Thomas - I just
        wanted to send out the final version of the webrev in case
        anyone else has other comments. <br>
        <br>
        Thanks, <br>
        <br>
        JohnC <br>
        <br>
        On 3/13/2013 2:01 PM, Bengt Rutisson wrote: <br>
        <blockquote type="cite"> <br>
          Hi John, <br>
          <br>
          Thanks for explaining a bit about the serial case where we
          still need to go through termination protocol. <br>
          <br>
          As you know I'll be on vacation the next two days, so I just
          wanted to let you know that with the changes that you outline
          below (the fix to CMRemarkTask and removing the do_stealing
          argument) I'm fine with this change. Feel free to push this if
          you get another review. <br>
          <br>
          Thanks, <br>
          Bengt <br>
          <br>
          On 3/13/13 6:57 PM, John Cuthbertson wrote: <br>
          <blockquote type="cite">Hi Bengt, <br>
            <br>
            Thanks for looking at the change again. Replies inline.... <br>
            <br>
            On 3/13/2013 3:22 AM, Bengt Rutisson wrote: <br>
            <blockquote type="cite"> <br>
              <br>
              Hi John, <br>
              <br>
              I'm not too familiar with the do_marking_step() code, so
              I'm trying to get to know it while I'm looking at your
              webrev. So, maybe I'm completely off here, but I think
              there are a couple of strange things. <br>
              <br>
              After your change do_marking_step() takes three bools:
              do_stealing, do_termination and is_serial. <br>
              <br>
              This means that we have 8 possible states if we combine
              all value of these bools. I went through all calls to
              do_marking_step() and it seems like we make use of 4 of
              those states: <br>
              <br>
              <br>
              G1CMKeepAliveAndDrainClosure::do_oop_work() <br>
                do_marking_step(false, false, true)             (1) <br>
              <br>
              CMConcurrentMarkingTask::work() <br>
                do_marking_step(true, true, false)              (2) <br>
              <br>
              CMRemarkTask::work() <br>
                do_marking_step(true, true, false)              (2) <br>
                do_marking_step(true, true, true)               (3) <br>
              <br>
              G1CMDrainMarkingStackClosure::do_void() <br>
                do_marking_step(false, true, true)              (4) <br>
                do_marking_step(true, true, false)              (2) <br>
              <br>
            </blockquote>
            <br>
            Yeah. Well... <br>
            <br>
            <blockquote type="cite"> <br>
              I numbered the states (1) - (4). Here are all states and I
              marked which ones we use: <br>
              <br>
               stealing,  termination,  serial    (3) <br>
               stealing, !termination,  serial <br>
               stealing, !termination, !serial <br>
               stealing,  termination, !serial    (2) parallel <br>
              !stealing,  termination,  serial    (4) <br>
              !stealing, !termination,  serial    (1) serial <br>
              !stealing, !termination, !serial <br>
              !stealing,  termination, !serial <br>
              <br>
              <br>
              I think state (2) makes sense as the parallel case. We do
              stealing and termination and we are not serial. State (1)
              also makes sense. It is clearly the serial case. No
              stealing or termination. Just serial. <br>
              <br>
              Now, case (3) looks odd to me. We say we are serial but we
              want to do stealing and termination. This case comes from
              CMRemarkTask. It takes a is_serial argument in its
              constructor. We get to case (3) from
              ConcurrentMark::checkpointRootsFinalWork(), where we do: <br>
              <br>
                if (G1CollectedHeap::use_parallel_gc_threads()) { <br>
                  ... <br>
                } else { <br>
                  ... <br>
                  CMRemarkTask remarkTask(this, active_workers, true /*
              is_serial*/); <br>
                } <br>
              <br>
              To me this looks like it should lead to the pure serial
              case (1) instead of (3). Do you agree? In that case I
              think CMRemarkTask::work() needs to be changed to do: <br>
              <br>
                      task-&gt;do_marking_step(1000000000.0 /* something
              very large */, <br>
                                            !_is_serial /* do_stealing
              */, <br>
                                            !_is_serial /*
              do_termination */, <br>
                                            _is_serial); <br>
              <br>
            </blockquote>
            <br>
            You're right here. The serial remark case should not enable
            stealing. It doesn't make sense. Done! <br>
            <br>
            <blockquote type="cite"> <br>
              The other case that sticks out is (4). We don't want to do
              stealing, but we want to do termination even though we are
              serial. This comes from how we set up the
              g1_drain_mark_stack in ConcurrentMark::weakRefsWork(). We
              pass that along to the reference processing, which I think
              is serial (wasn't that the original problem? :) ). We also
              pass the g1_keep_alive which is set up to be in state (1).
              So, I wonder if it is correct that the g1_drain_mark_stack
              is in state (4). If you agree that we should be using the
              pure serial case here too, I think we need to change
              G1CMDrainMarkingStackClosure::do_void() to be: <br>
              <br>
                    _task-&gt;do_marking_step(1000000000.0 /* something
              very large */, <br>
                                           !_is_serial /* do_stealing   
              */, <br>
                                           !_is_serial /* do_termination
              */, <br>
                                           _is_serial  /* is_serial */);
              <br>
              <br>
              <br>
            </blockquote>
            <br>
            That is what we had before and I wasn't happy with it. The
            serial drain closure is the last thing that get's executed
            as part of reference processing - when process_phaseJNI
            invokes the complete_gc.do_void() method. In the drain
            closure either serial or parallel I think we want to go
            through the termination protocol. We just don't want to
            interact with the terminator in the serial case. There's a
            bunch of guarantees and checks in the termination protocol
            and we should still go through them - even when serial. <br>
            <br>
            <blockquote type="cite"> <br>
              Again, I'm not sure about this, but if the above is
              correct we will be left with only two states. The parallel
              and the serial. In that case I think it would make sense
              to remove the do_stealing and do_termination arguments for
              do_marking_step() and just pass in the is_serial argument.
              Internally do_marking_step() could set up two local
              variable to track stealing and termination. That might be
              good if we see the need in the future to split these up
              again. <br>
            </blockquote>
            <br>
            I think we can get rid of the do_stealing parameter - it's
            always the negation of the is_serial parameter and I like
            the idea of making it a local. How about that? For example <br>
            <br>
            void CMTask::do_marking_step(double ms, bool do_termination,
            bool is_serial) { <br>
              // It does not make sense to allow stealing when called
            serially. <br>
              bool do_stealing = !is_serial; <br>
            <br>
            Thanks, <br>
            <br>
            JohnC <br>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>