RFR (S/M): 8200234: Cleanup Remark and Cleanup pause code

[sangheon.kim]

Hi Thomas,

On 03/28/2018 07:06 AM, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2018-03-28 at 14:18 +0200, Stefan Johansson wrote:
>> Hi Thomas,
>>
>> On 2018-03-26 17:59, Thomas Schatzl wrote:
>>> Hi all,
>>>
> [...]
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8200234
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8200234/webrev/
>> Looks good in general, but a few questions and suggestions:
>> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
>>    539   G1CMBitMap* const bitmap =
>> _g1h->collector_state()->mark_or_rebuild_in_progress() ?
>> _next_mark_bitmap : _prev_mark_bitmap;
>> The old code always used the _next_mark_bitmap, was this a bug or
>> has any timing changed?
> This is a (benign?) bug, and actually thinking about it again the
> current implementation is not completely correct either. :/
>
> Thanks for making me think about this again.
>
> The prev ("current") bitmap may contain the mark because of the
> humongous object surviving some previous marking.
>
> Now if you eagerly reclaim that object, the mark will simply remain
> until the next time we swap the bitmaps and clear the now "next"
> bitmap, so it's kind of self-healing after all.
>
> As for the actual impact, I do not think it has any except for stray
> verification errors I saw with some of our tests: we never use the
> bitmap for walking free regions (they are not walked at all), and if we
> reallocated into that region again, the objects would all be above
> tams, and so we would not use the bitmap either and live anyway.
>
> But I think this issue is unrelated to this cleanup change and will
> create an extra CR.
>
>> 1010     os::snprintf(buffer, BufLen, "During GC (%s)", caller);
>> Seems like jio_snprintf is more used in the HotSpot code, so maybe
>> change to use that one instead.
> Okay, I just did not know which to use. Both seemed equal to me. Fixed.
>
>> ---
>> src/hotspot/share/memory/iterator.hpp
>>    136 #ifdef ASSERT
>>    137   bool should_verify_oops() { return false; }
>>    138 #endif
>> Why is this needed? I don't see any use of NoHeaderExtendedOopClosure
>> in the new code. If needed I would prefer:
>> debug_only(bool should_verify_oops() { return false; })
> The reason is that without that change this prevents an OopClosure that
> does verification to actually trigger its typically more useful error
> message.
>
> But looking at it again at the other collector's verification closure,
> the consensus is that these verification closures should themselves
> override this method and not disable it globally.
>
> The particular issue is about G1Mux2Closure that if there is an error,
> it errors out too early, not showing G1 specific information as this
> assert triggers before the other; I will make a seperate change for
> that one too though.
>
> http://cr.openjdk.java.net/~tschatzl/8200234/webrev.0_to_1 (diff)
> http://cr.openjdk.java.net/~tschatzl/8200234/webrev.1 (full)
webrev.1 looks good.
Please update copyright year at heapRegion.inline.hpp before push.

Thanks for this cleanup!

Sangheon


>
> Thanks,
>    Thomas
>