RFR (S/M): 8178105: Switch mark bitmaps during Remark

[Stefan Johansson]

Hi Thomas,

On 2018-03-29 16:35, Thomas Schatzl wrote:
> 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:
> https://bugs.openjdk.java.net/browse/JDK-8178105
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8178105/
Looks good, but a few comments:

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
1031     for (uint i = region_idx; i < (region_idx + 
num_regions_in_humongous); i++) {
1032       HeapRegion* const r = _g1h->region_at(i);
1033       size_t const words_to_add = MIN2(HeapRegion::GrainWords, 
marked_words);
1034       r->add_to_marked_bytes(words_to_add * HeapWordSize);
1035       marked_words -= words_to_add;
1036     }
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.

1047       log_trace(gc)("Added " SIZE_FORMAT " words to region %u 
(%s)", marked_words, region_idx, hr->get_type_str());
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.

1045     if (!hr->is_humongous()) {
Since the comment talks about humongous regions I would prefer not 
negating the statement and have something like:
if (hr->is_humongous()) {
   if (hr->is_starts_humongous()) {
     distribute_marked_bytes(hr, marked_words);
   } else {
     assert(marked_words == 0, "continuous humongous are not accounted 
separately");
   }
} else {
   hr->add_to_marked_bytes(marked_words * HeapWordSize);
   log_trace(gc)("Added " SIZE_FORMAT " words to region %u (%s)", 
marked_words, region_idx, hr->get_type_str());
}

1058 }
Newline before public :)

1686   if (mark_completed) {
1687     is_alive = &is_alive_prev;
1688   } else {
1689     is_alive = &is_alive_next;
1690   }
1691   _gc_tracer_cm->report_object_count_after_gc(is_alive);

I would prefer moving the call into the if and only setup the closure 
when needed:
if (mark_completed) {
   G1ObjectCountIsAliveClosure is_alive(_g1h);
   _gc_tracer_cm->report_object_count_after_gc(&is_alive);
} else {
   G1CMIsAliveClosure is_alive(_g1h);
   _gc_tracer_cm->report_object_count_after_gc(&is_alive);
}
----

Thanks,
Stefan


> Testing:
> hs-tier 1-5
>
> Thanks,
>    Thomas
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180403/4dc316e5/attachment.htm>