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

[Thomas Schatzl]

Hi Stefan,

On Tue, 2018-04-03 at 12:58 +0200, Stefan Johansson wrote:
> 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 

Done.

> and maybe also that marked_words is greater than 0 during the
> iteration.

Can't check that - both sizes are unsigned type. The MIN2() in line
1033 prevents words_to_add to ever be greater than marked_words, so I
think a check like that will be useless.

I added a different kind of check that verifies what you probably
thought of, and that is that the number of words to add to the current
region must always be larger than zero. If it is zero, there is
something really wrong.

> 
> 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.

Done.

> 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); 
> }

All fixed.

Webrevs:
http://cr.openjdk.java.net/~tschatzl/8178105/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8178105/webrev.1/ (full)
Testing:
locally ran all gc/g1 jtreg tests with no problems

Thomas