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

8276098: Do precise BOT updates in G1 evacuation phase #6166

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/hotspot/share/gc/g1/g1AllocRegion.cpp
Expand Up @@ -43,16 +43,19 @@ void G1AllocRegion::setup(G1CollectedHeap* g1h, HeapRegion* dummy_region) {

// Make sure that any allocation attempt on this region will fail
// and will not trigger any asserts.
assert(dummy_region->allocate_no_bot_updates(1) == NULL, "should fail");
assert(dummy_region->allocate(1) == NULL, "should fail");
DEBUG_ONLY(size_t assert_tmp);
assert(dummy_region->par_allocate_no_bot_updates(1, 1, &assert_tmp) == NULL, "should fail");
assert(dummy_region->par_allocate(1, 1, &assert_tmp) == NULL, "should fail");

_g1h = g1h;
_dummy_region = dummy_region;
}

void G1AllocRegion::update_bot_for_region_waste(HeapWord* addr, size_t size) {
assert(_alloc_region != NULL, "invariant");
assert(_bot_updates, "must only be called for regions doing BOT updates");
_alloc_region->update_bot_at(addr, size);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why do we need to update for the filler objects? Because we are not scanning them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. You are correct in that we never need to scan these parts because of dirty cards, but during remembered set rebuilding (see: G1RebuildRemSetHeapRegionClosure::rebuild_rem_set_in_region(...)) we include the whole region when looking for references to other heap regions.

There might be some good way to avoid scanning those parts during rebuild, but such investigation is out of scope for this PR.

Thanks for reviewing 😄

size_t G1AllocRegion::fill_up_remaining_space(HeapRegion* alloc_region) {
assert(alloc_region != NULL && alloc_region != _dummy_region,
"pre-condition");
Expand Down Expand Up @@ -81,6 +84,10 @@ size_t G1AllocRegion::fill_up_remaining_space(HeapRegion* alloc_region) {
CollectedHeap::fill_with_object(dummy, free_word_size);
alloc_region->set_pre_dummy_top(dummy);
result += free_word_size * HeapWordSize;
// Update BOT if this is an old region requiring BOT updates.
if (_bot_updates) {
update_bot_for_region_waste(dummy, free_word_size);
}
break;
}

Expand Down Expand Up @@ -390,6 +397,7 @@ HeapRegion* OldGCAllocRegion::release() {
if (to_allocate_words >= G1CollectedHeap::min_fill_size()) {
HeapWord* dummy = attempt_allocation(to_allocate_words);
CollectedHeap::fill_with_object(dummy, to_allocate_words);
update_bot_for_region_waste(dummy, to_allocate_words);
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/gc/g1/g1AllocRegion.hpp
Expand Up @@ -126,6 +126,9 @@ class G1AllocRegion : public CHeapObj<mtGC> {
// Returns the number of bytes that have been filled up during retire.
virtual size_t retire(bool fill_up);

// Update the BOT for the filler object allocated at the given address.
void update_bot_for_region_waste(HeapWord* addr, size_t size);

size_t retire_internal(HeapRegion* alloc_region, bool fill_up);

// For convenience as subclasses use it.
Expand Down
12 changes: 2 additions & 10 deletions src/hotspot/share/gc/g1/g1AllocRegion.inline.hpp
Expand Up @@ -45,11 +45,7 @@ inline HeapWord* G1AllocRegion::allocate(HeapRegion* alloc_region,
size_t word_size) {
assert(alloc_region != NULL, "pre-condition");

if (!_bot_updates) {
return alloc_region->allocate_no_bot_updates(word_size);
} else {
return alloc_region->allocate(word_size);
}
return alloc_region->allocate_no_bot_updates(word_size);
}

inline HeapWord* G1AllocRegion::par_allocate(HeapRegion* alloc_region, size_t word_size) {
Expand All @@ -64,11 +60,7 @@ inline HeapWord* G1AllocRegion::par_allocate(HeapRegion* alloc_region,
assert(alloc_region != NULL, "pre-condition");
assert(!alloc_region->is_empty(), "pre-condition");

if (!_bot_updates) {
return alloc_region->par_allocate_no_bot_updates(min_word_size, desired_word_size, actual_word_size);
} else {
return alloc_region->par_allocate(min_word_size, desired_word_size, actual_word_size);
}
return alloc_region->par_allocate_no_bot_updates(min_word_size, desired_word_size, actual_word_size);
}

inline HeapWord* G1AllocRegion::attempt_allocation(size_t word_size) {
Expand Down
29 changes: 24 additions & 5 deletions src/hotspot/share/gc/g1/g1Allocator.cpp
Expand Up @@ -292,15 +292,30 @@ HeapWord* G1Allocator::old_attempt_allocation(size_t min_word_size,
return result;
}

G1PLAB::G1PLAB(size_t word_sz) : PLAB(word_sz) { }

bool G1PLAB::is_allocated() {
return _top != nullptr;
}
HeapWord* G1PLAB::get_filler() {
return _top;
}

size_t G1PLAB::get_filler_size() {
return pointer_delta(_hard_end, _top);
}

G1PLABAllocator::G1PLABAllocator(G1Allocator* allocator) :
_g1h(G1CollectedHeap::heap()),
_allocator(allocator) {
_allocator(allocator),
_bot_plab_region(nullptr),
_bot_plab_threshold(nullptr) {
for (region_type_t state = 0; state < G1HeapRegionAttr::Num; state++) {
_direct_allocated[state] = 0;
uint length = alloc_buffers_length(state);
_alloc_buffers[state] = NEW_C_HEAP_ARRAY(PLAB*, length, mtGC);
_alloc_buffers[state] = NEW_C_HEAP_ARRAY(G1PLAB*, length, mtGC);
for (uint node_index = 0; node_index < length; node_index++) {
_alloc_buffers[state][node_index] = new PLAB(_g1h->desired_plab_sz(state));
_alloc_buffers[state][node_index] = new G1PLAB(_g1h->desired_plab_sz(state));
}
}
}
Expand Down Expand Up @@ -331,7 +346,8 @@ HeapWord* G1PLABAllocator::allocate_direct_or_new_plab(G1HeapRegionAttr dest,
if ((required_in_plab <= plab_word_size) &&
may_throw_away_buffer(required_in_plab, plab_word_size)) {

PLAB* alloc_buf = alloc_buffer(dest, node_index);
G1PLAB* alloc_buf = alloc_buffer(dest, node_index);
update_bot_for_plab_waste(dest, alloc_buf);
alloc_buf->retire();

size_t actual_plab_size = 0;
Expand All @@ -346,6 +362,7 @@ HeapWord* G1PLABAllocator::allocate_direct_or_new_plab(G1HeapRegionAttr dest,
required_in_plab, plab_word_size, actual_plab_size, p2i(buf));

if (buf != NULL) {
calculate_new_bot_threshold(dest, buf);
alloc_buf->set_buf(buf, actual_plab_size);

HeapWord* const obj = alloc_buf->allocate(word_sz);
Expand All @@ -360,6 +377,7 @@ HeapWord* G1PLABAllocator::allocate_direct_or_new_plab(G1HeapRegionAttr dest,
// Try direct allocation.
HeapWord* result = _allocator->par_allocate_during_gc(dest, word_sz, node_index);
if (result != NULL) {
update_bot_for_direct_allocation(dest, result, word_sz);
_direct_allocated[dest.type()] += word_sz;
}
return result;
Expand All @@ -373,8 +391,9 @@ void G1PLABAllocator::flush_and_retire_stats() {
for (region_type_t state = 0; state < G1HeapRegionAttr::Num; state++) {
G1EvacStats* stats = _g1h->alloc_buffer_stats(state);
for (uint node_index = 0; node_index < alloc_buffers_length(state); node_index++) {
PLAB* const buf = alloc_buffer(state, node_index);
G1PLAB* const buf = alloc_buffer(state, node_index);
if (buf != NULL) {
update_bot_for_plab_waste(state, buf);
buf->flush_and_retire_stats(stats);
}
}
Expand Down
32 changes: 29 additions & 3 deletions src/hotspot/share/gc/g1/g1Allocator.hpp
Expand Up @@ -145,6 +145,16 @@ class G1Allocator : public CHeapObj<mtGC> {
uint node_index);
};

// Helper class to get the information needed to do
// BOT updates for the end of the PLAB.
class G1PLAB : public PLAB {
public:
G1PLAB(size_t word_sz);
bool is_allocated();
HeapWord* get_filler();
size_t get_filler_size();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of two methods that return the remaining space, one that returns a MemRegion would be nicer? Also call it something like remainder or so.

What about making PLAB::retire_internal virtual and override here, so that the explicit call in G1PLABAllocator::allocate_direct_or_new_plab goes away? (and these two helpers, and probably also is_allocated()).

Also the explict call in G1PLABAllocator::flush_and_retire_stats could maybe be hidden too. You could store the G1PLABAllocator in G1PLAB so that it can call the update_bot.... method in retire_internal.

};

// Manages the PLABs used during garbage collection. Interface for allocation from PLABs.
// Needs to handle multiple contexts, extra alignment in any "survivor" area and some
// statistics.
Expand All @@ -156,14 +166,27 @@ class G1PLABAllocator : public CHeapObj<mtGC> {
G1CollectedHeap* _g1h;
G1Allocator* _allocator;

PLAB** _alloc_buffers[G1HeapRegionAttr::Num];
// Region where the current old generation PLAB is allocated. Used to do BOT updates.
HeapRegion* _bot_plab_region;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is a breakage of abstraction if we only store this information for old gen - we after all allocate the G1PLAB for all G1HeapRegionAttr::num destination areas.
What about putting all that stuff into G1PLAB and initializing it appropriately?

// Current BOT threshold, a PLAB allocation crossing this threshold will cause a BOT
// update.
HeapWord* _bot_plab_threshold;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also only interesting for the old generation, isn't it? Same issue as above.


G1PLAB** _alloc_buffers[G1HeapRegionAttr::Num];

// Number of words allocated directly (not counting PLAB allocation).
size_t _direct_allocated[G1HeapRegionAttr::Num];

void flush_and_retire_stats();
inline PLAB* alloc_buffer(G1HeapRegionAttr dest, uint node_index) const;
inline PLAB* alloc_buffer(region_type_t dest, uint node_index) const;
inline G1PLAB* alloc_buffer(G1HeapRegionAttr dest, uint node_index) const;
inline G1PLAB* alloc_buffer(region_type_t dest, uint node_index) const;

// Helpers to do explicit BOT updates for allocations in old generation regions.
void update_bot_for_direct_allocation(G1HeapRegionAttr attr, HeapWord* addr, size_t size);
void update_bot_for_plab_waste(G1HeapRegionAttr dest, G1PLAB* plab);
// When a new PLAB is allocated a new threshold needs to be calculated and
// possibly also the current region where BOT updates should be done.
void calculate_new_bot_threshold(G1HeapRegionAttr attr, HeapWord* addr);

// Returns the number of allocation buffers for the given dest.
// There is only 1 buffer for Old while Young may have multiple buffers depending on
Expand Down Expand Up @@ -199,6 +222,9 @@ class G1PLABAllocator : public CHeapObj<mtGC> {
uint node_index);

void undo_allocation(G1HeapRegionAttr dest, HeapWord* obj, size_t word_sz, uint node_index);

// Update the BOT for an allocation inside an old PLAB.
void update_bot_for_object(HeapWord* obj_start, size_t obj_size);
};

// G1ArchiveAllocator is used to allocate memory in archive
Expand Down
60 changes: 58 additions & 2 deletions src/hotspot/share/gc/g1/g1Allocator.inline.hpp
Expand Up @@ -86,15 +86,15 @@ inline HeapWord* G1Allocator::attempt_allocation_force(size_t word_size) {
return mutator_alloc_region(node_index)->attempt_allocation_force(word_size);
}

inline PLAB* G1PLABAllocator::alloc_buffer(G1HeapRegionAttr dest, uint node_index) const {
inline G1PLAB* G1PLABAllocator::alloc_buffer(G1HeapRegionAttr dest, uint node_index) const {
assert(dest.is_valid(),
"Allocation buffer index out of bounds: %s", dest.get_type_str());
assert(_alloc_buffers[dest.type()] != NULL,
"Allocation buffer is NULL: %s", dest.get_type_str());
return alloc_buffer(dest.type(), node_index);
}

inline PLAB* G1PLABAllocator::alloc_buffer(region_type_t dest, uint node_index) const {
inline G1PLAB* G1PLABAllocator::alloc_buffer(region_type_t dest, uint node_index) const {
assert(dest < G1HeapRegionAttr::Num,
"Allocation buffer index out of bounds: %u", dest);

Expand Down Expand Up @@ -133,4 +133,60 @@ inline HeapWord* G1PLABAllocator::allocate(G1HeapRegionAttr dest,
return allocate_direct_or_new_plab(dest, word_sz, refill_failed, node_index);
}

inline void G1PLABAllocator::update_bot_for_plab_waste(G1HeapRegionAttr attr, G1PLAB* plab) {
if (!attr.is_old()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to make an extra predicate like needs_bot_update() for this instead of explictly replicating the code in a few places (and always adding the comment).

// BOT updates are only done for old generation.
return;
}

if (!plab->is_allocated()) {
return;
}
update_bot_for_object(plab->get_filler(), plab->get_filler_size());
}

inline void G1PLABAllocator::calculate_new_bot_threshold(G1HeapRegionAttr attr, HeapWord* addr) {
if (!attr.is_old()) {
// BOT updates are only done for old generation.
return;
}

_bot_plab_region = _g1h->heap_region_containing(addr);
_bot_plab_threshold = _bot_plab_region->bot_threshold_for_addr(addr);

assert(_bot_plab_threshold >= addr,
"threshold must be at or after PLAB start. " PTR_FORMAT " >= " PTR_FORMAT,
p2i(_bot_plab_threshold), p2i(addr));
assert(_bot_plab_region->is_old(),
"Updating BOT threshold for non-old region. addr: " PTR_FORMAT " region:" HR_FORMAT,
p2i(addr), HR_FORMAT_PARAMS(_bot_plab_region));
}

inline void G1PLABAllocator::update_bot_for_direct_allocation(G1HeapRegionAttr attr, HeapWord* addr, size_t size) {
if (!attr.is_old()) {
// BOT updates are only done for old generation.
return;
}

// Out of PLAB allocations in an old generation region. Update BOT.
HeapRegion* region = _g1h->heap_region_containing(addr);
region->update_bot_at(addr, size);
}

inline void G1PLABAllocator::update_bot_for_object(HeapWord* obj_start, size_t obj_size) {
HeapWord* obj_end = obj_start + obj_size;
if (obj_end <= _bot_plab_threshold) {
// Not crossing the threshold.
return;
}

if (!alloc_buffer(G1HeapRegionAttr::Old, 0)->contains(obj_start)) {
// Out of PLAB allocation, BOT already updated.
return;
}

// Update the BOT. The threshold also gets updated to the next threshold by this call.
_bot_plab_region->update_bot_crossing_threshold(&_bot_plab_threshold, obj_start, obj_end);
}

#endif // SHARE_GC_G1_G1ALLOCATOR_INLINE_HPP
9 changes: 9 additions & 0 deletions src/hotspot/share/gc/g1/g1BlockOffsetTable.hpp
Expand Up @@ -108,6 +108,7 @@ class G1BlockOffsetTable: public CHeapObj<mtGC> {

class G1BlockOffsetTablePart {
friend class G1BlockOffsetTable;
friend class HeapRegion;
friend class VMStructs;
private:
// allocation boundary at which offset array must be updated
Expand Down Expand Up @@ -181,6 +182,9 @@ class G1BlockOffsetTablePart {

void verify() const;

// Given an address calculate where the next threshold needing an update is.
inline HeapWord* threshold_for_addr(const void* addr);

// Returns the address of the start of the block containing "addr", or
// else "null" if it is covered by no block. (May have side effects,
// namely updating of shared array entries that "point" too far
Expand All @@ -204,6 +208,11 @@ class G1BlockOffsetTablePart {
// updated.
HeapWord* threshold() const { return _next_offset_threshold; }

// Sets the threshold explicitly to keep it consistent with what has been
// updated. This needs to be done when the threshold is not used for updating
// the bot, for example when promoting to old in young collections.
void set_threshold(HeapWord* threshold) { _next_offset_threshold = threshold; }

// These must be guaranteed to work properly (i.e., do nothing)
// when "blk_start" ("blk" for second version) is "NULL". In this
// implementation, that's true because NULL is represented as 0, and thus
Expand Down
17 changes: 14 additions & 3 deletions src/hotspot/share/gc/g1/g1BlockOffsetTable.inline.hpp
Expand Up @@ -31,6 +31,20 @@
#include "gc/shared/memset_with_concurrent_readers.hpp"
#include "runtime/atomic.hpp"

inline HeapWord* G1BlockOffsetTablePart::threshold_for_addr(const void* addr) {
assert(addr >= _hr->bottom() && addr < _hr->top(), "invalid address");
size_t index = _bot->index_for(addr);
HeapWord* card_boundary = _bot->address_for_index(index);
// Address at card boundary, use as threshold.
if (card_boundary == addr) {
return card_boundary;
}

// Calculate next threshold.
HeapWord* threshold = card_boundary + BOTConstants::N_words;
return threshold;
}

inline HeapWord* G1BlockOffsetTablePart::block_start(const void* addr) {
assert(addr >= _hr->bottom() && addr < _hr->top(), "invalid address");
HeapWord* q = block_at_or_preceding(addr);
Expand Down Expand Up @@ -109,9 +123,6 @@ inline HeapWord* G1BlockOffsetTablePart::block_at_or_preceding(const void* addr)
"Object crossed region boundary, found offset %u instead of 0",
(uint) _bot->offset_array(_bot->index_for(_hr->bottom())));

// We must make sure that the offset table entry we use is valid.
assert(addr < _next_offset_threshold, "Precondition");

size_t index = _bot->index_for(addr);

HeapWord* q = _bot->address_for_index(index);
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/gc/g1/g1CollectedHeap.cpp
Expand Up @@ -3293,6 +3293,7 @@ void G1CollectedHeap::retire_gc_alloc_region(HeapRegion* alloc_region,
_bytes_used_during_gc += allocated_bytes;
if (dest.is_old()) {
old_set_add(alloc_region);
alloc_region->update_bot_threshold();
} else {
assert(dest.is_young(), "Retiring alloc region should be young (%d)", dest.type());
_survivor.add_used_bytes(allocated_bytes);
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
Expand Up @@ -498,6 +498,9 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio
obj->incr_age();
}
_age_table.add(age, word_sz);
} else {
assert(dest_attr.is_old(), "Only update bot for allocations in old");
_plab_allocator->update_bot_for_object(obj_ptr, word_sz);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be good if the PLABAllocator returned a struct instead of just a pointer that contains

  • the HeapWord
  • word_sz
  • whether it was a direct allocation
    ?

Then this struct could be passed in here again instead of the code for update_bot_for_object trying to reconstruct whether it has been an out-of-plab allocation or not.

Or just skip the word_sz in that struct. Alternatively add a return value whether this allocation has been out-of-plab. But this method already has lots of locals (is too long?), so starting to group them might be a good idea.

I think explicitly carrying this information around would be much much cleaner to understand than trying to reconstruct that information later in `update_bot_for_object´ via

  if (!alloc_buffer(G1HeapRegionAttr::Old, 0)->contains(obj_start)) {
    // Out of PLAB allocation, BOT already updated.
    return;
  }

}

// Most objects are not arrays, so do one array check rather than
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/gc/g1/heapRegion.cpp
Expand Up @@ -233,7 +233,6 @@ HeapRegion::HeapRegion(uint hrm_index,
_top(NULL),
_compaction_top(NULL),
_bot_part(bot, this),
_par_alloc_lock(Mutex::service-2, "HeapRegionParAlloc_lock"),
_pre_dummy_top(NULL),
_rem_set(NULL),
_hrm_index(hrm_index),
Expand Down