Skip to content

8357218: G1: Remove loop in G1CollectedHeap::try_collect_fullgc #25296

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

8357218: G1: Remove loop in G1CollectedHeap::try_collect_fullgc #25296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
233a5d1
g1-remove-full-gc-loop
albertnetymk May 19, 2025
722aac3
review
albertnetymk May 19, 2025
666cc5a
review
albertnetymk May 19, 2025
22c5225
Revert "review"
albertnetymk May 19, 2025
93c6e99
review
albertnetymk May 19, 2025
f5768f8
review
albertnetymk May 20, 2025
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: 5 additions & 25 deletions src/hotspot/share/gc/g1/g1CollectedHeap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1860,30 +1860,6 @@ bool G1CollectedHeap::try_collect_concurrently(GCCause::Cause cause,
}
}

bool G1CollectedHeap::try_collect_fullgc(GCCause::Cause cause,
const G1GCCounters& counters_before) {
assert_heap_not_locked();

while(true) {
VM_G1CollectFull op(counters_before.total_collections(),
counters_before.total_full_collections(),
cause);
VMThread::execute(&op);

// Request is trivially finished.
if (!GCCause::is_explicit_full_gc(cause) || op.gc_succeeded()) {
return op.gc_succeeded();
Copy link

Choose a reason for hiding this comment

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

Complete removal of this part isn't correct. The premise of this change, that "full-gc always
run-to-completion" is not correct. A _g1_periodic_collection may be cancelled:

jdk/src/hotspot/share/gc/g1/g1VMOperations.cpp

Lines 39 to 48 in 50a7c61

bool VM_G1CollectFull::skip_operation() const {
// There is a race between the periodic collection task's checks for
// wanting a collection and processing its request. A collection in that
// gap should cancel the request.
if ((_gc_cause == GCCause::_g1_periodic_collection) &&
(G1CollectedHeap::heap()->total_collections() != _gc_count_before)) {
return true;
}
return VM_GC_Operation::skip_operation();
}

Such a collection is not an explicit full-gc, so that part of the test may be true, when this function
returns false because the gc did not succeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the description. The overall result of !GCCause::is_explicit_full_gc(cause) || op.gc_succeeded() is always true.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Somehow comments seem to have disappeared, probably my fault. anyway:)

This method returns false if the VM op has not been executed (skipped). The new code always returns true, which is wrong afaict.

}

{
MutexLocker ml(Heap_lock);
if (counters_before.total_full_collections() != total_full_collections()) {
return true;
}
}
}
}

bool G1CollectedHeap::try_collect(GCCause::Cause cause,
const G1GCCounters& counters_before) {
if (should_do_concurrent_full_gc(cause)) {
Expand All @@ -1902,7 +1878,11 @@ bool G1CollectedHeap::try_collect(GCCause::Cause cause,
return op.gc_succeeded();
} else {
// Schedule a Full GC.
return try_collect_fullgc(cause, counters_before);
VM_G1CollectFull op(counters_before.total_collections(),
counters_before.total_full_collections(),
cause);
VMThread::execute(&op);
return op.gc_succeeded();
}
}

Expand Down
3 changes: 0 additions & 3 deletions src/hotspot/share/gc/g1/g1CollectedHeap.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,6 @@ class G1CollectedHeap : public CollectedHeap {
uint gc_counter,
uint old_marking_started_before);

bool try_collect_fullgc(GCCause::Cause cause,
const G1GCCounters& counters_before);

// indicates whether we are in young or mixed GC mode
G1CollectorState _collector_state;

Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/gc/g1/g1VMOperations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ bool VM_G1CollectFull::skip_operation() const {
void VM_G1CollectFull::doit() {
G1CollectedHeap* g1h = G1CollectedHeap::heap();
GCCauseSetter x(g1h, _gc_cause);
_gc_succeeded = g1h->do_full_collection(false /* clear_all_soft_refs */,
false /* do_maximal_compaction */);
g1h->do_full_collection(false /* clear_all_soft_refs */,
false /* do_maximal_compaction */);
}

VM_G1TryInitiateConcMark::VM_G1TryInitiateConcMark(uint gc_count_before,
Expand Down
7 changes: 2 additions & 5 deletions src/hotspot/share/gc/g1/g1VMOperations.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,20 +31,17 @@
// VM_operations for the G1 collector.

class VM_G1CollectFull : public VM_GC_Operation {
bool _gc_succeeded;

protected:
bool skip_operation() const override;

public:
VM_G1CollectFull(uint gc_count_before,
uint full_gc_count_before,
GCCause::Cause cause) :
VM_GC_Operation(gc_count_before, cause, full_gc_count_before, true),
_gc_succeeded(false) { }
VM_GC_Operation(gc_count_before, cause, full_gc_count_before, true) { }
VMOp_Type type() const override { return VMOp_G1CollectFull; }
void doit() override;
bool gc_succeeded() const { return _gc_succeeded; }
bool gc_succeeded() const { return prologue_succeeded(); }
};

class VM_G1TryInitiateConcMark : public VM_GC_Operation {
Expand Down