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
JDK-8267562: G1: Missing BOT in Open Archive regions causes long pauses #4265
Conversation
|
@jianglizhou The following label 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 list. 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.
Lgtm apart from the comment issue.
src/hotspot/share/cds/filemap.cpp
Outdated
|
||
// Populate the open archive regions' G1BlockOffsetTableParts. That ensures | ||
// fast G1BlockOffsetTablePart::block_start operations for a given address | ||
// within the open archive regions during G1RemSet::refine_card_concurrently. |
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 only refinement is subject to the issue but any operation trying to find start of an object (e.g. card table scanning). Please fix the comment accordingly.
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.
Updated comment based on the feedback from Thomas. Thanks for the review.
@jianglizhou This change now passes all automated pre-integration checks. 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 26 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.
|
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.
looks good.
Thanks for the review. |
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.
One last comment change :)
src/hotspot/share/cds/filemap.cpp
Outdated
// This is only needed for open archive regions, but not the closed archive | ||
// regions, because objects in closed archive regions are 'immutable'. |
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 only needed for open archive regions, but not the closed archive | |
// regions, because objects in closed archive regions are 'immutable'. | |
// This is only needed for open archive regions, but not the closed archive | |
// regions, because objects in closed archive regions never reference objects | |
// outside the closed archive regions and they are immutable, so we never | |
// need their BOT during garbage collection. |
Thank for the suggestion. That does explain more clearly. Updated as suggested. |
/integrate |
@jianglizhou Since your change was applied there have been 30 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit b17b11e. |
Populate G1BlockOffsetTableParts and associated G1BlockOffsetTable::_offset_array entries for 'open' archive regions during VM initialization at runtime.
G1BlockOffsetTable divides the covered space (Java heap) into “N”-word subregions (“N” is from 2^”LogN”). It uses an _offset_array to tell how far back it must go to find the start of a block that contains the first word of a subregion. Every G1 region (is a G1ContiguousSpace) owns a G1BlockOffsetTablePart (associates to part of the _offset_array), which covers space of the current region. For a mapped open archive heap region with pre-populated Java objects, its G1BlockOffsetTablePart is never setup at runtime because there is no allocation done within the region. As a result, G1BlockOffsetTablePart::block_start() always does lookup from the start (bottom) of an open archive region when called at runtime, regardless if the given 'addr' is near the bottom, top or in the middle of the region. The lookup becomes linear, instead of O(2^LogN). Large heap region size makes the situation worse and young-gen pauses longer.
Removed G1FullGCPrepareTask::G1CalculatePointersClosure::update_bot (introduced by JDK-8264987 change, #3459). It's merged as G1BlockOffsetTablePart::update, which is used for Survivor-turned-to-Old regions in full gc and open archive region. Thanks Thomas Schatzl for pointing to JDK-8264987 change for code reuse.
Tested with tier1 and hotspot_gc tests on linux_x86. Appreciate any help for testing other platforms and more tiers. Thanks!
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4265/head:pull/4265
$ git checkout pull/4265
Update a local copy of the PR:
$ git checkout pull/4265
$ git pull https://git.openjdk.java.net/jdk pull/4265/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4265
View PR using the GUI difftool:
$ git pr show -t 4265
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4265.diff