-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8368174: Proactive initialization of @AOTSafeClassInitializer classes #27402
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
8368174: Proactive initialization of @AOTSafeClassInitializer classes #27402
Conversation
|
👋 Welcome back iklam! A progress list of the required criteria for merging this PR into |
|
@iklam 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 31 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 |
Webrevs
|
| return supertype->has_aot_safe_initializer(); | ||
| }); | ||
| } else { | ||
| // @AOTRuntimeSetup only meaningful in @AOTClassInitializer |
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.
| // @AOTRuntimeSetup only meaningful in @AOTClassInitializer | |
| // @AOTRuntimeSetup only meaningful in @AOTSafeClassInitializer |
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.
Fixed.
| // constant pool entries that were resolved during the training run. | ||
| FinalImageRecipes::apply_recipes(CHECK); | ||
|
|
||
| // Because the AOT assembly phase does not run the exact code as in the |
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.
| // Because the AOT assembly phase does not run the exact code as in the | |
| // Because the AOT assembly phase does not run the exact same code as in the |
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.
Fixed.
| /// This is usually the result of performing AOT optimizations for the | ||
| /// `java.lang.invoke` package. |
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 usually the result of performing AOT optimizations for the | |
| /// `java.lang.invoke` package. | |
| /// At present this is usually the result of performing AOT optimizations for | |
| /// the `java.lang.invoke` package but the set of classes which may be | |
| /// pre-initialized via this annotation is not restricted to just that case. |
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.
Under this this bullet point we are talking about classes that are initialized by other reasons, and not "via this annotation".
I changed the comment to the following to make it clear that the "At present ..." part is about the second scenario:
/// 1. If _X_ was initialized during the AOT training run, the JVM will proactively
/// initialize _X_ in the assembly phase.
/// 2. If _X_ was not initialized during the AOT training run, the initialization of
/// _X_ can still be triggered by normal execution of Java code in the assembly
/// phase. At present this is usually the result of performing AOT optimizations for
/// the `java.lang.invoke` package but it may include other cases as well.
What do you think?
adinn
left a comment
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 much simpler and clearer than the previous version making it easier for non-Leyden devs to understand what they are buying into when they use the annotation. Nice work @ashu-mehra and @iklam.
I made a few suggestions to clarify comments which you are free to adopt or drop as you see fit. Otherwise looks good to go.
| check_aot_annotations(ik); | ||
|
|
||
| if (!ik->is_initialized() && !ik->is_being_initialized()) { | ||
| if (ik->has_aot_safe_initializer()) { |
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.
Now that we are forcing initialization of classes annotated with AOTSafeClassInitializer, is it still possible that a class is not initialized but has_aot_safe_initializer() is true?
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's possible in some rare circumstances. I.e, in the assembly phase, we take a path that was not cover in the training run (e.g. some sort of advanced JLI linkage). We could load a class X without initializing it. This could happen if X is used only for instanceof checking, or if it was loaded during verification of other classes.
liach
left a comment
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 otherwise.
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.
Redundant change
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.
Fixed.
ashu-mehra
left a comment
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
|
@iklam |
|
Thanks @adinn @liach @ashu-mehra for the review |
|
Going to push as commit 17accf4.
Your commit was automatically rebased without conflicts. |
This is an alternative to #27024. Thanks to @ashu-mehra for the suggestion.
Background:
The AOT Assembly Phase is in essence a small Java program that executes a limited set of Java bytecodes. This program bootstraps the module system, loads classes, and performs certain ahead-of-time optimizations such as resolving
invokedynamiccall sites.As a side effect of Java program execution, a small set of Java classes are initialized in the Assembly Phase.
Since JDK-8360163, if a class
Xis annotated with@AOTSafeClassInitializerand is initialized in the Assembly Phase, thenXwill be stored in the AOT cache in the "initialized" state. When the AOT cache is used in the Production Run,X::<clinit>will not be executed, and the static variables ofXwill be available upon JVM bootstrap.Problem:
The Assembly Phase doesn't touch many classes that may benefit from
@AOTSafeClassInitializer. For example,jdk.internal.math.MathUtils::<clinit>creates a few large tables. CachingMathUtilsin the "initialized" state will improve start-up time. However, since no bytecodes executed by the Assembly Phase useMathUtils. it will not be initialized.Fix:
If a class
Xhas the@AOTSafeClassInitializerannotation and was initialized in the AOT Training Run, the JVM will proactively initializeXin the Assembly Phase. This will ensure thatXwill be cached in the "initialized" state.As a proof of concept,
@AOTSafeClassInitializeris added toMathUtils.@AOTSafeClassInitializerwill be added to more classes in future RFEs.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27402/head:pull/27402$ git checkout pull/27402Update a local copy of the PR:
$ git checkout pull/27402$ git pull https://git.openjdk.org/jdk.git pull/27402/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27402View PR using the GUI difftool:
$ git pr show -t 27402Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27402.diff
Using Webrev
Link to Webrev Comment