RFR: 8279008: G1: Calculate BOT threshold on-the-fly during Object Copy phase [v2]

[Stefan Johansson]

On Mon, 10 Jan 2022 10:56:56 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Simple change of unifying BOT logic for PLAB and direct allocation during GC.
>> 
>> Test: tier1-6; no difference observed while running BigRamTester, SPECjbb2015
>
> Albert Mingkun Yang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update src/hotspot/share/gc/g1/g1BlockOffsetTable.inline.hpp
>   
>   Co-authored-by: Thomas Schatzl <59967451+tschatzl at users.noreply.github.com>

Nice cleanup, not sure if I agree that it's that simple though 😉  At least it requires a lot of testing to make sure there is no regression. 

In general it looks good, I basically just have a few naming suggestions/comments. I think it would be nice to keep the terminology to use threshold rather than boundary since this is what the BOT is currently calling it.

src/hotspot/share/gc/g1/g1BlockOffsetTable.inline.hpp line 34:

> 32: #include "runtime/atomic.hpp"
> 33: 
> 34: inline HeapWord* G1BlockOffsetTablePart::align_up_by_card_size(HeapWord* const addr) const {

What do you think about calling this `align_up_to_threshold()`.

src/hotspot/share/gc/g1/g1ParScanThreadState.cpp line 515:

> 513:       // old. If the current allocation was done outside the PLAB this call will
> 514:       // have no effect since the _top of the PLAB has not changed.
> 515:       update_bot_after_copying(obj, word_sz);

The comment above needs to be modified to match the new code. This call now handles the update for direct allocations as well.

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

> 165: 
> 166:   // Update BOT if this obj is the first entering a new card (i.e. crossing the card boundary).
> 167:   inline void update_bot_if_crossing_boundary(HeapWord* obj_start, size_t obj_size);

I would prefer using threshold instead of boundary here since threshold is the term used in the BOT.

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

> 243:          p2i(obj_start), p2i(obj_end));
> 244: 
> 245:   HeapWord* cur_card_boundary = _bot_part.align_up_by_card_size(obj_start);

I think something like `next_bot_threshold` would be even more descriptive.

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

> 246: 
> 247:   // strictly greater-than
> 248:   bool cross_card_boundary = (obj_end > cur_card_boundary);

Any reason for keeping this as a separate statement? Otherwise I would just do the comparison directly in the if-statement.

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

Changes requested by sjohanss (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6916