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

[Thomas Schatzl]

On Wed, 29 Jun 2022 11:26:59 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> @kimbarrett: The `block_start` method returns the first object reaching into `addr`; if we aligned first, then the result would be the first object reaching into `aligned(addr)` which is different; the callers would need to advance forward not-requested objects then.
>> Or maybe I'm missing something in your suggestion?
>
>> 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?

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

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