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

[Stefan Johansson]

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 record_concurrent_mark_xxx_start/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
> check_bitmaps() 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::check_bitmaps() should not
> check the next bitmap.
>
> Previously this has been done using the somewhat tricky condition
>
>   674   if (_g1h->collector_state()->mark_or_rebuild_in_progress() ||
> !_g1h->_cmThread->in_progress()) {
>   675     res_n = verify_no_bits_over_tams("next", next_bitmap, 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
>