RFR: 8318647: Serial: Refactor BlockOffsetTable

[Thomas Schatzl]

On Mon, 23 Oct 2023 08:14:46 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> The diff is too large; maybe it's better to read the new impl directly, which I believe is much easier to follow.
> 
> There is some duplication with G1's implementation, which should probably be dealt with in its own PR.

Initial batch of comments.

src/hotspot/share/gc/serial/serialBlockOffsetTable.cpp line 84:

> 82: }
> 83: 
> 84: // Write the backskip value for each region.

Suggestion:

// Write the backskip value for the given region.

src/hotspot/share/gc/serial/serialBlockOffsetTable.cpp line 120:

> 118:                                              HeapWord* blk_end) {
> 119:   HeapWord* const cur_card_boundary = align_up_by_card_size(blk_start);
> 120:   size_t const offset_card =  _array->index_for(cur_card_boundary);

Suggestion:

  size_t const offset_card = _array->index_for(cur_card_boundary);

src/hotspot/share/gc/serial/serialBlockOffsetTable.cpp line 131:

> 129:   if (offset_card != end_card) {
> 130:     // Handling remaining cards.
> 131:     size_t start_card_for_region = offset_card + 1;

Maybe `start_card_for/to_update` is a better name?

src/hotspot/share/gc/serial/serialBlockOffsetTable.cpp line 135:

> 133:       // -1 so that the reach ends in this region and not at the start
> 134:       // of the next.
> 135:       size_t reach = offset_card + BOTConstants::power_to_cards_back(i+1) - 1;

Suggestion:

      size_t reach = offset_card + BOTConstants::power_to_cards_back(i + 1) - 1;

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp line 43:

> 41: // systems using card-table-based write barriers, the efficiency of this
> 42: // operation may be important.  Implementations of the BlockOffsetTable
> 43: // class may be useful in providing such efficient implementations.

The comment is completely outdated, talking about subtypes where there are none any more of this class.

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp line 45:

> 43: // class may be useful in providing such efficient implementations.
> 44: 
> 45: class BlockOffsetSharedArray: public CHeapObj<mtGC> {

`BlockOffsetSharedArray` could probably be moved to the .cpp file.

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp line 45:

> 43: // class may be useful in providing such efficient implementations.
> 44: 
> 45: class BlockOffsetSharedArray: public CHeapObj<mtGC> {

Should be prefixed by `Serial`, same as `BlockOffsetTable` (since this is a complete reimplementation anyway).

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp line 55:

> 53:   // address.
> 54:   VirtualSpace _vs;
> 55:   u_char* _offset_array;          // byte array keeping backwards offsets

I always wondered whether `u_char` is what we want here, maybe `uint8_t` is more appropriate?

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp line 141:

> 139:                                         HeapWord* const obj_end) {
> 140:     HeapWord* cur_card_boundary = align_up_by_card_size(obj_start);
> 141:     // strictly greater-than

This comment should not only mention what the code below does but also why.

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp line 157:

> 155: };
> 156: 
> 157: 

extra newline

src/hotspot/share/gc/serial/serialBlockOffsetTable.inline.hpp line 31:

> 29: 
> 30: #include "gc/shared/space.hpp"
> 31: #include "runtime/safepoint.hpp"

These includes seem superfluous.

src/hotspot/share/gc/serial/serialBlockOffsetTable.inline.hpp line 35:

> 33: //////////////////////////////////////////////////////////////////////////
> 34: // BlockOffsetSharedArray inlines
> 35: //////////////////////////////////////////////////////////////////////////

I think this comment does not add value and should be removed.

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

Changes requested by tschatzl (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16304#pullrequestreview-1696951303
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371541243
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371540936
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371545013
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371542603
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371530149
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371527773
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371531382
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371545829
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371535236
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371535817
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371539016
PR Review Comment: https://git.openjdk.org/jdk/pull/16304#discussion_r1371537058