Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8272773: Configurable card table card size #5838

Closed
wants to merge 12 commits into from
Closed
@@ -45,6 +45,9 @@ static size_t calculate_heap_alignment(size_t space_alignment) {
}

void G1Arguments::initialize_alignments() {
// Initialize card size before initializing alignments
CardTable::initialize_card_size();

// Set up the region size and associated fields.
//
// There is a circular dependency here. We base the region size on the heap
@@ -1660,7 +1660,7 @@ jint G1CollectedHeap::initialize() {

// The G1FromCardCache reserves card with value 0 as "invalid", so the heap must not
// start within the first card.
guarantee(heap_rs.base() >= (char*)G1CardTable::card_size, "Java heap must not start within the first card.");
guarantee((uintptr_t)(heap_rs.base()) >= G1CardTable::card_size, "Java heap must not start within the first card.");
G1FromCardCache::initialize(max_reserved_regions());
// Also create a G1 rem set.
_rem_set = new G1RemSet(this, _card_table, _hot_card_cache);
@@ -31,11 +31,21 @@
#include "services/memTracker.hpp"
#include "utilities/align.hpp"

uint ObjectStartArray::block_shift = 0;
uint ObjectStartArray::block_size = 0;
uint ObjectStartArray::block_size_in_words = 0;

void ObjectStartArray::initialize_block_size(uint card_shift) {
block_shift = card_shift;
block_size = 1 << block_shift;
block_size_in_words = block_size / sizeof(HeapWord);
}

void ObjectStartArray::initialize(MemRegion reserved_region) {
// We're based on the assumption that we use the same
// size blocks as the card table.
assert((int)block_size == (int)CardTable::card_size, "Sanity");
assert((int)block_size <= 512, "block_size must be less than or equal to 512");
assert(block_size <= MaxBlockSize, "block_size must be less than or equal to " UINT32_FORMAT, MaxBlockSize);

// Calculate how much space must be reserved
_reserved_region = reserved_region;
@@ -52,11 +52,17 @@ class ObjectStartArray : public CHeapObj<mtGC> {
clean_block = -1
};

enum BlockSizeConstants {
block_shift = 9,
block_size = 1 << block_shift,
block_size_in_words = block_size / sizeof(HeapWord)
};
static uint block_shift;
static uint block_size;
static uint block_size_in_words;
Copy link
Contributor

@tschatzl tschatzl Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

@vish-chan vish-chan Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I've kept the names of all the pre-existing constants (which are now variables) same as before. Otherwise we have to change all the references also.

Copy link
Contributor

@tschatzl tschatzl Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see what the second reviewer thinks. Otherwise this can be done separately as it might hide the interesting part of this change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a review (yet), just a response to these questions.

Public data members are very rare in HotSpot code. There are a few POD/standard layout classes (examples include G1BufferNodeList and OopStorage::BasicParState::IterationData), but otherwise accessor functions are preferred. The style-guide says
"Use public accessor functions for member variables accessed outside the class."

The style-guide says this:
"Class data member names have a leading underscore, and use lowercase with words separated by a single underscore (_foo_bar)."
It makes no distinction for public or not, or for static or not. That is intentional, based on some discussion about that question when I was updating the style-guide a while ago.

Of course, there are probably counter-examples to both of these lurking in the code base.

Copy link
Contributor

@tschatzl tschatzl Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if this introduction of getters were moved to an extra CR, but I'm good to do that now as well; not so much a problem with the block_* ones, more with the ones in CardTable that add lots of noise.


// Maximum size an offset table entry can cover. This maximum is derived from that
// we need an extra bit for possible offsets in the byte for backskip values, leaving 2^7 possible offsets.
// Minimum object alignment is 8 bytes (2^3), so we can at most represent 2^10 offsets within a BOT value.
static const uint MaxBlockSize = 1024;

// Initialize block size based on card size
static void initialize_block_size(uint card_shift);

protected:

@@ -97,6 +97,8 @@ static size_t default_gen_alignment() {
}

void ParallelArguments::initialize_alignments() {
// Initialize card size before initializing alignments
CardTable::initialize_card_size();
SpaceAlignment = GenAlignment = default_gen_alignment();
HeapAlignment = compute_heap_alignment();
}
@@ -33,6 +33,18 @@
#include "runtime/java.hpp"
#include "services/memTracker.hpp"

uint BOTConstants::LogN = 0;
uint BOTConstants::LogN_words = 0;
uint BOTConstants::N_bytes = 0;
uint BOTConstants::N_words = 0;

void BOTConstants::initialize_bot_size(uint card_shift) {
LogN = card_shift;
LogN_words = LogN - LogHeapWordSize;
N_bytes = 1 << LogN;
N_words = 1 << LogN_words;
}

//////////////////////////////////////////////////////////////////////
// BlockOffsetSharedArray
//////////////////////////////////////////////////////////////////////
@@ -27,6 +27,7 @@

#include "gc/shared/gc_globals.hpp"
#include "gc/shared/memset_with_concurrent_readers.hpp"
#include "gc/shared/cardTable.hpp"
#include "memory/allocation.hpp"
#include "memory/memRegion.hpp"
#include "memory/virtualspace.hpp"
@@ -49,16 +50,20 @@ class ContiguousSpace;

class BOTConstants : public AllStatic {
public:
static const uint LogN = 9;
static const uint LogN_words = LogN - LogHeapWordSize;
static const uint N_bytes = 1 << LogN;
static const uint N_words = 1 << LogN_words;
static uint LogN;
static uint LogN_words;
static uint N_bytes;
static uint N_words;

// entries "e" of at least N_words mean "go back by Base^(e-N_words)."
// All entries are less than "N_words + N_powers".
static const uint LogBase = 4;
static const uint Base = (1 << LogBase);
static const uint N_powers = 14;

// Initialize bot size based on card size
static void initialize_bot_size(uint card_shift);

static size_t power_to_cards_back(uint i) {
return (size_t)1 << (LogBase * i);
}
@@ -93,6 +98,7 @@ class BlockOffsetTable {
BlockOffsetTable(HeapWord* bottom, HeapWord* end):
_bottom(bottom), _end(end) {
assert(_bottom <= _end, "arguments out of order");
assert(BOTConstants::N_bytes == CardTable::card_size, "sanity");
}

// Note that the committed size of the covered space may have changed,
@@ -25,13 +25,41 @@
#include "precompiled.hpp"
#include "gc/shared/cardTable.hpp"
#include "gc/shared/collectedHeap.hpp"
#include "gc/shared/gcLogPrecious.hpp"
#include "gc/shared/gc_globals.hpp"
#include "gc/shared/space.inline.hpp"
#include "logging/log.hpp"
#include "memory/virtualspace.hpp"
#include "runtime/java.hpp"
#include "runtime/os.hpp"
#include "services/memTracker.hpp"
#include "utilities/align.hpp"
#if INCLUDE_PARALLELGC
#include "gc/parallel/objectStartArray.hpp"
#endif

uint CardTable::card_shift = 0;
uint CardTable::card_size = 0;
uint CardTable::card_size_in_words = 0;

void CardTable::initialize_card_size() {
assert(UseG1GC || UseParallelGC || UseSerialGC,
"Initialize card size should only be called by card based collectors.");

card_size = GCCardSizeInBytes;
vish-chan marked this conversation as resolved.
Show resolved Hide resolved
card_shift = log2i_exact(card_size);
card_size_in_words = card_size / sizeof(HeapWord);

// Set blockOffsetTable size based on card table entry size
BOTConstants::initialize_bot_size(card_shift);

#if INCLUDE_PARALLELGC
// Set ObjectStartArray block size based on card table entry size
ObjectStartArray::initialize_block_size(card_shift);
#endif

log_info_p(gc, init)("CardTable entry size: " UINT32_FORMAT, card_size);
}

size_t CardTable::compute_byte_map_size() {
assert(_guard_index == cards_required(_whole_heap.word_size()) - 1,
@@ -56,8 +84,6 @@ CardTable::CardTable(MemRegion whole_heap) :
{
assert((uintptr_t(_whole_heap.start()) & (card_size - 1)) == 0, "heap must start at card boundary");
assert((uintptr_t(_whole_heap.end()) & (card_size - 1)) == 0, "heap must end at card boundary");

assert(card_size <= 512, "card_size must be less than 512"); // why?
}

CardTable::~CardTable() {
@@ -428,7 +454,8 @@ MemRegion CardTable::dirty_card_range_after_reset(MemRegion mr,
}

uintx CardTable::ct_max_alignment_constraint() {
return card_size * os::vm_page_size();
// Calculate maximum alignment using GCCardSizeInBytes as card_size hasn't been set yet
return GCCardSizeInBytes * os::vm_page_size();
Copy link
Member

@albertnetymk albertnetymk Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why card_size can't be used here is that card_size haven't been set when this method is called, correct? If so, please put a comment here.

}

void CardTable::verify_guard() {
@@ -228,17 +228,18 @@ class CardTable: public CHeapObj<mtGC> {
MemRegion dirty_card_range_after_reset(MemRegion mr, bool reset,
int reset_val);

// Constants
enum SomePublicConstants {
card_shift = 9,
card_size = 1 << card_shift,
card_size_in_words = card_size / sizeof(HeapWord)
};
// CardTable entry size
static uint card_shift;
static uint card_size;
static uint card_size_in_words;

static constexpr CardValue clean_card_val() { return clean_card; }
static constexpr CardValue dirty_card_val() { return dirty_card; }
static intptr_t clean_card_row_val() { return clean_card_row; }

// Initialize card size
static void initialize_card_size();

// Card marking array base (adjusted for heap low boundary)
// This would be the 0th element of _byte_map, if the heap started at 0x0.
// But since the heap starts at some higher address, this points to somewhere
@@ -692,9 +692,13 @@
product(uintx, GCDrainStackTargetSize, 64, \
"Number of entries we will try to leave on the stack " \
"during parallel gc") \
range(0, max_juint)

// end of GC_FLAGS
range(0, max_juint) \
\
product(uint, GCCardSizeInBytes, 512, \
"Card table entry size (in bytes) for card based collectors") \
range(128, 1024) \
constraint(GCCardSizeInBytesConstraintFunc,AtParse)
// end of GC_FLAGS
Copy link
Contributor

@tschatzl tschatzl Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


DECLARE_FLAGS(GC_FLAGS)

@@ -23,6 +23,7 @@
*/

#include "precompiled.hpp"
#include "gc/shared/cardTable.hpp"
#include "gc/shared/genArguments.hpp"
#include "gc/shared/generation.hpp"
#include "logging/log.hpp"
@@ -61,6 +62,8 @@ static size_t bound_minus_alignment(size_t desired_size,
}

void GenArguments::initialize_alignments() {
// Initialize card size before initializing alignments
CardTable::initialize_card_size();
SpaceAlignment = GenAlignment = (size_t)Generation::GenGrain;
HeapAlignment = compute_heap_alignment();
}
@@ -424,3 +424,15 @@ JVMFlag::Error MaxMetaspaceSizeConstraintFunc(size_t value, bool verbose) {
}
}

JVMFlag::Error GCCardSizeInBytesConstraintFunc(uint value, bool verbose) {
if (!is_power_of_2(value)) {
JVMFlag::printError(verbose,
"GCCardSizeInBytes ( %u ) must be "
"a power of 2\n",
value);
return JVMFlag::VIOLATES_CONSTRAINT;
} else {
return JVMFlag::SUCCESS;
}
}

@@ -66,7 +66,8 @@
f(uintx, TLABWasteIncrementConstraintFunc) \
f(uintx, SurvivorRatioConstraintFunc) \
f(size_t, MetaspaceSizeConstraintFunc) \
f(size_t, MaxMetaspaceSizeConstraintFunc)
f(size_t, MaxMetaspaceSizeConstraintFunc) \
f(uint, GCCardSizeInBytesConstraintFunc)

SHARED_GC_CONSTRAINTS(DECLARE_CONSTRAINT)