RFR: 8278917: Use Prev Bitmap for recording evac failed objects [v4]

[Thomas Schatzl]

On Tue, 21 Dec 2021 09:55:53 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Please review this enhancement to improve evacuation failure handling in G1.
>> 
>> **Summary**
>> Prior to this change the evacuation failure handling recorded the failing objects during evacuation in a set backed by `G1SegmentedArray`. Later when processing the objects the set needed to be sorted to allow for efficient zapping of the areas between the live objects. During this phase we also mark the live objects in the prev mark bitmap to allow for correct verification.
>> 
>> This change optimizes away sorting part by directly marking failing objects in the mark bitmap. The marks are then used to determine which objects need to be processed (and what areas should be zapped) when cleaning up after the evacuation phase. For this to work, there can't be any stale marks in the bitmap. This require us to do some additional clearing that was not needed before. Both at the end of a Full GC and during preparation for mixed collections. 
>> 
>> This additional clearing does cost a bit, but are done in parallel and the benefit outweighs the cost. For the Full GC the clearing of the bitmap is moved into the compaction closure. This is something that was thought of before and the closure already have the bitmap as a member. This way the Full GC regression becomes minimal.
>> 
>> **Testing** 
>> 
>> - [x] Mach5 with and without additional evacuation failures injected
>> - [x] Local testing to verify the evacuation failure processing is improved
>> - [x] Performance testing not generating evacuation failures show no regression
>> - [x] Full GC tests show very limited regressions due to additional clearing in compaction phase
>
> Stefan Johansson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Hamlin review

Lgtm, some bikeshedding :p

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 625:

> 623:   void verify_region_attr_remset_is_tracked() PRODUCT_RETURN;
> 624: 
> 625:   void clear_region_in_prev_bitmap(HeapRegion* hr);

A name like `clear_prev_bitmap_for_region` would be preferable to me, putting what we clear right next to the verb.

src/hotspot/share/gc/g1/g1FullGCCompactTask.cpp line 97:

> 95:     // location, but also clears the bitmap for it. This is needed
> 96:     // for bitmap verification as well as for being able to use the
> 97:     // prev_bitmap for evacuation failures. Testing showed that it

... for evacuation failures in the next young collections.

src/hotspot/share/gc/g1/g1FullGCCompactTask.hpp line 53:

> 51:   class G1CompactRegionClosure : public StackObj {
> 52:     G1CMBitMap* _bitmap;
> 53:     void clear_bitmap(oop object);

I would prefer a name like `par_clear_in_prev_bitmap` like other single bit operations for the bitmap (drop the `par` if you do not like it) - we do not clear the entire bitmap as the current name implies to me.

src/hotspot/share/gc/g1/g1ParScanThreadState.cpp line 620:

> 618:     HeapRegion* r = _g1h->heap_region_containing(old);
> 619: 
> 620:     // Objects failing evacuation will turn in to old objects since the regions

s/in to/into

src/hotspot/share/gc/g1/g1ParScanThreadState.cpp line 621:

> 619: 
> 620:     // Objects failing evacuation will turn in to old objects since the regions
> 621:     // is relabeled as such. We mark the failing objects in the prev bitmap and

s/is/are

src/hotspot/share/gc/g1/g1RemSet.cpp line 1262:

> 1260:   };
> 1261: 
> 1262:   // Closure to clear the prev bitmap for any old region in the CSet. This is

I would prefer if terms like `CSet` are not abbreviated in comments, i.e. s/CSet/collection set.

src/hotspot/share/gc/g1/g1RemSet.cpp line 1286:

> 1284:   };
> 1285: 
> 1286:   // Helper to allow multiple closure to be applied when

s/multiple/two :)

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

Marked as reviewed by tschatzl (Reviewer).

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