On Wed, 14 May 2025 15:38:06 GMT, Vladimir Kozlov <k...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - java.md updates from @rose00
>> - Resolved differences with CSR JDK-8356010
>
> src/hotspot/share/cds/cds_globals.hpp line 123:
>
>> 121: product(ccstr, AOTCacheOutput, nullptr,
>> \
>> 122: "Write AOT cache into this file (overrides AOTCache when "
>> \
>> 123: "writing)")
>> \
>
> This looks not complete description. And "override AOTCache .." is confusing.
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.
> src/hotspot/share/cds/metaspaceShared.cpp line 1150:
>
>> 1148: print_java_launcher(&ss);
>> 1149: const char* cmd = ss.freeze();
>> 1150: tty->print_cr("Launching child process %s to assemble AOT cache %s
>> using configuration %s", cmd, AOTCacheOutput, AOTConfiguration);
>
> 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?
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.
> src/hotspot/share/runtime/arguments.cpp line 3060:
>
>> 3058: }
>> 3059:
>> 3060: static JavaVMOption* get_last_aotmode_arg(const JavaVMInitArgs* args) {
>
> 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.
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.
> src/java.base/share/man/java.md line 4141:
>
>> 4139: mode should be used only as a "fail-fast" debugging aid to
>> check if your command-line
>> 4140: options are compatible with the AOT cache. An alternative is to
>> run your application with
>> 4141: `-XX:AOTMode=auto -Xlog:cds,aot` to see if the AOT cache can be
>> used or not.
>
> `-Xlog:aot`
`-Xlog:cds,aot` is correct for the current state of this PR. I'll change it
after JDK-8356595 is integrated into the mainline.
> test/hotspot/jtreg/runtime/cds/appcds/aotFlags/AOTFlags.java line 166:
>
>> 164: "-XX:-AOTClassLinking",
>> 165: "-XX:AOTConfiguration=" + aotConfigFile,
>> 166: "-Xlog:cds=debug",
>
> `-Xlog:aot`
> I assume JDK-8356595: "Convert -Xlog:cds to -Xlog:aot " will be pushed first.
`-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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2090133925
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2090133649
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2090133612
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2090133585
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2090133557
