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

[Stefan Johansson]

On 2018-04-04 14:42, Thomas Schatzl wrote:
> Hi all,
> 
>    while looking at some parallelizing work in this area I noticed that
> one assert in the rebuild remsets code that has been changed in this
> change already needs an update too.
> It uses a wrong value for the informational text, which makes it really
> confusing.
> 
> Since this change touches this code already, I would like to amend this
> change with this small fix:
> 
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8178105/webrev.2_to_3/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8178105/webrev.3/ (full)
> 
> Sorry for the trouble.
NP, still good!

Stefan
> 
> Thanks,
>    Thomas
> 
> On Tue, 2018-04-03 at 17:00 +0200, Stefan Johansson wrote:
>>
>> 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
>>>
>