-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8264735: Make dynamic dump repeatable #4646
Conversation
👋 Welcome back minqi! A progress list of the required criteria for merging this PR into |
Webrevs
|
@@ -158,6 +162,11 @@ class DynamicArchiveBuilder : public ArchiveBuilder { | |||
write_archive(serialized_data); | |||
release_header(); | |||
|
|||
post_dump(); | |||
|
|||
// Restore dumptime talbes |
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.
Typo: talbes -> tables
src/hotspot/share/cds/filemap.cpp
Outdated
assert(jrt != NULL, | ||
"No modular java runtime image present when allocating the CDS classpath entry table"); | ||
|
||
// After dynamic dump, _saved_shared_path_table is corrupt, can not be used again. |
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.
Typo: can not -> cannot
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.
Will update.
void SystemDictionaryShared::restore_dumptime_tables() { | ||
// FileMapInfo::clean_cloned_shared_path_table(); |
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.
Please delete line 1605 if it is not needed.
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.
Ah, forget to delete it. 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.
Looks good. I have a few minor comments.
@yminqi 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 11 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 |
post_dump(); | ||
|
||
// Restore dumptime tables | ||
SystemDictionaryShared::restore_dumptime_tables(); |
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.
Maybe this should be put inside post_dump as well?
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 would be better to keep it here with SystemDictionaryShared::clone_dumptime_table().
// GCC will memset for default ctor. With user defined ctor, data members need manually initialized. | ||
// We could not use default ctor, it will destroy allocation_type in debug version and fail to delete. | ||
// see bug 8269537 | ||
DumpTimeLambdaProxyClassDictionary() : _count(0) {} |
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.
Since you changed the construction call from
(ResourceObj::C_HEAP, mtClass)DumpTimeLambdaProxyClassDictionary();
to
(ResourceObj::C_HEAP, mtClass)DumpTimeLambdaProxyClassDictionary;
I think it's no longer necessary to add the RunTimeLambdaProxyClassInfo()
and DumpTimeSharedClassTable()
constructors in this PR. In 8269537, we should find a generic way to prevent this bug from happening.
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.
Initialization of _count is still needed. I could remove DumpTimeSharedClassTable, but still, we should keep the ctor which initialize the two data members --- though they will be reset in update_counts. I will remove the comments.
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
@calvinccheung @iklam Thanks for review! |
Going to push as commit f741e4c.
Your commit was automatically rebased without conflicts. |
Hi, Please review
Currently after dynamic dump, dump time tables (
_dumptime_talbles, _dumptime_lambda_proxy_class_dictionary and _saved_shared_path_table
) are corrupted and could not be used for next dump. The patch clones the three tables, and after dump restore them so the next dump is possible. With the fix, jcmd VM.cds dynamic_dump can do multiple dump to the same live process.Tests: tier1,tier2,tier3,tier4
Thanks
Yumin
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4646/head:pull/4646
$ git checkout pull/4646
Update a local copy of the PR:
$ git checkout pull/4646
$ git pull https://git.openjdk.java.net/jdk pull/4646/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4646
View PR using the GUI difftool:
$ git pr show -t 4646
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4646.diff