RFR: 8289538: Make G1BlockOffsetTablePart unaware of block sizes [v2]

[Albert Mingkun Yang]

On Wed, 6 Jul 2022 12:39:18 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   can I have reviews for this change that makes `G1BlockOffsetTablePart` unaware of block sizes: as discussed in PR#9059/[JDK-8287555](https://bugs.openjdk.org/browse/JDK-8287555) this is a good idea to make `G1BlockOffsetTablePart` just for "dumb" retrieval of the BOT entries, which makes the code simpler, avoiding some calling back and forth between `HeapRegion` and `G1BlockOffsetTablePart`.
>> 
>> There is still the wart that `G1BlockOffsetTablePart::verify()` needs `block_size`, but since it's just verification I think this is fine. An alternative suggested by @albertnetymk is to remove that method completely.
>> 
>> Testing: local compilation, gha, some benchmark checking that card scans/ms does not degrade
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make block_start_reaching_into_card() const

Just some minor & subjective comment.

src/hotspot/share/gc/g1/heapRegion.hpp line 151:

> 149:   // next block (or the end of the HeapRegion.)
> 150:   inline HeapWord* forward_to_block_containing_addr(HeapWord* q, HeapWord* n,
> 151:                                                     const void* addr) const;

Not sure how helpful this comment is; it's not that the text is hard to comprehend but rather this method is not a proper (internal) API -- it's highly dependent on its caller. Therefore, I suggest inlining it to its sole caller.

src/hotspot/share/gc/g1/heapRegion.inline.hpp line 89:

> 87:     // being precise, we should never have to step through more than
> 88:     // a single card.
> 89:     _bot_part.assert_same_bot_entry(n, addr);

I think `assert(!is_crossing_card_boundary(n, addr)` matches the comment better.

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

Marked as reviewed by ayang (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9395