8306582: Remove MetaspaceShared::exit_after_static_dump()#14879
8306582: Remove MetaspaceShared::exit_after_static_dump()#14879matias9927 wants to merge 9 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back matsaave! A progress list of the required criteria for merging this PR into |
|
@matias9927 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. |
b4e06ff to
cf270aa
Compare
Webrevs
|
There was a problem hiding this comment.
Hi Matias,
I'm not keen on having to teach the launcher about -Xshare:dump and that it is a terminal operation. But after looking into it I have to admit it is a cleaner and more consistent termination strategy. That said you need to be aware that this will now do a clean shutdown of the VM after the dump, so shutdown hooks will now run.
Lets see what core-libs folk think too.
Thanks.
|
|
b479e64 to
2a27344
Compare
|
I messed up a merge and pushed the incorrect contents to this PR. It will be temporarily reverted to a draft and force pushed to undo my mistake. The existing comments should be addressed in the most recent commits. |
iklam
left a comment
There was a problem hiding this comment.
Looks good overall. Some suggestions for code refactoring.
iklam
left a comment
There was a problem hiding this comment.
Looks good. Just a couple of nits.
|
@matias9927 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 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
AlanBateman
left a comment
There was a problem hiding this comment.
Updated launcher change looks fine.
calvinccheung
left a comment
There was a problem hiding this comment.
Looks good overall. I have one question.
|
Thank you for the reviews @iklam, @calvinccheung, @dholmes-ora, and @AlanBateman! |
|
Going to push as commit cff25dd.
Your commit was automatically rebased without conflicts. |
|
@matias9927 Pushed as commit cff25dd. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Currently we exit the VM after static dumping with
MetaspaceShared::exit_after_static_dump().As the comment suggests, the VM state is altered when preparing and performing the static dump, so this change aims to prevent these state changes so the VM can exit normally after the static dump completes. There are three major aspects to this change:
Verified with tier 1-9 tests.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14879/head:pull/14879$ git checkout pull/14879Update a local copy of the PR:
$ git checkout pull/14879$ git pull https://git.openjdk.org/jdk.git pull/14879/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14879View PR using the GUI difftool:
$ git pr show -t 14879Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14879.diff
Webrev
Link to Webrev Comment