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

[Stefan Johansson]

On Tue, 21 Dec 2021 10:06:04 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Stefan Johansson has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Hamlin review
>
> 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.

I prefer this as well.

> 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.

Added.

> 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.

Good point, I'll update the name, but will leave the "par"-part out as this is not done with a par-operation. Since the whole region is handled by a single worker the clearing is done with a `_bm.clear_bit()` and I can't come up with a case where this would be a problem. The regions and bitmap should be aligned so that there are no words in the bitmap spanning two regions, right?

> 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.

I agree.

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

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