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

[Stefan Johansson]

On 2018-04-03 14:25, Thomas Schatzl wrote:
> 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.
Yes, but it could be if equal marked_words will be 0 and I want to 
assert that this never happens before the last iteration.
> 
> 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.
Exactly, that's what I wanted. But I don't see that check now, just 
having a assert(marked_words != 0, "") on row 1032 would be enough.
> 
>>
>> 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.
Sorry for being picky but the current solution won't print anything for 
continuous humongous. Adding the log to distribute_marked_bytes() would 
solve this and we would still get info on region type so I see no need for:
1052         log_trace(gc, marking)("Adding " SIZE_FORMAT " words to 
humongous start region %u (%s), word size %d (%f)",
1053                                marked_words, region_idx, 
hr->get_type_str(),
1054                                oop(hr->bottom())->size(), 
(double)oop(hr->bottom())->size() / HeapRegion::GrainWords);

> 
>> 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)
Apart from the small things above this looks great.

Thanks,
Stefan

> Testing:
> locally ran all gc/g1 jtreg tests with no problems
> 
> Thomas
>