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

[Ioi Lam]

On Wed, 7 May 2025 02:33:50 GMT, Ashutosh Mehra <asmehra at openjdk.org> wrote:

>> 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?

Fixed.

> 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.

Fixed.

> 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.

I changed to "At least one of AOTCacheOutput and AOTConfiguration must be specified when using -XX:AOTMode=record", so it's clear that both option can be specified together.

> 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`

Fixed.

> 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.

I end up using `strstr()`, as I don't want to repeat the string literal twice (risking a typo) or use a macro.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796934
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796911
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796869
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796827
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796802