RFR: 8278917: Use Prev Bitmap for recording evac failed objects

[Albert Mingkun Yang]

On Thu, 16 Dec 2021 16:40:09 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

Just some minor comments/suggestions.

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

> 163:     // All objects that failed evacuation has been marked in the prev bitmap.
> 164:     // Use the bitmap to apply the above closure to all failing objects.
> 165:     hr->apply_to_marked_objects(const_cast<G1CMBitMap*>(_g1h->concurrent_mark()->prev_mark_bitmap()), &rspc);

Maybe placing the bitmap on its own line, sth like `bitmap = const_cast<G1CMBitMap*>(_g1h->concurrent_mark()->prev_mark_bitmap())`?

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

> 191: 
> 192:       hr->note_self_forwarding_removal_end(live_bytes);
> 193:       _g1h->verifier()->check_bitmaps("Self-Forwarding Ptr Removal", hr);

Now that you are editing this method, why `if (_evac_failure_regions->contains(hr->hrm_index()))`? I thought that's the precondition.

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

> 93:     // location, but also clears the bitmap for it. This is needed
> 94:     // for bitmap verification as well as for being able to use the
> 95:     // prev_bitmap for evacuation failures.

Instead of clearing the bit for each obj individually, I wonder if clearing the bitmap for the whole region will be faster. IOW, just call `collector()->mark_bitmap()->clear_region(hr);` unconditionally.

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

> 1272:       if (hr->is_old()) {
> 1273:         _g1h->clear_region_bitmap(hr);
> 1274:       }

Does it make sense to assert "Young regions should always have cleared bitmaps"?

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

Marked as reviewed by ayang (Reviewer).

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