On Thu, 19 Sep 2024 21:43:54 GMT, Mat Carter <maca...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dholmes-ora comments: do not check for -XX:AOTMode=create in JLI java.c
>
> src/hotspot/share/cds/cds_globals.hpp line 102:
>
>> 100: /* See CDSConfig::check_flag_aliases().
>> */ \
>> 101:
>> \
>> 102: product(ccstr, AOTMode, nullptr,
>> \
>
> you can replace nullptr with "auto" as that's the default, that would
> simplify some of the FLAG_IS_DEFAULT(AOTMode) || (strcmp(AOTMode, "auto") ==
> 0 checks. So you would now check strcmp(AOTMode, "auto") == 0 which I guess
> is slightly less performant (as FLAG_IS_DEFAULT is a cheap early out),
> however the current implementation means that the definition of the default
> value can spread across the codebase vs only being specified in
> cds_globals.hpp
If AOTMode has the "auto" value by default, that will be inconsistent if the
JVM is started with old parameters like `-Xshare:dump` and `-Xshare:off`. I
think it's better to leave it as null to avoid confusions (especially when
running inside gdb, etc).
> src/hotspot/share/cds/metaspaceShared.cpp line 673:
>
>> 671: }
>> 672:
>> 673: if (AOTMode != nullptr && strcmp(AOTMode, "create") == 0) {
>
> Would it be better to wrap this test in a CDSConfig::is_ method; which in
> turn can test the AOTMode FLAG directly or test
> (CDSConfig::is_dumping_static_archive() &&
> !CDSConfig::is_old_cds_flags_used()), the point being that the meaning of the
> test is captured in the method name, but the test itself can change over time
> as needed
I added `CDSConfig::is_old_cds_flags_used()` and edited the comment. There's no
need to check for `CDSConfig::is_dumping_static_archive()` as this function is
called only when dumping the static archive.
(The "if" check may need further tweaking when merging into the Leyden repo,
which has several modes of "static" archive dumping).
> test/hotspot/jtreg/runtime/cds/appcds/AOTFlags.java line 51:
>
>> 49: }
>> 50:
>> 51: static void positiveTests() throws Exception {
>
> Missing a test for AOTMode=on
Added new test.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20516#discussion_r1771832594
PR Review Comment: https://git.openjdk.org/jdk/pull/20516#discussion_r1771832798
PR Review Comment: https://git.openjdk.org/jdk/pull/20516#discussion_r1771832634
