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

8267073: Race between Card Redirtying and Freeing Collection Set regions results in missing remembered set entries with G1 #4429

Closed
Changes from all 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
@@ -1460,6 +1460,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
@@ -1752,6 +1753,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;
@@ -3469,6 +3472,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;

Choose a reason for hiding this comment

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

Consider using BitMap and par_xxx operations instead of an array of bool and explicit atomic operations.

Choose a reason for hiding this comment

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

I would also be fine with the change as is, if that gets this fix for a fairly noisy bug pushed sooner. So also consider a followup RFE to switch to using BitMap.


EvacuationFailedInfo* _evacuation_failed_info_array;

@@ -1144,10 +1146,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