RFR: 8329603: G1: Merge G1BlockOffsetTablePart into G1BlockOffsetTable [v2]

Albert Mingkun Yang ayang at openjdk.org
Fri Apr 5 09:11:59 UTC 2024


On Fri, 5 Apr 2024 08:33:25 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1BlockOffsetTable.hpp line 43:
>> 
>>> 41: // start of the chunk that includes the first word of the subregion.
>>> 42: //
>>> 43: // Each G1BlockOffsetTable is owned by a HeapRegion.
>> 
>> Need revision.
>
> I think it is good to delete this line and the blank line. What do you think about it?

Removing it is fine, IMO.

>> src/hotspot/share/gc/g1/g1BlockOffsetTable.hpp line 134:
>> 
>>> 132:   }
>>> 133: 
>>> 134:   void set_for_starts_humongous(HeapRegion* hr, HeapWord* obj_top, size_t fill_size);
>> 
>> I feel this doesn't belong to BOT. Can probably be dealt with in another ticket.
>
>> I feel this doesn't belong to BOT. Can probably be dealt with in another ticket.
> 
> OK.
> 
> What about the method `G1BlockOffsetTable::verify`? Is it good to be moved to `HeapRegion` and change the name as `verify_bot`, `verify_BOT` or `verify_block_offset_table`?

That sounds reasonable. (Should not be done in this PR though.)

(My experience with BOT is that they are almost never corrupted, so doing only checking-after-each-write is enough, sth like `ObjectStartArray::verify_for_block` -- there is possibly little value in verifying BOT in `HeapRegion::verify`. I wonder what others' opinions are.)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18634#discussion_r1553227172
PR Review Comment: https://git.openjdk.org/jdk/pull/18634#discussion_r1553227054


More information about the hotspot-gc-dev mailing list