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

[Stefan Johansson]

On 2018-04-03 16:55, Thomas Schatzl wrote:
> Hi,
> 
>    thanks for your patience...
> 
> On Tue, 2018-04-03 at 14:49 +0200, Stefan Johansson wrote:
>>
>> 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,
>>>>> [...]
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8178105
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~tschatzl/8178105/
>>>>
>>>>    Looks good, but a few comments:
>>>>
> [...]
>>> 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.
> 
> My fault. I did not upload the latest version of the change :( It is up
> now, the only difference to previous is line
> 
> 1036       assert(words_to_add > 0, "Out of space to distribute before
> end of humongous object in region %u (starts %u)", i, region_idx);
> 
> But there is a new webrev anyway... in the 1_to_2 webrev it is line
> 1033
> 
>>>>
>>>> 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:
> 
> Okay. I think I got your intention right this time :)
> 
>> 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);
> [...]
>>> 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.
> 
> http://cr.openjdk.java.net/~tschatzl/8178105/webrev.1_to_2/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8178105/webrev.2/ (full)
Looks good!

Thanks,
StefanJ

> 
> Ran gc/g1 tests successfully with that change.
> 
> Thanks,
>    Thomas
>