On Mon, 5 May 2025 01:00:31 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> This is the implementation of the draft [JEP: Ahead-of-time Command Line
>> Ergonomics](https://bugs.openjdk.org/browse/JDK-8350022)
>>
>> - Implemented new flag `AOTCacheOutput`, which can be used to create an AOT
>> cache using the "one-command workflow"
>> - Added processing of the `JAVA_AOT_OPTIONS` environment variable that can
>> supply extra VM options when creating an AOT cache
>> - Added `%p` substitution for `AOTCache`, `AOTCacheOutput`, and
>> `AOTConfiguration` options
>>
>> Please see the [JEP](https://bugs.openjdk.org/browse/JDK-8350022) and
>> [CSR](https://bugs.openjdk.org/browse/JDK-8356010) for detailed
>> specification.
>>
>> Examples:
>>
>>
>> # Create an AOT cache with a single command:
>> $ java -cp HelloWorld.jar -XX:AOTMode=record -XX:AOTCacheOutput=foo.aot
>> HelloWorld
>> Hello World
>> Temporary AOTConfiguration recorded: foo.aot.config
>> Launching child process /usr/bin/java to assemble AOT cache foo.aot using
>> configuration foo.aot.config
>> Picked up JAVA_TOOL_OPTIONS: -Djava.class.path=HelloWorld.jar
>> -XX:AOTCacheOutput=foo.aot -XX:AOTConfiguration=foo.aot.config
>> -XX:AOTMode=create
>> Reading AOTConfiguration foo.aot.config and writing AOTCache foo.aot
>> AOTCache creation is complete: foo.aot 10240000 bytes
>>
>> # Create logging file for the AOT cache assembly phase
>> $ export AOT_TOOL_COMMAND=-Xlog:cds:file=log.txt
>> $ java -cp HelloWorld.jar -XX:AOTMode=record -XX:AOTCacheOutput=foo.aot
>> HelloWorld
>>
>>
>> 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).
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> @vnkozlov comments
src/hotspot/share/cds/cdsConfig.cpp line 428:
> 426:
> 427: // At least one AOT flag has been used
> 428: _new_aot_flags_used = true;
indentation seems off?
src/hotspot/share/cds/cdsConfig.cpp line 490:
> 488: bool has_output = !FLAG_IS_DEFAULT(AOTCacheOutput);
> 489:
> 490: if (has_output) {
Can this if-else block be refactored as:
if (!has_output && !has_config) {
vm_exit_during_initialization("...");
}
if (has_output) {
}
This pattern in much easier to read IMO.
src/hotspot/share/cds/cdsConfig.cpp line 504:
> 502: } else {
> 503: if (!has_config) {
> 504: vm_exit_during_initialization("-XX:AOTMode=record cannot be used
> without setting AOTCacheOutput or AOTConfiguration");
I suggest to be consistent in phrasing the error messages related to incomplete
options. I liked the way it is worded in `check_aotmode_create()`. So my
preference is to replace `-XX:AOTMode=record cannot be used without setting
AOTCacheOutput or AOTConfiguration` with `AOTCacheOutput or AOTConfiguration
must be specified when using -XX:AOTMode=record`. But you may chose otherwise
as long as the structure of the error messages is consistent.
src/hotspot/share/cds/cdsConfig.cpp line 526:
> 524: void CDSConfig::check_aotmode_create() {
> 525: if (FLAG_IS_DEFAULT(AOTConfiguration)) {
> 526: vm_exit_during_initialization("-XX:AOTMode=create cannot be used
> without setting AOTConfiguration");
Same suggestion regarding error msg:
Replace `-XX:AOTMode=create cannot be used without setting AOTConfiguration`
with
`AOTConfiguration must be specified when using -XX:AOTMode=create`
src/hotspot/share/cds/metaspaceShared.cpp line 1082:
> 1080: for (int i = 0; i < Arguments::num_jvm_args(); i++) {
> 1081: const char* arg = Arguments::jvm_args_array()[i];
> 1082: if (strncmp("-XX:AOTCacheOutput=", arg, 19) == 0 ||
I have seen this hard coding of string length in the `strncmp` call many times
in the code base. I wonder why we don't use something like this:
strncmp("-XX:AOTCacheOutput=", arg, sizeof("-XX:AOTCacheOutput=")-1)
Counting chars is really error prone. sizeof("...")-1 gives the same result.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076664964
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076665081
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076667404
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076667613
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076667744
