RFR: 8242847: G1 should not clear mark bitmaps with no marks

[Thomas Schatzl]

On Mon, 23 Aug 2021 07:51:51 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

> Hi all,
> 
> Please review this change to bound the range of bitmap clearing for each region using the liveness data collected during marking. For the Concurrent Undo Mark cycle, the liveness information (next_top_at_mark_start and live_words) is in sync wrt. the _next_mark_bitmap that needs clearing. Hence, we use these details to clear only bitmaps for regions that were dirtied and need clearing i.e. only clear between [bottom, ntams), and only clear bitmaps for regions that had at least one bit set (i.e. have some live data).
> 
> Testing: Tier 1-3.

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 591:

> 589: 
> 590:   bool has_aborted() {
> 591:     if (_cm != NULL) {

Maybe put the predicate `_cm != nullptr` in an extra method, something like `is_clear_concurrent()` (or a better name) would improve readability...

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 614:

> 612:       return r->next_top_at_mark_start();
> 613:     }
> 614:     return r->end();

If not in the `UndoMark` operation (and doing the work concurrently), we could use the `prev_top_at_mark_start()` here, couldn't we?

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 618:

> 616: 
> 617:   public:
> 618:     G1ClearBitmapHRClosure(G1CMBitMap* bitmap, G1ConcurrentMark* cm) : HeapRegionClosure(), _bitmap(bitmap), _cm(cm) {

Since this code is so highly specific to clearing `_next_bitmap` (and depending on current state which TAMS is the correct one for a given region) I would prefer if the code did not have configurable `bitmap` too.

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 673:

> 671:   }
> 672: 
> 673: 

Unnecessary newline.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5213