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

[Thomas Schatzl]

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)

Thanks,
  Thomas