-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8353115: GenShen: mixed evacuation candidate regions need accurate live_data #24319
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
Closed
kdnilsen
wants to merge
40
commits into
openjdk:master
from
kdnilsen:fix-live-data-for-mixed-evac-candidates
Closed
Changes from all commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
702710e
Improve documentation of how Evac-OOM Protocol works
kdnilsen 61b575f
Merge branch 'openjdk:master' into master
kdnilsen 51d056f
Revert "Improve documentation of how Evac-OOM Protocol works"
kdnilsen ba98e42
Merge branch 'openjdk:master' into master
kdnilsen 441487c
Merge branch 'openjdk:master' into master
kdnilsen dafc363
Merge branch 'openjdk:master' into master
kdnilsen c4c252e
Merge branch 'openjdk:master' into master
kdnilsen 41ba86a
Merge branch 'openjdk:master' into master
kdnilsen f215a70
Merge branch 'openjdk:master' into master
kdnilsen 4d6b5cd
Merge branch 'openjdk:master' into master
kdnilsen 7fe605f
Merge branch 'openjdk:master' into master
kdnilsen 2e224f6
Merge branch 'openjdk:master' into master
kdnilsen 46ad5c6
Merge branch 'openjdk:master' into master
kdnilsen 9a1989d
Merge branch 'openjdk:master' into master
kdnilsen 4126c22
Merge branch 'openjdk:master' into master
kdnilsen 981692e
Merge branch 'openjdk:master' into master
kdnilsen 3a67b1f
Make GC logging less verbose
kdnilsen 3692312
Revert "Make GC logging less verbose"
kdnilsen 045590b
Merge branch 'openjdk:master' into master
kdnilsen fbbd88c
Merge branch 'openjdk:master' into master
kdnilsen 7e0edf0
Merge branch 'openjdk:master' into master
kdnilsen 3525369
Merge branch 'openjdk:master' into master
kdnilsen fe0da51
Merge branch 'openjdk:master' into master
kdnilsen db12fe5
Merge branch 'openjdk:master' into master
kdnilsen 0440bae
Merge branch 'openjdk:master' into master
kdnilsen 3bdc022
Merge branch 'openjdk:master' into master
kdnilsen 1ee2ff1
Merge branch 'openjdk:master' into master
kdnilsen e6e772f
Merge branch 'openjdk:master' into master
kdnilsen c5a159e
Merge branch 'openjdk:master' into master
kdnilsen e7ca4f8
Merge branch 'openjdk:master' into master
kdnilsen 42a93c7
Merge branch 'openjdk:master' into master
kdnilsen 7061388
Track live and garbage for mixed-evac regions
kdnilsen 3c1f788
Experiment with reviewer suggestion
kdnilsen 8ff388d
Experiment 2: refinements to reduce regressions
kdnilsen 8e820f2
Fix garbage_before_padded_for_promote()
kdnilsen d3cba66
Fix set_live() after full gc
kdnilsen eb2679a
Refactor for better abstraction
kdnilsen b9f828c
Adjust candidate live memory for each mixed evac
kdnilsen ef783d4
Remove deprecation conditional compiles
kdnilsen e6e44b6
Fix uninitialized variable
kdnilsen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do we need a new field to track this? During
final_mark
, we callincrease_live_data_alloc_words
to addTAMS + top
to_live_data
to account for objects allocated during mark. Could we "fix"get_live_data
so that it always returned marked objects (counted byincrease_live_data_gc_words
) plustop - TAMS
. This way, the live data would not become stale afterfinal_mark
and we wouldn't have another field to manage. What do you think?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.
This is a good idea. Let me experiment with this.
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.
My experiment with an initial attempt at this failed with over 60 failures. The "problem" is that we often consult get_live_data() in contexts from which it is "not appropriate" to add (top- TAMS) to the atomic volatile ShenandoahHeapRegion::_live_data() . I think most of these are asserts. I have so far confirmed that there are at least two different places that need to be fixed. Not sure how many total scenarios.
I'm willing to move forward with changes to the failing asserts to make this change work. I think the code would be cleaner with your suggested refactor. It just makes this PR a little more far-reaching than the original.
See the most recent commit on this PR to see the direction this would move us. Let me know if you think I should move forward with more refactoring, or revert this most recent change.
Thanks.
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 does look simpler. Do you have an example of one of the failing asserts?
One thing I hadn't considered is how "hot"
ShenandoahHeapRegion::get_live_data_words
is. Is there going to be a significant performance hit if we make this method do more work? It does look like this method is called frequently.Uh oh!
There was an error while loading. Please reload this page.
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.
Examples:
FullGC worker:
ShenandoahInitMarkUpdateRegionStateClosure::heap_region_do() {
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.
Not sure about performance impact, other than implementing and testing...
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 suspect performance impact is minimal.
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've committed changes that endeavor to implement the suggested refactor. Performance impact does appear to be minimal. This broader refactoring does change behavior slightly. In particular:
On one recently completed test run, we observed the following impacts compared to tip:
Shenandoah
+80.69% specjbb2015/trigger_failure p=0.00000
Control: 58.250 (+/- 13.48 ) 110
Test: 105.250 (+/- 33.13 ) 30
Genshen
-19.46% jme/context_switch_count p=0.00176
Control: 117.420 (+/- 28.01 ) 108
Test: 98.292 (+/- 32.76 ) 30
Perhaps we need more data to decide whether this is "significant".
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.
This result seems to be consistent. The effect on traditional Shenandoah is apparently to reduce the size of traditional Shenandoah collection sets also because certain regions that would have been collected are now rejected due to "better awareness" of how much live data will need to be copied. The amount of garbage associated with candidate regions for the young collection set is reduced by the amount of allocations above TAMS.
Previously, this had been erroneously reported as garbage. This has the effect of delaying reclamation of some garbage, resulting in an increase in allocation failures on the specjbb 2025 workload.
We might argue that the original behavior was incorrect, in that it was allowing violation of the intended evacuation budget.
We apparently were getting away with this violation because we were able to flip mutator regions to collector space, and/or because evacuation waste was sufficient to accommodate the unbudgeted evacuations.
Now that we have more accurate accounting of live memory, we could perhaps slightly reduce the default evacuation waste budget if we want to claw back the losses in specjbb performance (to enable larger collection sets) as part of this PR.