RFR: 8329771: G1: Refactor G1BlockOffsetTable::verify [v2]

[Guoxiong Li]

On Fri, 12 Apr 2024 07:19:42 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Guoxiong Li has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
>> 
>>  - Extract a common method.
>>  - Move into ASSERT block.
>>  - Merge branch 'master' into REFACTOR_VERIFY
>>  - JDK-8329771
>
> src/hotspot/share/gc/g1/g1BlockOffsetTable.cpp line 216:
> 
>> 214: #endif
>> 215: 
>> 216:   debug_only(G1BlockOffsetTable::verify_for_block(blk_start, blk_end);)
> 
> I would prefer to make the `verify_for_block` method have a `NOT_DEBUG_RETURN` and enclose the actual method with an `#ifdef ASSERT` - this avoids the `debug_only` macro use which is kind of distracting imo.

Fixed. There is a `ASSERT` block above `G1BlockOffsetTable::verify_for_block(blk_start, blk_end);` so I put it into the `ASSERT` block.

> src/hotspot/share/gc/g1/g1BlockOffsetTable.cpp line 234:
> 
>> 232:            (uint) offset_array(current_card),
>> 233:            (uint) offset_array(current_card),
>> 234:            (uint) (CardTable::card_size_in_words() + BOTConstants::N_powers - 1));
> 
> The first assert could have the same descriptive assert message than the other; define a static method that gets the upper bound passed.
> Well, maybe not because of the `> 0` condition, but that could be separated as an extra assert maybe.

I moved the common content to a new method. But I don't mark it as `static` because the `offset_array` it uses is not `static`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18711#discussion_r1562357045
PR Review Comment: https://git.openjdk.org/jdk/pull/18711#discussion_r1562357116