8254028: G1 incorrectly updates scan_top for collection set regions during preparation of evacuation #557
Conversation
|
👋 Welcome back tschatzl! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
I'm not to fond of the comment saying this is not really needed. After the fix for JDK-8252752 prepare_region_for_scan() don't do anything with regions in the collection-set so I would prefer if we instead filtered those regions at the call-site.
In G1PrepareRegionsClosure::do_heap_region(HeapRegion* hr) we could add:
// Pepare regions not in the collection set for scanning.
if (!hr->in_collection_set()) {
_g1h->rem_set()->prepare_region_for_scan(hr);
}This way we don't need to check for collection-set regions in the assert either. What do you think?
|
Mailing list message from Thomas Schatzl on hotspot-gc-dev: Hi, On 08.10.20 20:48, Stefan Johansson wrote:
The comment can be fixed :) Would something like // Only non-collection set old regions should be scanned. be better? I considered changing the caller as suggested, but this adds additional - the caller needs to know that the initialization of the corresponding - and that this is the only preparation work performed in that method. This seems unnecessary. What do you think? Thanks, |
But it also makes the code clearer showing that we don't do any thing for collection-set regions in this call.
But I do agree that it's nice to assert that the collection-set regions are in the correct state. And if we want to prepare for future preparation that also do stuff for the collection-set regions I think we should go back to the old if-else like this: if (r->in_collection_set()) {
// Regions in the collection-set should not be scanned.
assert(_scan_state->scan_top(hrm_index) == NULL,
"scan_top of region %u is unexpectedly " PTR_FORMAT,
hrm_index, p2i(_scan_state->scan_top(hrm_index)));
} else if(r->is_old_or_humongous_or_archive()) {
// Prepare all other regions for scanning.
_scan_state->set_scan_top(hrm_index, r->top());
} else {
assert(r->is_free(), "Region %u should be free but is %s",
hrm_index, r->get_type_str());
} I think this satisfies both our wishes, do you agree? |
|
@tschatzl 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 34 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
|
Mailing list message from Thomas Schatzl on hotspot-gc-dev: Hi, On 10.10.20 00:29, Thomas Schatzl wrote:
after some chat with Stefan we agreed on above version. Since this revision has been created in direct conversation with Stefan, Thanks for your reviews. Thanks, |
openjdk
bot
added
integrated
|
@tschatzl Since your change was applied there have been 45 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit bf46acf. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi all,
can I have reviews for this change that makes values of G1RemSetScanState::_scan_top for regions in the initial collection set consistent with ones in optional collection set?
So currently G1RemSetScanState::_scan_top is top() for regions in the initial collection set although they will never be scanned as enforced when dropping the remsets onto the card table. For the optional collection set, G1 sets scan_top manually to NULL when selecting the next few optional regions (using G1RemSet::exclude_region_from_scan()).
When debugging this discrepancy recently posed some slight surprise for me, so I would like to change this. Also there is some risk that future code that relies on that property will be surprised.
Testing: tier1-4; lots of kitchensink runs with JDK-8254164.
Thanks,
Thomas
Progress
Testing
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/557/head:pull/557$ git checkout pull/557