-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8355798: Implement JEP 514: Ahead-of-Time Command Line Ergonomics #24942
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
8355798: Implement JEP 514: Ahead-of-Time Command Line Ergonomics #24942
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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
|
JEP and CSR call env var : |
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.
Few comments.
|
@iklam this pull request can not be integrated into git checkout 8355798-implement-leyden-ergo-jep-8350022
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
Webrevs
|
Fixed. |
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.
Good.
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.
Few comments.
| "Write AOT cache into this file (overrides AOTCache when " \ | ||
| "writing)") \ |
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 looks not complete description. And "override AOTCache .." is confusing.
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.
I changed it to "Specifies the file name for writing the AOT cache". Since the behavior is complex, I don't know how much detail I should put here.
| stringStream ss; | ||
| print_java_launcher(&ss); | ||
| const char* cmd = ss.freeze(); | ||
| tty->print_cr("Launching child process %s to assemble AOT cache %s using configuration %s", cmd, AOTCacheOutput, AOTConfiguration); |
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.
I noticed that AOT produces outputs on TTY like this unconditionally. I think it is fine for development but for production we should use UL I think.
Was this discussed?
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.
Currently we print things that are important to the user. The other examples are:
$ java -XX:AOTMode=record ....
AOTConfiguration recorded: hw.aotconfig
$ java -XX:AOTMode=create ....
Reading AOTConfiguration hw.aotconfig and writing AOTCache hw.aot
AOTCache creation is complete: hw.aot 10498048 bytes
I think AOT is still a new feature so a small number of TTY prints would be helpful. If people complain about the verbosity we can change them to UL logs.
| return parse_options_environment_variable("JAVA_TOOL_OPTIONS", args); | ||
| } | ||
|
|
||
| static JavaVMOption* get_last_aotmode_arg(const JavaVMInitArgs* args) { |
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.
I don't like that we pollute Arguments code with AOT specific flags processing.
Can we move this into CDSConfig? Both these 2 new methods.
But I will agree if you want to keep it here. It is not critical.
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.
These two functions are for parsing JDK_AOT_VM_OPTIONS, so I think they belong to arguments.cpp. cdsConfig.cpp is a consumer of the options parsed by arguments.cpp.
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.
okay
src/java.base/share/man/java.md
Outdated
| mode should be used only as a "fail-fast" debugging aid to check if your command-line | ||
| options are compatible with the AOT cache. An alternative is to run your application with | ||
| `-XX:AOTMode=auto -Xlog:cds` to see if the AOT cache can be used or not. | ||
| `-XX:AOTMode=auto -Xlog:cds,aot` to see if the AOT cache can be used or not. |
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.
-Xlog:aot
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.
-Xlog:cds,aot is correct for the current state of this PR. I'll change it after JDK-8356595 is integrated into the mainline.
| "-XX:AOTMode=record", | ||
| "-XX:-AOTClassLinking", | ||
| "-XX:AOTConfiguration=" + aotConfigFile, | ||
| "-Xlog:cds=debug", |
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.
-Xlog:aot
I assume JDK-8356595: "Convert -Xlog:cds to -Xlog:aot " will be pushed first.
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.
-Xlog:cds=debug is correct for the current state of this PR (otherwise the test will fail). I'll change it after JDK-8356595 is integrated into the mainline.
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.
| One way to improve test coverage of ahead-of-time (AOT) optimizations in the JDK is to | ||
| run existing jtreg test cases in a special "AOT_JDK" mode. Example: | ||
|
|
||
|
|
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.
Is this extra newline intentional?
doc/testing.md
Outdated
|
|
||
| Also, test cases that were written specifically to test AOT, such as the tests | ||
| under [test/hotspot/jtreg/runtime/cds](../test/hotspot/jtreg/runtime/cds/), cannot | ||
| be execute with the AOT_JDK mode. |
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.
| be execute with the AOT_JDK mode. | |
| be executed with the AOT_JDK mode. |
doc/testing.md
Outdated
| under [test/hotspot/jtreg/runtime/cds](../test/hotspot/jtreg/runtime/cds/), cannot | ||
| be execute with the AOT_JDK mode. | ||
|
|
||
| Valid values for `AOT_JDK` are `one_step` and `two_step`. These control how |
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 like the values are onestep and twostep in the makefile.
doc/testing.md
Outdated
| In this testing mode, we first perform an AOT training run (see https://openjdk.org/jeps/483) | ||
| of a special test program ([test/setup_aot/TestSetupAOT.java](../test/setup_aot/TestSetupAOT.java)) | ||
| that accesses about 5,0000 classes in the JDK core libraries. Optimization artifacts for | ||
| these classes (such as pre-linked lambda expressions, execution profiles, and pre-generated native code) | ||
| are stored into an AOT cache file, which will be used by all the JVMs launched by the selected jtreg | ||
| test cases. |
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.
Some of these lines are >80 chars. Can you try to format line lengths in this file to stay under where reasonably feasible? It's ok if long markups such as links go over, just try to keep the normal text adhering to this.
make/RunTests.gmk
Outdated
| $$($1_AOT_JDK_CACHE): $$(JDK_IMAGE_DIR)/release | ||
| $$(call MakeDir, $$($1_AOT_JDK_OUTPUT_DIR)) | ||
|
|
||
| ifeq ($$($1_TRAINING), onestep) |
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.
Sorry for not noticing this earlier. This is a conditional within a recipe. The preferred style for those are to align the ifeq, else and endif with the recipe itself, using spaces (as it's not a recipe line), and then indent the body with 2 extra spaces for logical indentation. See points 5 and 6 in https://openjdk.org/groups/build/doc/code-conventions.html. You can find an example of this in make/Bundles.gmk.
|
/integrate |
|
Going to push as commit dede353.
Your commit was automatically rebased without conflicts. |
This is the implementation of the draft JEP: Ahead-of-time Command Line Ergonomics
AOTCacheOutput, which can be used to create an AOT cache using the "one-command workflow"JDK_AOT_VM_OPTIONSenvironment variable that can supply extra VM options when creating an AOT cache%psubstitution forAOTCache,AOTCacheOutput, andAOTConfigurationoptionsPlease see the JEP and CSR for detailed specification.
Examples:
Note: the child process is launched with Java API because the HotSpot native APIs are not sufficient (no way to set env vars for child process).
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24942/head:pull/24942$ git checkout pull/24942Update a local copy of the PR:
$ git checkout pull/24942$ git pull https://git.openjdk.org/jdk.git pull/24942/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24942View PR using the GUI difftool:
$ git pr show -t 24942Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24942.diff
Using Webrev
Link to Webrev Comment