RFR: 8272773: Investigate making card table size configurable [v5]

[Thomas Schatzl]

On Wed, 17 Nov 2021 12:25:00 GMT, Vishal Chand <duke at openjdk.java.net> 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.
>
> Vishal Chand has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Remove G1 region size and card size dependency
>  - Merge branch 'card-size-configurable' of https://github.com/vish-chan/jdk into card-size-configurable
>  - Merge branch 'master' of https://github.com/vish-chan/jdk into card-size-configurable

Changes requested by tschatzl (Reviewer).

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

> 59:   // Maximum size an offset table entry can cover. This maximum is derived from that
> 60:   // we need an extra bit for possible offsets in the byte for backskip values, leaving 2^7 possible offsets.
> 61:   // Mininum object alignment is 8 bytes (2^3), so we can at most represent 2^10 offsets within a BOT value.

s/Mininum/Minimum/

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

> 60:   // we need an extra bit for possible offsets in the byte for backskip values, leaving 2^7 possible offsets.
> 61:   // Mininum object alignment is 8 bytes (2^3), so we can at most represent 2^10 offsets within a BOT value.
> 62:   static const uint _max_block_size = 1024;

Static consts should be CamelCased.
Suggestion:

  static const uint MaxBlockSize = 1024;

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

> 28: #include "gc/shared/space.inline.hpp"
> 29: #include "gc/shared/gcLogPrecious.hpp"
> 30: #include "gc/parallel/objectStartArray.hpp"

This include is in the wrong order and should probably be guarded by `#if INCLUDE_PARALLELGC`.

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

> 33: #include "runtime/java.hpp"
> 34: #include "runtime/os.hpp"
> 35: #include "runtime/globals_extension.hpp"

I think we typically include the globals include that contains the flag - this should probably be `#include "gc/shared/gc_globals.hpp` (and sorted correctly in this list).

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

> 45:          "Initialize card size should only be called by card based collectors.");
> 46: 
> 47:   // Card size is the max. of minimum permissible value and GCCardSizeInBytes

Outdated comment. Remove.

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

> 455: uintx CardTable::ct_max_alignment_constraint() {
> 456:   // CardTable max alignment is computed with _card_size_max
> 457:   return _card_size_max * os::vm_page_size();

If the constraint were `AtParse`, the code could directly use `GCCardSizeInBytes`. Without needing to increase the minimum heap size.

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

> 234:   static uint card_size_in_words;
> 235: 
> 236:   // max permissible card size

s/max/Maximum

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:           constraint(GCCardSizeInBytesConstraintFunc, AfterErgo)            \

Suggestion:

          constraint(GCCardSizeInBytesConstraintFunc,AtParse)

Before `AfterErgo` there should be no space as in other constraints in this file. Also, since it is the last item in the macro, there is no need to have the `` at the very end.
If this constraint were made `AtParse` as suggested, then we can use the actual value of `GCCardSize` in the `ct_alignment_constraint()` function, not requiring a minimum heap size of 4MB (on x86), i.e. a doubling of minimum heap size.

src/hotspot/share/gc/shared/jvmFlagConstraintsGC.cpp line 429:

> 427: JVMFlag::Error GCCardSizeInBytesConstraintFunc(uint value, bool verbose) {
> 428:   if(!(UseG1GC || UseParallelGC || UseSerialGC))
> 429:     return JVMFlag::SUCCESS;

Please remove this check - if we are doing the constraint check at parse time, the `Use***GC` flags have not been set. It does not hurt to do the unnecessary check either even a GC does not support that option.

src/hotspot/share/gc/shared/jvmFlagConstraintsGC.cpp line 432:

> 430:   if (!is_power_of_2(value)) {
> 431:     JVMFlag::printError(verbose,
> 432:                         "GCCardSizeInBytes (" UINT32_FORMAT ") must be "

Please use `%u` instead of `UINT32_FORMAT`.

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

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