On Tue, 23 Aug 2022 16:27:40 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @shipilev review comments
>
> make/Images.gmk line 132:
>
>> 130:
>> 131: # Only G1 supports dumping the shared heap, so explicitly use G1
>> 132: # it if the JVM supports it. (Note: the default GC with zero is
>> SerialGC)
>
> Suggestion:
>
> # Only G1 supports dumping the shared heap, so explicitly use G1 if the JVM
> supports it.
Fixed.
> make/Images.gmk line 133:
>
>> 131: # Only G1 supports dumping the shared heap, so explicitly use G1
>> 132: # it if the JVM supports it. (Note: the default GC with zero is
>> SerialGC)
>> 133: $1_$2_CDS_DUMP_FLAGS := $(CDS_DUMP_FLAGS) $(if $(filter g1gc,
>> $(JVM_FEATURES_$1)),-XX:+UseG1GC)
>
> I think it should be e.g. (untested):
>
> Suggestion:
>
> $1_$2_CDS_DUMP_FLAGS := $(CDS_DUMP_FLAGS)
> ifeq ($(call check-jvm-feature, g1gc), true)
> $1_$2_CDS_DUMP_FLAGS += -XX:+UseG1GC
> endif
That doesn't work because `check-jvm-feature` requires `JVM_VARIANT` to be set,
but `CreateCDSArchive` is not called in a context where `JVM_VARIANT` is set.
( `JVM_VARIANT` is set only in a few specific places in Main.gmk, etc).
One option is to change the foreach loop a few lines below to:
$(foreach JVM_VARIANT, $(JVM_VARIANTS), \
$(eval $(call CreateCDSArchive,)) \
)
But I've not seen `JVM_VARIANT` being used this way, so I am a little hesitant
about doing it.
-------------
PR: https://git.openjdk.org/jdk/pull/9984
