RFR: 8355798: Implement JEP 514: Ahead-of-Time Command Line Ergonomics [v3]

[Ashutosh Mehra]

On Mon, 5 May 2025 01:00:31 GMT, Ioi Lam <iklam at 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