RFR: 8287555: Tighten G1 G1BlockOffsetTable::block_start() code

[Albert Mingkun Yang]

On Thu, 30 Jun 2022 10:18:15 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>>> I had something like that, but the "not observable" part was not that clear in my results
>> 
>> I added the following to `G1BlockOffsetTablePart::block_start` to see the cost.
>> 
>> 
>>   if (is_aligned(addr, BOTConstants::card_size())) {
>>     return q;
>>   }
>> 
>> 
>> Using `Scan Heap Roots (ms)` and `Scanned Cards` from `-Xlog:gc,gc+phases`, I can calculate #cards processed per ms. Surprisingly, I can observe some reduction (~2.7%) in this metric on my box.
>> 
>> Re Kim's suggestion, the model where `G1BlockOffsetTablePart::block_start` accepts only card-aligned addresses and the caller (`HeapRegion::block_start`) handles block-walking (heap parsing) makes more sense to me. (After all, "block size" is sth meaningful only inside `HeapRegion`.) Ofc, this can be dealt with in this PR or another.
>
> [Here](https://github.com/openjdk/jdk/compare/master...tschatzl:jdk:submit/move-block-size-out-of-bot?expand=1)'s a prototype for (trying to) move `block_start` out of `G1BlockOffsetTablePart`; in the end I have a bit of mixed feelings about it:
> * the BlockOffsetTable deals with blocks; blocks intrinsically have a start and a size; removing the BlockOffsetTable to only deal with parts of a block seems a bit limited.
> Additionally not knowing about size will require to move out `verify` too if we are strict about this distinction, which means that lots of code is being dumped in the already (too) large `HeapRegion` class. Of course, like in the prototype, the `G1BlockOffsetTablePart::verify` could also stay as is....
> * the BlockOffsetTable is already very tightly dependent on `HeapRegion` anyway (for current layout, i.e. `bottom`, `top`, `end`).
> 
> Comments?

The structure of the patch looks good. Both options (leaking block-walking logic to `G1BlockOffsetTablePart` or moving `verify` to `HeapRegion`) are OK to me, though I prefer a third option -- removing `verify()` directly, since the BOT algorithm is not updated constantly. `verify()` essentially never fails.

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

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