Skip to content
Permalink
Browse files
8267073: Race between Card Redirtying and Freeing Collection Set regi…
…ons results in missing remembered set entries with G1

Reviewed-by: kbarrett, sjohanss
  • Loading branch information
Thomas Schatzl committed Jun 10, 2021
1 parent 7cd5a6e commit 2b41459e95e8d6c4ea4c25e8f1d851907d65ef73
Showing 9 changed files with 39 additions and 45 deletions.
@@ -1477,6 +1477,7 @@ G1CollectedHeap::G1CollectedHeap() :
_cr(NULL),
_task_queues(NULL),
_num_regions_failed_evacuation(0),
_regions_failed_evacuation(NULL),
_evacuation_failed_info_array(NULL),
_preserved_marks_set(true /* in_c_heap */),
#ifndef PRODUCT
@@ -1769,6 +1770,8 @@ jint G1CollectedHeap::initialize() {

_collection_set.initialize(max_reserved_regions());

_regions_failed_evacuation = NEW_C_HEAP_ARRAY(volatile bool, max_regions(), mtGC);

G1InitLogger::print();

return JNI_OK;
@@ -3467,6 +3470,8 @@ void G1CollectedHeap::pre_evacuate_collection_set(G1EvacuationInfo& evacuation_i
_expand_heap_after_alloc_failure = true;
Atomic::store(&_num_regions_failed_evacuation, 0u);

memset((void*)_regions_failed_evacuation, false, sizeof(bool) * max_regions());

// Disable the hot card cache.
_hot_card_cache->reset_hot_cache_claimed_index();
_hot_card_cache->set_use_cache(false);
@@ -867,6 +867,8 @@ class G1CollectedHeap : public CollectedHeap {

// Number of regions evacuation failed in the current collection.
volatile uint _num_regions_failed_evacuation;
// Records for every region on the heap whether evacuation failed for it.
volatile bool* _regions_failed_evacuation;

EvacuationFailedInfo* _evacuation_failed_info_array;

@@ -1145,10 +1147,15 @@ class G1CollectedHeap : public CollectedHeap {

// True iff an evacuation has failed in the most-recent collection.
inline bool evacuation_failed() const;
// True iff the given region encountered an evacuation failure in the most-recent
// collection.
inline bool evacuation_failed(uint region_idx) const;

inline uint num_regions_failed_evacuation() const;
// Notify that the garbage collection encountered an evacuation failure in a
// region. Should only be called once per region.
inline void notify_region_failed_evacuation();
// Notify that the garbage collection encountered an evacuation failure in the
// given region. Returns whether this has been the first occurrence of an evacuation
// failure in that region.
inline bool notify_region_failed_evacuation(uint const region_idx);

void remove_from_old_gen_sets(const uint old_regions_removed,
const uint archive_regions_removed,
@@ -194,12 +194,25 @@ bool G1CollectedHeap::evacuation_failed() const {
return num_regions_failed_evacuation() > 0;
}

bool G1CollectedHeap::evacuation_failed(uint region_idx) const {
assert(region_idx < max_regions(), "Invalid region index %u", region_idx);

return Atomic::load(&_regions_failed_evacuation[region_idx]);
}

uint G1CollectedHeap::num_regions_failed_evacuation() const {
return Atomic::load(&_num_regions_failed_evacuation);
}

void G1CollectedHeap::notify_region_failed_evacuation() {
Atomic::inc(&_num_regions_failed_evacuation, memory_order_relaxed);
bool G1CollectedHeap::notify_region_failed_evacuation(uint const region_idx) {
assert(region_idx < max_regions(), "Invalid region index %u", region_idx);

volatile bool* region_failed_addr = &_regions_failed_evacuation[region_idx];
bool result = !Atomic::load(region_failed_addr) && !Atomic::cmpxchg(region_failed_addr, false, true, memory_order_relaxed);
if (result) {
Atomic::inc(&_num_regions_failed_evacuation, memory_order_relaxed);
}
return result;
}

#ifndef PRODUCT
@@ -237,7 +237,7 @@ class RemoveSelfForwardPtrHRClosure: public HeapRegionClosure {
assert(!hr->is_pinned(), "Unexpected pinned region at index %u", hr->hrm_index());
assert(hr->in_collection_set(), "bad CS");

if (hr->evacuation_failed()) {
if (_g1h->evacuation_failed(hr->hrm_index())) {
hr->clear_index_in_opt_cset();

bool during_concurrent_start = _g1h->collector_state()->in_concurrent_start_gc();
@@ -605,8 +605,7 @@ oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m) {
// Forward-to-self succeeded. We are the "owner" of the object.
HeapRegion* r = _g1h->heap_region_containing(old);

if (r->set_evacuation_failed()) {
_g1h->notify_region_failed_evacuation();
if (_g1h->notify_region_failed_evacuation(r->hrm_index())) {
_g1h->hr_printer()->evac_failure(r);
}

@@ -265,9 +265,9 @@ class RedirtyLoggedCardTableEntryClosure : public G1CardTableEntryClosure {
}

bool will_become_free(HeapRegion* hr) const {
// A region will be freed by free_collection_set if the region is in the
// A region will be freed by during the FreeCollectionSet phase if the region is in the
// collection set and has not had an evacuation failure.
return _g1h->is_in_cset(hr) && !hr->evacuation_failed();
return _g1h->is_in_cset(hr) && !_g1h->evacuation_failed(hr->hrm_index());
}

public:
@@ -379,8 +379,8 @@ class FreeCSetStats {
}

void account_evacuated_region(HeapRegion* r) {
size_t used = r->used();
assert(used > 0, "region %u %s zero used", r->hrm_index(), r->get_short_type_str());
size_t used = r->used();
assert(used > 0, "region %u %s zero used", r->hrm_index(), r->get_short_type_str());
_before_used_bytes += used;
_regions_freed += 1;
}
@@ -431,7 +431,7 @@ class FreeCSetClosure : public HeapRegionClosure {
Tickspan _non_young_time;
FreeCSetStats* _stats;

void assert_in_cset(HeapRegion* r) {
void assert_tracks_surviving_words(HeapRegion* r) {
assert(r->young_index_in_cset() != 0 &&
(uint)r->young_index_in_cset() <= _g1h->collection_set()->young_region_length(),
"Young index %u is wrong for region %u of type %s with %u young regions",
@@ -486,15 +486,14 @@ class FreeCSetClosure : public HeapRegionClosure {
JFREventForRegion event(r, _worker_id);
TimerForRegion timer(timer_for_region(r));

_g1h->clear_region_attr(r);
stats()->account_rs_length(r);

if (r->is_young()) {
assert_in_cset(r);
assert_tracks_surviving_words(r);
r->record_surv_words_in_group(_surviving_young_words[r->young_index_in_cset()]);
}

if (r->evacuation_failed()) {
if (_g1h->evacuation_failed(r->hrm_index())) {
handle_failed_region(r);
} else {
handle_evacuated_region(r);
@@ -104,7 +104,6 @@ void HeapRegion::setup_heap_region_size(size_t max_heap_size) {
void HeapRegion::handle_evacuation_failure() {
uninstall_surv_rate_group();
clear_young_index_in_cset();
reset_evacuation_failed();
set_old();
_next_marked_bytes = 0;
}
@@ -118,8 +117,6 @@ void HeapRegion::unlink_from_list() {
void HeapRegion::hr_clear(bool clear_space) {
assert(_humongous_start_region == NULL,
"we should have already filtered out humongous regions");
assert(!in_collection_set(),
"Should not clear heap region %u in the collection set", hrm_index());

clear_young_index_in_cset();
clear_index_in_opt_cset();
@@ -134,7 +131,6 @@ void HeapRegion::hr_clear(bool clear_space) {
init_top_at_mark_start();
if (clear_space) clear(SpaceDecorator::Mangle);

Atomic::store(&_evacuation_failed, false);
_gc_efficiency = -1.0;
}

@@ -242,7 +238,6 @@ HeapRegion::HeapRegion(uint hrm_index,
_hrm_index(hrm_index),
_type(),
_humongous_start_region(NULL),
_evacuation_failed(false),
_index_in_opt_cset(InvalidCSetIndex),
_next(NULL), _prev(NULL),
#ifdef ASSERT
@@ -210,9 +210,6 @@ class HeapRegion : public CHeapObj<mtGC> {
// For a humongous region, region in which it starts.
HeapRegion* _humongous_start_region;

// True iff an attempt to evacuate an object in the region failed.
volatile bool _evacuation_failed;

static const uint InvalidCSetIndex = UINT_MAX;

// The index in the optional regions array, if this region
@@ -498,15 +495,6 @@ class HeapRegion : public CHeapObj<mtGC> {
// Clear the card table corresponding to this region.
void clear_cardtable();

// Returns the "evacuation_failed" property of the region.
inline bool evacuation_failed() const;

// Sets the "evacuation_failed" property of the region, returning true if this
// has been the first call, false otherwise.
inline bool set_evacuation_failed();

inline void reset_evacuation_failed();

// Notify the region that we are about to start processing
// self-forwarded objects during evac failure handling.
void note_self_forwarding_removal_start(bool during_concurrent_start,
@@ -452,16 +452,4 @@ inline void HeapRegion::record_surv_words_in_group(size_t words_survived) {
_surv_rate_group->record_surviving_words(age_in_group, words_survived);
}

inline bool HeapRegion::evacuation_failed() const {
return Atomic::load(&_evacuation_failed);
}

inline bool HeapRegion::set_evacuation_failed() {
return !Atomic::load(&_evacuation_failed) && !Atomic::cmpxchg(&_evacuation_failed, false, true, memory_order_relaxed);
}

inline void HeapRegion::reset_evacuation_failed() {
Atomic::store(&_evacuation_failed, false);
}

#endif // SHARE_GC_G1_HEAPREGION_INLINE_HPP

0 comments on commit 2b41459

Please sign in to comment.