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

8338534: GenShen: Handle alloc failure differently when immediate garbage is pending #479

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
7bb1d38
Remove dead code for inelastic plabs
kdnilsen Feb 26, 2024
8bc4367
Revert "Remove dead code for inelastic plabs"
kdnilsen Feb 26, 2024
99cce53
Round LAB sizes down rather than up to force alignment
kdnilsen Feb 26, 2024
11b26bb
Revert "Round LAB sizes down rather than up to force alignment"
kdnilsen Feb 26, 2024
941d8aa
Merge branch 'openjdk:master' into master
kdnilsen Feb 27, 2024
39c5885
Merge branch 'openjdk:master' into master
kdnilsen Mar 13, 2024
28a382b
Make satb-mode Info logging less verbose
kdnilsen Mar 13, 2024
a43675a
Merge branch 'openjdk:master' into master
kdnilsen Apr 10, 2024
d881300
Change behavior of max_old and min_old
kdnilsen Apr 11, 2024
c2cb1b7
Revert "Change behavior of max_old and min_old"
kdnilsen Apr 11, 2024
141fec1
Merge branch 'openjdk:master' into master
kdnilsen Apr 15, 2024
bac08f0
Merge branch 'openjdk:master' into master
kdnilsen Apr 23, 2024
84f27d7
Merge branch 'openjdk:master' into master
kdnilsen May 15, 2024
118f5b1
Merge branch 'openjdk:master' into master
kdnilsen Jun 6, 2024
5312029
Merge branch 'openjdk:master' into master
kdnilsen Jun 11, 2024
56567b0
Merge branch 'openjdk:master' into master
kdnilsen Jun 13, 2024
25ee3f5
Merge branch 'openjdk:master' into master
kdnilsen Jun 26, 2024
c076aa3
Merge branch 'openjdk:master' into master
kdnilsen Jul 8, 2024
ff99de7
Merge branch 'openjdk:master' into master
kdnilsen Aug 12, 2024
b8b4e42
Merge branch 'openjdk:master' into master
kdnilsen Aug 18, 2024
1c26ae0
Reorder concurrent cleanup following final mark
kdnilsen Aug 18, 2024
634ef66
Do not log_status() for free set after cleanup early
kdnilsen Aug 19, 2024
4198b75
Notify waiting allocators after cleanup of immediate garbage
kdnilsen Aug 19, 2024
408f789
Do not clear alloc-failure flag after cleanup early
kdnilsen Aug 19, 2024
9e9c54c
Improve comments regarding when we can recycle trashed regions
kdnilsen Aug 19, 2024
8461939
Do not cancel GC on allocation failure if immediate garbage is adequate
kdnilsen Aug 20, 2024
02fd5c1
Whitespace
kdnilsen Aug 21, 2024
5c0bb7d
Experiment with white space jcheck
kdnilsen Aug 21, 2024
f11a3c8
Fix whitespace
kdnilsen Aug 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -152,23 +152,28 @@ bool ShenandoahConcurrentGC::collect(GCCause::Cause cause) {
return false;
}

// Concurrent stack processing
if (heap->is_evacuation_in_progress()) {
entry_thread_roots();
}

// Process weak roots that might still point to regions that would be broken by cleanup
// Process weak roots that might still point to regions that would be broken by cleanup. This must precede cleanup.
if (heap->is_concurrent_weak_root_in_progress()) {
entry_weak_refs();
entry_weak_roots();
}

// Final mark might have reclaimed some immediate garbage, kick cleanup to reclaim
// the space. This would be the last action if there is nothing to evacuate. Note that
// we will not age young-gen objects in the case that we skip evacuation.
// Final mark might have reclaimed some immediate garbage, kick cleanup to reclaim the space. We do this before
// concurrent roots and concurrent class unloading so as to expedite recycling of immediate garbage. Note that
// we will not age young-gen objects in the case that we skip evacuation for abbreviated cycles.
entry_cleanup_early();

#ifdef KELVIN_DEPRECATE
// We just dumped the free set after rebuilding free set in final_mark. Let's not dump it again.
// There may be some contention with mutator and GC worker threads who are trying to begin their evacuation
// efforts, and would prefer not to grab the heap lock right here.
heap->free_set()->log_status_under_lock();
#endif

// Concurrent stack processing
if (heap->is_evacuation_in_progress()) {
entry_thread_roots();
}

// Perform concurrent class unloading
if (heap->unload_classes() &&
Expand Down Expand Up @@ -746,7 +751,7 @@ void ShenandoahConcurrentGC::op_final_mark() {

// Arm nmethods/stack for concurrent processing
if (!heap->collection_set()->is_empty()) {
// Iff objects will be evaluated, arm the nmethod barriers. These will be disarmed
// Iff objects will be evacuated, arm the nmethod barriers. These will be disarmed
// under the same condition (established in prepare_concurrent_roots) after strong
// root evacuation has completed (see op_strong_roots).
ShenandoahCodeRoots::arm_nmethods_for_evac();
Expand Down Expand Up @@ -1061,7 +1066,10 @@ void ShenandoahConcurrentGC::op_strong_roots() {
}

void ShenandoahConcurrentGC::op_cleanup_early() {
ShenandoahHeap::heap()->free_set()->recycle_trash();
ShenandoahHeap* heap = ShenandoahHeap::heap();
if (heap->free_set()->recycle_trash()) {
heap->control_thread()->notify_alloc_failure_waiters(false);
Copy link
Member

Choose a reason for hiding this comment

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

Can you motivate this notification? As far as I can tell, all waiters will react to the notification by waking up, finding that the variables are still set, and consequently go back to wait.

I am sure I am missing something here, or you didn't make an intended change to allow waiters to retry allocation after waking up and go back to sleep if they didn't succeed?

A documentation comment would definitely help cross the t's and dot the i's for the reader.

}
}

void ShenandoahConcurrentGC::op_evacuate() {
Expand Down
19 changes: 13 additions & 6 deletions src/hotspot/share/gc/shenandoah/shenandoahController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ size_t ShenandoahController::get_gc_id() {
return Atomic::load(&_gc_id);
}

void ShenandoahController::anticipate_immediate_garbage(size_t anticipated_immediate_garbage) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested rename see further above. set_<field>.

Atomic::store(&_anticipated_immediate_garbage, anticipated_immediate_garbage);
}

void ShenandoahController::handle_alloc_failure(ShenandoahAllocRequest& req, bool block) {
ShenandoahHeap* heap = ShenandoahHeap::heap();

Expand All @@ -65,11 +69,12 @@ void ShenandoahController::handle_alloc_failure(ShenandoahAllocRequest& req, boo
req.type_string(),
byte_size_in_proper_unit(req.size() * HeapWordSize), proper_unit_for_byte_size(req.size() * HeapWordSize));

// Now that alloc failure GC is scheduled, we can abort everything else
heap->cancel_gc(GCCause::_allocation_failure);
if (Atomic::load(&_anticipated_immediate_garbage) < req.size()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To make sure I understand... here we are saying that if final mark anticipates this much immediate garbage (computed when it rebuilt the freeset after choosing the collection set), then we aren't going to cancel the GC if this particular request could be satisfied. Instead we will block as though the gc has already been cancelled. This thread will be notified when concurrent cleanup completes.

// Now that alloc failure GC is scheduled, we can abort everything else
heap->cancel_gc(GCCause::_allocation_failure);
}
}


if (block) {
MonitorLocker ml(&_alloc_failure_waiters_lock);
while (is_alloc_failure_gc()) {
Expand All @@ -92,9 +97,11 @@ void ShenandoahController::handle_alloc_failure_evac(size_t words) {
heap->cancel_gc(GCCause::_shenandoah_allocation_failure_evac);
}

void ShenandoahController::notify_alloc_failure_waiters() {
_alloc_failure_gc.unset();
_humongous_alloc_failure_gc.unset();
void ShenandoahController::notify_alloc_failure_waiters(bool clear_alloc_failure) {
if (clear_alloc_failure) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we not clear the alloc failure? This seems like it would confuse the control thread. Isn't this going to have the control thread attempt to notify alloc failure waiters again when the cycle is finished?

_alloc_failure_gc.unset();
_humongous_alloc_failure_gc.unset();
}
Comment on lines +101 to +104
Copy link
Member

Choose a reason for hiding this comment

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

For good hygiene, I'd move the variable value changes into the monitor which is held when waiting or notifying. I realize this doesn't matter for correctness, but makes debugging easier.

Further, if you protect the updates and reads of the variables with the lock, you don't need to do the extra atomic ops.

You'd need to examine all sets/gets and waits/notifys to make sure this works, but I am guessing it will, and it'll also improve performance.

However, that can be done in a separate effort, if you prefer, for which I'm happy to file a separate ticket for that investigation/change.

Copy link
Member

Choose a reason for hiding this comment

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

I realize now that this idiom is quite pervasive in Shenandoah code, so just fixing this instance of it doesn't accomplish much at this time. I am not convinced it's a good idiom. I'll investigate this separately. I vaguely recall a discussion along these lines in an older PR.

I'll file a separate ticket for this; you can ignore this remark for the purposes of this PR.

MonitorLocker ml(&_alloc_failure_waiters_lock);
ml.notify_all();
}
Expand Down
6 changes: 5 additions & 1 deletion src/hotspot/share/gc/shenandoah/shenandoahController.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ class ShenandoahController: public ConcurrentGCThread {
shenandoah_padding(1);
volatile size_t _gc_id;
shenandoah_padding(2);
volatile size_t _anticipated_immediate_garbage;
shenandoah_padding(3);

protected:
ShenandoahSharedFlag _alloc_failure_gc;
Expand Down Expand Up @@ -71,6 +73,8 @@ class ShenandoahController: public ConcurrentGCThread {
// until another cycle runs and clears the alloc failure gc flag.
void handle_alloc_failure(ShenandoahAllocRequest& req, bool block);

void anticipate_immediate_garbage(size_t anticipated_immediate_garbage_words);
Copy link
Member

Choose a reason for hiding this comment

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

a 1-line documentation comment on the role of the field (and that the method sets it -- why not simply call it set_foo(value) for field _foo ? I realize you want readers to get the most recently written value, hence the atomic store & load.


// Invoked for allocation failures during evacuation. This cancels
// the collection cycle without blocking.
void handle_alloc_failure_evac(size_t words);
Expand All @@ -79,7 +83,7 @@ class ShenandoahController: public ConcurrentGCThread {
bool try_set_alloc_failure_gc(bool is_humongous);

// Notify threads waiting for GC to complete.
void notify_alloc_failure_waiters();
void notify_alloc_failure_waiters(bool clear_alloc_failure = true);

// True if allocation failure flag has been set.
bool is_alloc_failure_gc();
Expand Down
17 changes: 14 additions & 3 deletions src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,9 @@ ShenandoahRegionPartitions::ShenandoahRegionPartitions(size_t max_regions, Shena
}

inline bool ShenandoahFreeSet::can_allocate_from(ShenandoahHeapRegion *r) const {
// This test for trash regions is conservative. Strictly, we only need to assure that concurrent weak reference processing
// is not under way. That finishes long before concurrent weak root processing. It is ok to be conservative. At the
// end of weak reference processing, we recycle trashed regions en masse.
return r->is_empty() || (r->is_trash() && !_heap->is_concurrent_weak_root_in_progress());
}

Expand Down Expand Up @@ -1230,13 +1233,17 @@ HeapWord* ShenandoahFreeSet::allocate_contiguous(ShenandoahAllocRequest& req) {
return _heap->get_region(beg)->bottom();
}

void ShenandoahFreeSet::try_recycle_trashed(ShenandoahHeapRegion* r) {
bool ShenandoahFreeSet::try_recycle_trashed(ShenandoahHeapRegion* r) {
bool result = false;
if (r->is_trash()) {
r->recycle();
result = true;
}
return true;
}
Comment on lines +1236 to 1243
Copy link
Member

Choose a reason for hiding this comment

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

If I understood your intent, I think this has a bug because it always returns true here. I believe you just wanted:

    if (r->is_trash()) {
      r->recycle();
      return true;
    }
   return false;


void ShenandoahFreeSet::recycle_trash() {
bool ShenandoahFreeSet::recycle_trash() {
bool result = false;
Copy link
Member

Choose a reason for hiding this comment

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

I'd take the opportunity to do some counting verification here.

int n_trash_regions = 0;

Read on ...

// lock is not reentrable, check we don't have it
shenandoah_assert_not_heaplocked();

Expand All @@ -1256,9 +1263,13 @@ void ShenandoahFreeSet::recycle_trash() {
ShenandoahHeapLocker locker(_heap->lock());
const jlong deadline = os::javaTimeNanos() + deadline_ns;
while (idx < count && os::javaTimeNanos() < deadline) {
try_recycle_trashed(_trash_regions[idx++]);
if (try_recycle_trashed(_trash_regions[idx++])) {
result = true;
Copy link
Member

Choose a reason for hiding this comment

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

...

   n_trash_regions++;

...

}
}
}
_heap->control_thread()->anticipate_immediate_garbage((size_t) 0);
return result;
Comment on lines +1271 to +1272
Copy link
Member

Choose a reason for hiding this comment

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

...

clear_anticipated_immediate_garage(n_trash_regions*HeapRegionSize);
   return n_trash_regions > 0;

with

void ...::clear_anticipated_immediate_garbage(size_t aig) {
    assert(_anticipated_immediate_garbage == aig, "Mismatch?");
    _anticipated_immediate_garbage = 0;
}

Is this an intended invariant? I think it is, but don't understand enough of the details to be certain.

}

void ShenandoahFreeSet::flip_to_old_gc(ShenandoahHeapRegion* r) {
Expand Down
7 changes: 5 additions & 2 deletions src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,9 @@ class ShenandoahFreeSet : public CHeapObj<mtGC> {
void flip_to_old_gc(ShenandoahHeapRegion* r);

void clear_internal();
void try_recycle_trashed(ShenandoahHeapRegion *r);

// If region r is a trash region, recycle it, returning true iff r was recycled.
bool try_recycle_trashed(ShenandoahHeapRegion *r);

// Returns true iff this region is entirely available, either because it is empty() or because it has been found to represent
// immediate trash and we'll be able to immediately recycle it. Note that we cannot recycle immediate trash if
Expand Down Expand Up @@ -410,7 +412,8 @@ class ShenandoahFreeSet : public CHeapObj<mtGC> {
// for evacuation, invoke this to make regions available for mutator allocations.
void move_regions_from_collector_to_mutator(size_t cset_regions);

void recycle_trash();
// Recycle any trash that is known to the freeset, returning true if any trash was recycled.
bool recycle_trash();

// Acquire heap lock and log status, assuming heap lock is not acquired by the caller.
void log_status_under_lock();
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,9 @@ void ShenandoahGeneration::prepare_regions_and_collection_set(bool concurrent) {
// We are preparing for evacuation. At this time, we ignore cset region tallies.
size_t first_old, last_old, num_old;
heap->free_set()->prepare_to_rebuild(young_cset_regions, old_cset_regions, first_old, last_old, num_old);
size_t anticipated_immediate_garbage = (old_cset_regions + young_cset_regions) * ShenandoahHeapRegion::region_size_words();
Copy link
Member

Choose a reason for hiding this comment

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

This makes it sound like old_cset_regions & young_cset_regions hold all regions that will be part of immediate garbage -- which indeed is the case. The name prepare_to_rebuild() does not give one much clue as to the fact that that's happening when we return from the call, and the API spec of the method does not explicitly specify it. One needs to read into the code of the method find_regions_with_alloc_capacity() to realize that this is what is happening.

In summary, firstly, I feel some of these methods are in need of tighter header file documentation of post-conditions that callers are relying on and, secondly, I feel given the extremely fat APIs (lots of reference variables that are modified by these methods) that some amount of refactoring is needed in the longer term. The refactoring should be a separate effort, but in the shorter term I think the API/spec documentation of prepare_to_rebuild and find_regions_with_alloc_capacity could be improved.

Copy link
Contributor

Choose a reason for hiding this comment

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

The names old_cset_regions and young_cset_regions is very confusing as well. These regions are already trash before the collection set is chosen during final mark (and so, will not themselves be part of the collection set). Suggest calling them old_trashed_regions and young_trashed_regions here.

heap->control_thread()->anticipate_immediate_garbage(anticipated_immediate_garbage);

// Free set construction uses reserve quantities, because they are known to be valid here
heap->free_set()->finish_rebuild(young_cset_regions, old_cset_regions, num_old, true);
}
Expand Down
7 changes: 5 additions & 2 deletions src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2436,9 +2436,9 @@
ShenandoahPhaseTimings::final_update_refs_update_region_states :
ShenandoahPhaseTimings::degen_gc_final_update_refs_update_region_states);

final_update_refs_update_region_states();
final_update_refs_update_region_states();

assert_pinned_region_status();
assert_pinned_region_status();
}

{
Expand All @@ -2447,7 +2447,7 @@
ShenandoahPhaseTimings::degen_gc_final_update_refs_trash_cset);
trash_cset_regions();
}
}

Check failure on line 2450 in src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

View check run for this annotation

openjdk / jcheck-openjdk/shenandoah-479

Whitespace error

Column 0: trailing whitespace Column 1: trailing whitespace

void ShenandoahHeap::final_update_refs_update_region_states() {
ShenandoahSynchronizePinnedRegionStates cl;
Expand All @@ -2462,6 +2462,9 @@
size_t young_cset_regions, old_cset_regions;
size_t first_old_region, last_old_region, old_region_count;
_free_set->prepare_to_rebuild(young_cset_regions, old_cset_regions, first_old_region, last_old_region, old_region_count);
size_t anticipated_immediate_garbage = (old_cset_regions + young_cset_regions) * ShenandoahHeapRegion::region_size_words();
control_thread()->anticipate_immediate_garbage(anticipated_immediate_garbage);

Copy link
Member

Choose a reason for hiding this comment

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

This is the line that has two whitespaces, vide the jcheck whitespace error above.

// If there are no old regions, first_old_region will be greater than last_old_region
assert((first_old_region > last_old_region) ||
((last_old_region + 1 - first_old_region >= old_region_count) &&
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,9 @@ void ShenandoahOldGeneration::prepare_regions_and_collection_set(bool concurrent
size_t cset_young_regions, cset_old_regions;
size_t first_old, last_old, num_old;
heap->free_set()->prepare_to_rebuild(cset_young_regions, cset_old_regions, first_old, last_old, num_old);
size_t anticipated_immediate_garbage = (cset_young_regions + cset_old_regions) * ShenandoahHeapRegion::region_size_words();
heap->control_thread()->anticipate_immediate_garbage(anticipated_immediate_garbage);
Copy link
Member

Choose a reason for hiding this comment

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

suggest set_<foo> for changing field value <foo>.


// This is just old-gen completion. No future budgeting required here. The only reason to rebuild the freeset here
// is in case there was any immediate old garbage identified.
heap->free_set()->finish_rebuild(cset_young_regions, cset_old_regions, num_old);
Expand Down