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

Stefan Johansson stefan.johansson at oracle.com
Wed Mar 28 12🔞15 UTC 2018

Hi Thomas,

On 2018-03-26 17:59, Thomas Schatzl wrote:

Hi all,

can I have reviews for this change that cleans up Remark and Cleanup pause code? It mainly moves the recordconcurrentmarkxxxstart/end() methods to the top/bottom of the remark() and cleanup() methods, and refactors the verification code into a single method. There is one more somewhat tricky related change here that fixes the checkbitmaps() method of that verification: after we swapped bitmaps (in Cleanup right now) until the previous prev-bitmap has been cleared, its bits are invalid, and G1HeapVerifier::checkbitmaps() should not check the next bitmap. Previously this has been done using the somewhat tricky condition 674 if (g1h->collectorstate()->markorrebuildinprogress() || !g1h->cmThread->inprogress()) { 675 resn = verifynobitsovertams("next", nextbitmap, ntams, end); 676 } which you might immediately understand as the best way to prevent checking the next bitmap from the Cleanup until the (new) next bitmap has been cleared, but I did not. Further, there is no similarly easy condition if the bitmap swapping is moved to the Remark pause, so I created a new flag in G1CollectorState that directly records that we are currently clearing the next bitmap. 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?

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.

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; })

Thanks, Stefan

Testing: hs-tier 1-5

It is based on JDK-8151171 which is also out for review currently. Thanks, Thomas

More information about the hotspot-gc-dev mailing list