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

[Hamlin Li]

On Mon, 20 Dec 2021 20:53:20 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:
> 
>   Fix product build

Lgtm. Just some minor comments.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2960:

> 2958: }
> 2959: 
> 2960: void G1CollectedHeap::clear_region_bitmap(HeapRegion* hr) {

Maybe currently a name like `clear_region_in_prev_bitmap` is more clear.

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

> 1255:   inline bool is_obj_dead_full(const oop obj) const;
> 1256: 
> 1257:   // Mark the live object that failed evacuation in the correct bitmap(s).

As we are explicitly using prev bitmap, so maybe comment should say so too.

src/hotspot/share/gc/g1/g1EvacFailure.cpp line 79:

> 77:     zap_dead_objects(_last_forwarded_object_end, obj_addr);
> 78: 
> 79:     // Zapping clears the bitmap, make sure it didn't clear to much.

s/to/too

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

> 81:   assert(cast_to_oop(destination)->klass() != NULL, "should have a class");
> 82: 
> 83:   clear_bitmap(obj);

Maybe a simple comment here will be helpful, although we already have comments where G1CompactRegionClosure is used.

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

Marked as reviewed by mli (Reviewer).

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