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

Thomas Schatzl thomas.schatzl at oracle.com
Wed Mar 28 14:06:06 UTC 2018

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->collectorstate()->markorrebuildinprogress() ? nextmarkbitmap : prevmarkbitmap; The old code always used the nextmarkbitmap, 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 jiosnprintf 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 shouldverifyoops() { 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: debugonly(bool shouldverifyoops() { 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

More information about the hotspot-gc-dev mailing list