-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8331048: G1: Prune rebuild candidates based on G1HeapWastePercent early #18970
Conversation
👋 Welcome back iwalulya! A progress list of the required criteria for merging this PR into |
@walulyai This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
if (_needs_remembered_set_rebuild) { | ||
// Prune rebuild candidates based on G1HeapWastePercent. | ||
// Improves rebuild time in addition to remembered set memory usage. | ||
G1CollectionSetChooser::build(_g1h->workers(), _g1h->num_regions(), _g1h->policy()->candidates()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to put this block into above block so that it is part of the "Select For Rebuild and Reclaim Empty Regions" measurement.
if (_needs_remembered_set_rebuild) { | ||
// Prune rebuild candidates based on G1HeapWastePercent. | ||
// Improves rebuild time in addition to remembered set memory usage. | ||
G1CollectionSetChooser::build(_g1h->workers(), _g1h->num_regions(), _g1h->policy()->candidates()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build
method sorts according to gc efficiency and prunes.
GC efficiency can not be properly determined at this point and is entirely based on occupancy (remsets are empty); so basically what this does is sort and prune by live objects.
The whole gc-efficiency calculation can then be removed.
Actually I am not sure making the efficiency only based on free space is problematic, however we shouldn't pretend we care then.
An alternative I tried long time ago has been counting incoming references during marking as a substitute for the remset size metric. However in reality, the remembered set is a metric based on cards, so it will potentially be hard to assign cost for remembered set processing based on incoming references.
@@ -95,6 +95,21 @@ void G1CollectionCandidateList::verify() { | |||
#endif | |||
|
|||
int G1CollectionCandidateList::compare(G1CollectionSetCandidateInfo* ci1, G1CollectionSetCandidateInfo* ci2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if the old comparison function would be renamed as well similarly to the other, e.g. compare_by_efficiencies
.
if (reclaimable1 == reclaimable2) return 0; | ||
if (reclaimable1 > reclaimable2) return -1; | ||
return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the same style for this if-elseif-else cascade than for the other one.
HeapRegion* hr = (*iter)->_r; | ||
(*iter)->_gc_efficiency = hr->calc_gc_efficiency(); | ||
} | ||
_marking_regions.sort_by_efficiency(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit surprising that this method also sorts by efficiency. I would almost say that the sorting is more important than the calculations.
I.e. something like sort_by_gc_efficiency
seems better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. Lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that pruning occurs in remark
, I wonder if those pruned regions should/can switch to remset-untracked instead.
@@ -199,7 +225,7 @@ void G1CollectionSetCandidates::set_candidates_from_marking(G1CollectionSetCandi | |||
verify(); | |||
} | |||
|
|||
void G1CollectionSetCandidates::sort_by_efficiency() { | |||
void G1CollectionSetCandidates::sort_retained_by_efficiency() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this renaming? Checking its callers, I feel the postcondition is that both kinds of candidates are sorted -- marking-candidates can be skipped seems an impl detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me revert this!
@@ -129,7 +129,9 @@ class G1CollectionCandidateList : public CHeapObj<mtGC> { | |||
// Comparison function to order regions in decreasing GC efficiency order. This | |||
// will cause regions with a lot of live objects and large remembered sets to end | |||
// up at the end of the list. | |||
static int compare(G1CollectionSetCandidateInfo* ci1, G1CollectionSetCandidateInfo* ci2); | |||
static int compare_gc_efficiency(G1CollectionSetCandidateInfo* ci1, G1CollectionSetCandidateInfo* ci2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preexisting: I wonder if these can be static functions in cpp file, i.e. not in hpp at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible, but then they end up in different .cpp files, not sure if that is cleaner
iiuc, the |
Thanks @albertnetymk and @tschatzl for your reviews! /integrate |
Going to push as commit ce73fec.
Your commit was automatically rebased without conflicts. |
Hi,
Please review this change that moves the
G1CollectionSetChooser::build
out of the cleanup pause into the remark pause. This prunes the rebuild candidates based on G1HeapWastePercent, thereby reducing the rebuild time in cases where pruning happens.Testing: Tier 1 - 5
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18970/head:pull/18970
$ git checkout pull/18970
Update a local copy of the PR:
$ git checkout pull/18970
$ git pull https://git.openjdk.org/jdk.git pull/18970/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18970
View PR using the GUI difftool:
$ git pr show -t 18970
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18970.diff
Webrev
Link to Webrev Comment