-
Couldn't load subscription status.
- Fork 6.1k
8334898: Resolve static field/method references at CDS dump time #27958
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
|
👋 Welcome back asmehra! A progress list of the required criteria for merging this PR into |
|
@ashu-mehra 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 47 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 |
|
@ashu-mehra 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
|
|
@iklam can you please 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.
LGTM. I am testing this PR in our CI.
|
The zero builds fail with the following: |
|
Other than the zero build failures, all other tests passed in our CI. |
VM_Version::supports_fast_class_init_checks() is false Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
…ast_class_init_checks() is false Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
|
@iklam Zero does not support fast class init checks, so the CP entries for static bytecodes should not be resolved. I have pushed changes to do that. |
remove_resolved_method_entries_if_non_deterministic() Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
|
@iklam can you please review the new changes. With this update zero config builds fine. |
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.
Update looks good!
| resolved_klass->name()->as_C_string(), | ||
| (rme->is_resolved(Bytecodes::_invokestatic) ? " *** static" : "")); | ||
| } | ||
| ArchiveBuilder::alloc_stats()->record_method_cp_entry(archived, resolved && !archived); |
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 certainly looks right to move this outside the test for logging being enabled but does it make any practical difference i.e.is the statistic used for anything other than this logging category?
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.
Nope, I don't it affects anything other than the stats for logging. I think this was just a bug.
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 to me.
|
@adinn if it looks okay, can you please approve it. |
This patch pre-resolves constant pool entries referred by getstatic, putstatic and invokestatic bytecodes in the assembly phase.
It also extends ResolvedConstants.java test to run in AOT mode workflow and additional test for checking resolution of static CP entries.
Before this PR, stats on pre-resolved CP entries in the assembly phase reported by -Xlog:aot=info:
After this PR:
Update: The stats quoted above are for a simple HelloWorld program
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27958/head:pull/27958$ git checkout pull/27958Update a local copy of the PR:
$ git checkout pull/27958$ git pull https://git.openjdk.org/jdk.git pull/27958/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27958View PR using the GUI difftool:
$ git pr show -t 27958Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27958.diff
Using Webrev
Link to Webrev Comment