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

8277736: G1: Allow forced evacuation failure of first N regions in collection set #6561

Closed
Closed
Changes from 2 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
@@ -421,8 +421,8 @@ HeapWord* G1ParScanThreadState::allocate_copy_slow(G1HeapRegionAttr* dest_attr,
}

#if EVAC_FAILURE_INJECTOR
bool G1ParScanThreadState::inject_evacuation_failure() {
return _g1h->evac_failure_injector()->evacuation_should_fail(_evac_failure_inject_counter);
bool G1ParScanThreadState::inject_evacuation_failure(uint region_idx) {
return _g1h->evac_failure_injector()->evacuation_should_fail(_evac_failure_inject_counter, region_idx);
}
#endif

@@ -470,7 +470,7 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio
assert(_g1h->is_in_reserved(obj_ptr), "Allocated memory should be in the heap");

// Should this evacuation fail?
if (inject_evacuation_failure()) {
if (inject_evacuation_failure(from_region->hrm_index())) {
// Doing this after all the allocation attempts also tests the
// undo_allocation() method too.
undo_allocation(dest_attr, obj_ptr, word_sz, node_index);
@@ -107,7 +107,7 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {
EvacuationFailedInfo _evacuation_failed_info;
G1EvacFailureRegions* _evac_failure_regions;

bool inject_evacuation_failure() EVAC_FAILURE_INJECTOR_RETURN_( return false; );
bool inject_evacuation_failure(uint region_idx) EVAC_FAILURE_INJECTOR_RETURN_( return false; );

public:
G1ParScanThreadState(G1CollectedHeap* g1h,
@@ -30,6 +30,33 @@

#if EVAC_FAILURE_INJECTOR

class SelectEvacFailureRegionClosure : public HeapRegionClosure {
CHeapBitMap& _regions;
size_t _evac_failure_regions_num;

public:
SelectEvacFailureRegionClosure(CHeapBitMap& regions, size_t cset_length) :
_regions(regions),
_evac_failure_regions_num(cset_length * G1EvacuationFailureALotCSetPercent / 100) { }

bool do_heap_region(HeapRegion* r) override {
assert(r->in_collection_set(), "must be");
if (_evac_failure_regions_num > 0) {
_regions.set_bit(r->hrm_index());
--_evac_failure_regions_num;
return false;
Copy link
Contributor

@tschatzl tschatzl Nov 25, 2021

Choose a reason for hiding this comment

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

Just an initial comment to think about and discuss: This would most likely always ever select the same regions (probably eden) that are first in the collection set.
I think selecting by probability (with G1EvacuationFailureALotCSetPercent uniform probability) would be more interesting even if it makes the selection more random.

Copy link
Author

@Hamlin-Li Hamlin-Li Nov 26, 2021

Choose a reason for hiding this comment

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

Yes, currently I only make it select regions starting from first.
The reason I don't add random selection is that, currently we need this functionality to verify the functionality and perf improvement of JDK-8256265 implementation, so a more stable selection of evacuation failure regions will help to verify the effect of JDK-8256265.

I agree that random selection is an interesting option to supply too, I just file https://bugs.openjdk.java.net/browse/JDK-8277851 to track the issue.

Copy link
Member

@albertnetymk albertnetymk Nov 30, 2021

Choose a reason for hiding this comment

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

This PR makes the first X% regions in cset evac-fail, but G1EvacuationFailureALotCSetPercent doesn't show such bias. (It kind of implies uniform distribution, IMO.) It would be nice if the name and/or the explanation string could be more specific.

}
return true;
}
};

void G1YoungGCEvacFailureInjector::select_evac_failure_regions() {
G1CollectedHeap* g1h = G1CollectedHeap::heap();
_regions.reinitialize(g1h->max_reserved_regions());
SelectEvacFailureRegionClosure closure(_regions, g1h->collection_set()->cur_length());
g1h->collection_set_iterate_all(&closure);
}

bool G1YoungGCEvacFailureInjector::arm_if_needed_for_gc_type(bool for_young_gc,
bool during_concurrent_start,
bool mark_or_rebuild_in_progress) {
@@ -68,6 +95,10 @@ void G1YoungGCEvacFailureInjector::arm_if_needed() {
arm_if_needed_for_gc_type(in_young_only_phase,
in_concurrent_start_gc,
mark_or_rebuild_in_progress);

if (_inject_evacuation_failure_for_current_gc) {
select_evac_failure_regions();
}
}
}

@@ -56,11 +56,17 @@ class G1YoungGCEvacFailureInjector {
// Used to determine whether evacuation failure injection should be in effect
// for the current GC.
size_t _last_collection_with_evacuation_failure;

// Records the regions that will fail evacuation.
CHeapBitMap _regions;
Copy link
Member

@albertnetymk albertnetymk Nov 30, 2021

Choose a reason for hiding this comment

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

How about calling this field _evac_fail_regions?

#endif

bool arm_if_needed_for_gc_type(bool for_young_gc,
bool during_concurrent_start,
bool mark_or_rebuild_in_progress) EVAC_FAILURE_INJECTOR_RETURN_( return false; );

// Selects the regions that will fail evacuation by G1EvacuationFailureALotCSetPercent.
void select_evac_failure_regions() EVAC_FAILURE_INJECTOR_RETURN;
public:

// Arm the evacuation failure injector if needed for the current
@@ -69,7 +75,7 @@ class G1YoungGCEvacFailureInjector {

// Return true if it's time to cause an evacuation failure; the caller
// provides the (preferably thread-local) counter to minimize performance impact.
bool evacuation_should_fail(size_t& counter) EVAC_FAILURE_INJECTOR_RETURN_( return false; );
bool evacuation_should_fail(size_t& counter, uint region_idx) EVAC_FAILURE_INJECTOR_RETURN_( return false; );

// Reset the evacuation failure injection counters. Should be called at
// the end of an evacuation pause in which an evacuation failure occurred.
@@ -32,10 +32,13 @@

#if EVAC_FAILURE_INJECTOR

inline bool G1YoungGCEvacFailureInjector::evacuation_should_fail(size_t& counter) {
inline bool G1YoungGCEvacFailureInjector::evacuation_should_fail(size_t& counter, uint region_idx) {
if (!_inject_evacuation_failure_for_current_gc) {
return false;
}
if (!_regions.at(region_idx)) {
return false;
}
if (++counter < G1EvacuationFailureALotCount) {
return false;
}
@@ -73,7 +73,11 @@
\
product(bool, G1EvacuationFailureALotDuringMixedGC, true, \
"Force use of evacuation failure handling during mixed " \
"evacuation pauses")
"evacuation pauses") \
\
product(uintx, G1EvacuationFailureALotCSetPercent, 100, \
Copy link
Member

@albertnetymk albertnetymk Nov 30, 2021

Choose a reason for hiding this comment

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

Why uintx? Does uint work?

Copy link
Author

@Hamlin-Li Hamlin-Li Dec 1, 2021

Choose a reason for hiding this comment

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

I will refine the explanation specifically.
As Thomas suggested, an random selection will be added later, I plan to add another bool option to indicate if it's random selection or not, so will keep G1EvacuationFailureALotCSetPercent.

Copy link
Contributor

@tschatzl tschatzl Dec 1, 2021

Choose a reason for hiding this comment

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

Another, imo better way would be allowing a string for the type of selection algorithm, like VerifyGCType, with current possible values being NthObject (or just default) and FirstRegions or so.

Copy link
Contributor

@tschatzl tschatzl Dec 1, 2021

Choose a reason for hiding this comment

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

That could be extended to RandomRegions later.

This needs another flag though, maybe something like G1EvacuationFailureALotObjectSelectionType (random name right now, probably you will come up with something better).

Copy link
Author

@Hamlin-Li Hamlin-Li Dec 1, 2021

Choose a reason for hiding this comment

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

I think you mean we add this option when implement https://bugs.openjdk.java.net/browse/JDK-8277851, am I right? If positive, sure, it make sense to me, it's much easier to extend the algorithms in the future.

Copy link
Contributor

@tschatzl tschatzl Dec 1, 2021

Choose a reason for hiding this comment

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

I am good with deferring the additional flag.

"Percent of the regions in cset that will fail evacuation") \
Copy link
Contributor

@tschatzl tschatzl Dec 1, 2021

Choose a reason for hiding this comment

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

Please spell out "cset" in the description text though as it is done everywhere else in g1_globals.hpp. Considering Albert's suggestion, maybe something like:

The percentage of regions in the collection set starting from the beginning where the forced evacuation failure injection will be applied.

Copy link
Author

@Hamlin-Li Hamlin-Li Dec 1, 2021

Choose a reason for hiding this comment

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

Sure. Thanks!

range(1, 100)
#else
#define GC_G1_EVACUATION_FAILURE_FLAGS(develop, \
develop_pd, \