<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <tt>Hi Thomas,</tt><br>
    <br>
    <div class="moz-cite-prefix">On 2018-03-29 16:35, Thomas Schatzl
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:1522334136.2451.15.camel@oracle.com">
      <pre wrap="">Hi all,

  can I have reviews for this small change that makes G1 switch the
mark bitmaps already during the Remark pause as opposed waiting until
Cleanup?

This is the last step before G1 can reclaim regions at Remark, which
means that empty regions can be cleaned up more more quickly than
before (JDK-8154528, coming soon :)).

The main changes apart from actually switching the bitmaps consist of
updating the liveness information, i.e. how many bytes are live in a
region, now already available at Remark since JDK-8197850, also at the
same time.

Previously G1 used the Rebuild Remsets phase to calculate that at the
same time.

I could not find significant differences in timing.

This change depends on the recent g1ConcurrentMark refactorings,
particularly JDK-8200234.

CR:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8178105">https://bugs.openjdk.java.net/browse/JDK-8178105</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8178105/">http://cr.openjdk.java.net/~tschatzl/8178105/</a></pre>
    </blockquote>
    <tt>Looks good, but a few comments:<br>
    </tt><tt><br>
      src/hotspot/share/gc/g1/g1ConcurrentMark.cpp</tt><br>
    <tt>1031     for (uint i = region_idx; i < (region_idx +
      num_regions_in_humongous); i++) {</tt><br>
    <tt>1032       HeapRegion* const r = _g1h->region_at(i);</tt><br>
    <tt>1033       size_t const words_to_add =
      MIN2(HeapRegion::GrainWords, marked_words);</tt><br>
    <tt>1034       r->add_to_marked_bytes(words_to_add *
      HeapWordSize);</tt><br>
    <tt>1035       marked_words -= words_to_add;</tt><br>
    <tt>1036     }</tt><br>
    <tt>Could be nice to assert that marked_words is zero after the
      iteration, i.e. that everything got distributed and maybe also
      that marked_words is greater than 0 during the iteration.</tt><br>
    <br>
    <tt>1047       log_trace(gc)("Added " SIZE_FORMAT " words to region
      %u (%s)", marked_words, region_idx, hr->get_type_str());</tt><br>
    <tt>I think the logging should use at least one more tag, maybe
      "region" or "marking", but you probably know what makes most sense
      in this area. I also think it would be nice if we got this same
      log-line for all regions. Now we get this for "normal" regions,
      but we get "Distributing..." for humongous start regions and "Not
      Added..." for humongous continuous. So I would suggest adding the
      same log-line to the distribute-function after line 1034.<br>
      <br>
      1045     if (!hr->is_humongous()) {<br>
      Since the comment talks about humongous regions I would prefer not
      negating the statement and have something like:<br>
      if (hr->is_humongous()) {<br>
        if (hr->is_starts_humongous()) {<br>
          distribute_marked_bytes(hr, marked_words);<br>
        } else {<br>
          assert(marked_words == 0, "continuous humongous are not
      accounted separately");<br>
        }<br>
      } else {<br>
        hr->add_to_marked_bytes(marked_words * HeapWordSize);<br>
        log_trace(gc)("Added " SIZE_FORMAT " words to region %u (%s)",
      marked_words, region_idx, hr->get_type_str());<br>
      }<br>
      <br>
    </tt><tt><span class="new">1058 }</span></tt><br>
    <tt><span class="new"></span></tt><tt><span class="new">Newline
        before public :)<br>
        <br>
        1686   if (mark_completed) {<br>
        1687     is_alive = &is_alive_prev;<br>
        1688   } else {<br>
        1689     is_alive = &is_alive_next;<br>
        1690   }<br>
        1691   _gc_tracer_cm->report_object_count_after_gc(is_alive);<br>
        <br>
        I would prefer moving the call into the if and only setup the
        closure when needed:<br>
        if (mark_completed) {<br>
          G1ObjectCountIsAliveClosure is_alive(_g1h);<br>
          _gc_tracer_cm->report_object_count_after_gc(&is_alive);<br>
        } else {<br>
          G1CMIsAliveClosure is_alive(_g1h);<br>
          _gc_tracer_cm->report_object_count_after_gc(&is_alive);
        <br>
        }<br>
      </span></tt><tt>----<br>
    </tt>
    <pre><span class="new"></span></pre>
    <tt><span class="new">Thanks,<br>
        Stefan<br>
        <br>
      </span></tt><tt><br>
    </tt>
    <blockquote type="cite"
      cite="mid:1522334136.2451.15.camel@oracle.com">
      <pre wrap="">
Testing:
hs-tier 1-5

Thanks,
  Thomas

</pre>
    </blockquote>
    <br>
  </body>
</html>