-
Notifications
You must be signed in to change notification settings - Fork 6.3k
8314599: [GenShen] Couple adaptive tenuring and generation size budgeting #27632
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
base: master
Are you sure you want to change the base?
8314599: [GenShen] Couple adaptive tenuring and generation size budgeting #27632
Conversation
…he first cohort that would be tenured
Some of the code is left behind to continue collecting the census when adaptive tenuring is disabled.
Added tag jdk-26+12 for changeset 02fe095
…ed to collection set
…ng excess old regions to young generation)
…ld' into make-evac-tracking-runtime-option
…es when computing excess old regions
👋 Welcome back wkemper! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
|
@earthling-amzn The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
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'm thinking that there should be a change to heap->rebuild_freeset() that adds get_promotion_potential() to the reserve for OldCollector. We did not previously make this reserve because we were only counting data that was to be promoted in place.
This may have the effect of triggering a bit sooner than in existing master. We could subtract out the pip_promo_reserve() if we want to keep track of that separately, because that doesn't need to be reserved.
Once the cset is constructed, we will shrink the reserves because at that point, we'll know better how much we really plan to evacuate into old.
|
||
size_t ShenandoahCollectionSet::get_young_bytes_reserved_for_evacuation() const { | ||
size_t ShenandoahCollectionSet::get_live_bytes_in_young_regions() const { | ||
return _young_bytes_to_evacuate - _young_bytes_to_promote; |
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'm wondering if these new names properly reflect the intention. It seems get_live_byte_in_young_regions() really means get_live_bytes_that_we_intend_to_evacuate_to_young(). (This number does not include _live_bytes_in_young_regions() that we expect to evacuate to old.)
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 called the complementary method get_live_bytes_in_tenurable_regions
. How about get_live_bytes_in_untenurable_regions
?
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'm still maybe a bit confused. Is it get_untenurable_live_bytes_in_young_regions()? Are we distinguishing?
So we have a total of N live bytes within young regions that have been placed into the collection set. We expect that P bytes (P < N) will be promoted, and the remaining S bytes (S + P == N) will be evacuated to "survivor space" within young.
Does get_live_bytes_in_untenurable_regions() equal P + S?
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 tenure here refers to the regions the objects reside in, not the objects themselves. get_live_bytes_in_tenurable_regions
is the sum of live bytes in all regions with an age above the tenuring threshold (we expect to promote all of these, though some promotions may fail). It's complement get_live_bytes_in_untenurable_regions
is the sum of live bytes in all regions with an age less than the tenuring threshold (we expect to promote some of these, but we don't really know how many). This was part of the reason I wanted to rename these methods. They represent the provenance of the objects in the collection set, not necessarily the regions they will be evacuated to.
// Add in the excess_old memory to hold unanticipated promotions, if any. If there are more unanticipated | ||
// promotions than fit in reserved memory, they will be deferred until a future GC pass. | ||
size_t total_promotion_reserve = young_advance_promoted_reserve_used + excess_old; | ||
old_generation->set_promoted_reserve(total_promotion_reserve); |
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.
Can you clarify why we no longer need this set_promoted_reserve() call? (just in a PR comment probably, not necessarily a code comment.)
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.
In compute_evacuation_reserves
we are setting the promotion reserve to the maximum possible to handle everything tenurable this cycle (this is still capped by the maximum evacuation reserve for old). I was reluctant to scale the promotion reserve by ShenandoahPromoEvacWaste
for fear it would over commit the collector's reserves and lead to OOM errors during evacuation.
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.
So in the new design, we have full awareness of all promotable objects, and we've already done our best to budget for those. So there's no such thing as "unanticipated promotions".
Separate question is whether we scale promotion reserve by ShenandoahPromoEvacWaste. So if old is larger than necessary to handle the anticipated mixed evacuations and promotions, the new code is essentially saying "use this extra space for mixed evacuations rather than for promotions". Since we're not expanding the promoted_reserve, promotions will not be allowed to touch it.
Am I understanding the intent correctly?
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.
In compute_evacuation_budgets
, if there are mixed collection candidates we set the initial promotion reserve to zero and the old evacuation reserve to the maximum. However, we then restrict old evacuation reserve to only empty regions. The difference between old available
and old unaffiliated
is given to the promotion reserve. Here again, I didn't want to scale the promotion reserve because it's basically the scraps of the old generation and I worry about over committing the old reserve. When there are no mixed collections, we use the entirety of old for promotions. Any old regions not needed for old evacuations or promotions are transferred to the young generation as they were before this change.
Notable changes:
With these changes, GenShen is expected to have fewer promotion failures and this is indeed the case. As a result of this, we expect less time to be spent in concurrent marking and update refs for young collections. We may also expect shorter concurrent evacuation phases because GenShen will have fewer densely packed regions stuck in the young generation. With more objects being promoted, we also expect to see longer remembered set scan times. This is generally the case across all benchmarks, but we do also see some counter-intuitive results.
Here we are comparing 20 executions (10 on x86, 10 on aarch64) of the changes in the PR (experiment) against 20 executions of the same benchmarks results from tip. This is a summary of statistically significant changes of more than 5% across all benchmarks:
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27632/head:pull/27632
$ git checkout pull/27632
Update a local copy of the PR:
$ git checkout pull/27632
$ git pull https://git.openjdk.org/jdk.git pull/27632/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27632
View PR using the GUI difftool:
$ git pr show -t 27632
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27632.diff
Using Webrev
Link to Webrev Comment