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
Changes from 10 commits
c9d5203
8306221
9247350
c9c8db8
43fa93e
9d02c66
475de50
1f0612a
125a0af
0d1e15d
6d7b084
a3551a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -90,6 +90,9 @@ void HeapRegion::setup_heap_region_size(size_t max_heap_size) { | ||
guarantee(GrainWords == 0, "we should only set it once"); | ||
GrainWords = GrainBytes >> LogHeapWordSize; | ||
|
||
// Initialize card size | ||
CardTable::initialize_card_size(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason for having |
||
|
||
guarantee(CardsPerRegion == 0, "we should only set it once"); | ||
CardsPerRegion = GrainBytes >> G1CardTable::card_shift; | ||
|
||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The style-guide says this: Of course, there are probably counter-examples to both of these lurking in the code base. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
// 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: | ||
|
||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -23,8 +23,13 @@ | ||
*/ | ||
|
||
#include "precompiled.hpp" | ||
#if INCLUDE_PARALLELGC | ||
#include "gc/parallel/objectStartArray.hpp" | ||
#endif | ||
#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" | ||
@@ -33,6 +38,29 @@ | ||
#include "services/memTracker.hpp" | ||
#include "utilities/align.hpp" | ||
|
||
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, | ||
"uninitialized, check declaration order"); | ||
@@ -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,7 @@ MemRegion CardTable::dirty_card_range_after_reset(MemRegion mr, | ||
} | ||
|
||
uintx CardTable::ct_max_alignment_constraint() { | ||
return card_size * os::vm_page_size(); | ||
return GCCardSizeInBytes * os::vm_page_size(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason why |
||
} | ||
|
||
void CardTable::verify_guard() { | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See e.g. |
||
|
||
DECLARE_FLAGS(GC_FLAGS) | ||
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
(uintptr_t)heap_rs.base() >= G1CardTable::card_size
? Converting an arbitrary integer to a pointer is implementation-defined.