RFR: 8272773: Investigate making card table size configurable

[Thomas Schatzl]

On Wed, 6 Oct 2021 12:50:25 GMT, Vishal Chand <github.com+10235864+vish-chan at openjdk.org> wrote:

> Hi,
> 
> Please review the changes to make CardTable entry size configurable. The changes primarily consists of:
> 
> 1. Addition of a cmdline flag **GCCardSizeInBytes** to make the card size startup time configurable.
> 2. Setting the card size based on the flag in G1, Parallel and Serial GC memory initialization paths.
> 3. Setting BlockOffsetTable size and ObjectStartArray size based on the card size.

Hi Vish-chan,

  thanks for your contribution.

Could you configure Github actions (see the green box where it says "Testing is not configured") ? This will uncover the issues found when trying to compile it for performance testing.

This performance testing will take bit.

I am also curious why you enabled minimum card size of 128 bytes? Did you see any advantages in your testing on small heaps?

Also, the other comments are based on a very passing look on the code.

Thanks,
  Thomas

src/hotspot/share/gc/g1/heapRegion.cpp line 96:

> 94:   // Initialize card size based on the region size.
> 95:   // Maximum no. of cards per region is 2^16.
> 96:   CardTable::initialize_card_size(1 << (region_size_log - 16));

Please make a constant out of the `16`; the reason is the size of the `G1CardSetArray::EntryDataType` being 16 bit.

src/hotspot/share/gc/g1/heapRegion.cpp line 103:

> 101:   LogCardsPerRegion = log2i(CardsPerRegion);
> 102: 
> 103:   assert(LogCardsPerRegion <= 16, "Total cards per region should be less than or equal to 2^16");

Same here. This check should probably be moved into (G1) argument processing instead of being checked here all the time.

src/hotspot/share/gc/parallel/objectStartArray.cpp line 48:

> 46:   // size blocks as the card table.
> 47:   assert((int)block_size == (int)CardTable::card_size, "Sanity");
> 48:   assert((int)block_size <= 1024, "block_size must be less than or equal to 1024");

The maximum of `1024` is derived from that we need an extra bit for possible offsets in the byte for backskip values (at least that's a hard limit). Please factor out a constant and comment it.
Maybe for now, also check that card element size and offset table element size are equal (although they need not be, but the code may assume that).

src/hotspot/share/gc/parallel/objectStartArray.hpp line 57:

> 55:   static uint block_shift;
> 56:   static uint block_size;
> 57:   static uint block_size_in_words;

I *think* the naming guideline for static members is the same as for regular class members, i.e. use an underscore in front. Maybe wait on changing this for another person to comment on (I did not look up the style guide).
We do not use public static members too often :)

src/hotspot/share/gc/shared/cardTable.cpp line 48:

> 46:   // GCCardSizeInBytes is rounded off to the nearest power of 2, and clamped
> 47:   // between min and max card sizes
> 48:   card_size = GCCardSizeInBytes;

I would prefer, instead of rounding, add a constraint function on that option that makes sure that the given value is a power of two.

src/hotspot/share/gc/shared/cardTable.cpp line 50:

> 48:   card_size = GCCardSizeInBytes;
> 49:   card_size = round_up_power_of_2(card_size);
> 50:   card_size = clamp(card_size, MAX2(min_card_size,card_size_min), card_size_max);

I think this is unnecessary: the constraint on the option already prohibits that the given value is within [128, 1024]

src/hotspot/share/gc/shared/cardTable.cpp line 52:

> 50:   card_size = clamp(card_size, MAX2(min_card_size,card_size_min), card_size_max);
> 51:   card_shift = log2i_exact(card_size);
> 52:   card_size = 1 << card_shift;

Windows compilation correctly fails here:

[2021-10-07T09:16:43,181Z] cardTable.cpp(52): error C2220: the following warning is treated as an error
[2021-10-07T09:16:43,181Z] cardTable.cpp(52): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)

src/hotspot/share/gc/shared/cardTable.cpp line 64:

> 62:     FLAG_SET_ERGO(GCCardSizeInBytes, card_size);
> 63:   }
> 64: 

This would also be unnecessary with a constraint function that limits the values of the option.

src/hotspot/share/gc/shared/cardTable.cpp line 65:

> 63:   }
> 64: 
> 65:   log_info(gc, barrier)("CardTable entry size: " UINTX_FORMAT,  card_size);

Please use `log_info_p` here, probably with `gc, init` so that this is marked as "precious" so that this is also printed to the `hs_err` file.

src/hotspot/share/gc/shared/cardTable.cpp line 96:

> 94:   assert((uintptr_t(_whole_heap.start())  & (card_size - 1))  == 0, "heap must start at card boundary");
> 95:   assert((uintptr_t(_whole_heap.end()) & (card_size - 1))  == 0, "heap must end at card boundary");
> 96:   assert(card_size >= card_size_min && card_size <= card_size_max, "card_size must be between min and max");

Also unnecessary with the constraint function.

src/hotspot/share/gc/shared/cardTable.cpp line 468:

> 466: uintx CardTable::ct_max_alignment_constraint() {
> 467:   // CardTable max alignment is computed with card_size_max
> 468:   return card_size_max * os::vm_page_size();

I think this change causes some tests to fail because their heap will get too large. Maybe it is possible to move code so that the actual card size value is used here to avoid this, or fix the errors (will be visible with enabled github actions).

src/hotspot/share/gc/shared/cardTable.hpp line 234:

> 232:   static uintx card_shift;
> 233:   static uintx card_size;
> 234:   static uintx card_size_in_words;

Please do not use `uintx`; I think `uint` is just fine for those.

src/hotspot/share/gc/shared/cardTable.hpp line 238:

> 236:   // min and max permissible card sizes
> 237:   static const uintx card_size_min = 128;
> 238:   static const uintx card_size_max = 1024;

static consts should be CamelCased.

src/hotspot/share/gc/shared/gc_globals.hpp line 700:

> 698:           "Card table entry size (in bytes) for card based collectors")     \
> 699:           range(128, 1024)                                                  \
> 700:   // end of GC_FLAGS

See e.g. `MaxTenuringThreshold` for how adding a constraint function works.

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

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