On Tue, 10 Sep 2024 21:31:31 GMT, Ashutosh Mehra <asme...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dholmes-ora comments: logging indents
>
> src/hotspot/share/cds/aotClassLinker.cpp line 149:
>
>> 147: add_candidate(ik);
>> 148:
>> 149: if (log_is_enabled(Info, cds, aot, load)) {
>
> Is `load` the correct log tag to use in this class? Can it be replaced with
> `link` tag?
I changed to the `link` tag.
> src/hotspot/share/cds/aotClassLinker.hpp line 111:
>
>> 109:
>> 110: static int num_app_initiated_classes();
>> 111: static int num_platform_initiated_classes();
>
> I don't see these methods (num_app_initiated_classes and
> num_platform_initiated_classes) used anywhere. Should they be removed?
I added logging in DumpAllocStats::print_stats() to use these methods.
> src/hotspot/share/cds/aotConstantPoolResolver.cpp line 111:
>
>> 109:
>> 110: if (CDSConfig::is_dumping_aot_linked_classes()) {
>> 111: if (AOTClassLinker::try_add_candidate(ik)) {
>
> Are we relying on the call to `try_add_candidate` to add the class to the
> candidate list? I guess that shouldn't be the case as the class have already
> been added through
> ArchiveBuilder::gather_klasses_and_symbols()->AOTClassLinker::add_candidates().
> If so can we use AOTClassLinker::is_candidate(ik) here?
You're correct. I changed it to `AOTClassLinker::is_candidate(ik)`
> src/hotspot/share/cds/archiveBuilder.cpp line 766:
>
>> 764: #define ADD_COUNT(x) \
>> 765: x += 1; \
>> 766: x ## _a += aotlinked;
>
> Can we do this instead:
>
> ```x ## _a += (aotlinked ? 1 : 0)```
>
> and make `aotlinked` a bool.
Fixed.
> src/hotspot/share/cds/archiveBuilder.cpp line 779:
>
>> 777: DECLARE_INSTANCE_KLASS_COUNTER(num_app_klasses);
>> 778: DECLARE_INSTANCE_KLASS_COUNTER(num_hidden_klasses);
>> 779: DECLARE_INSTANCE_KLASS_COUNTER(num_unlinked_klasses);
>
> Nit-picking here - "unlinked" category doesn't need the "aot-linked" counter.
Fixed.
> src/hotspot/share/cds/filemap.cpp line 2455:
>
>> 2453: const char* prop =
>> Arguments::get_property("java.system.class.loader");
>> 2454: if (prop != nullptr) {
>> 2455: if (has_aot_linked_classes()) {
>
> Should this check be part of `FileMapInfo::validate_aot_class_linking`?
I put the check here so that the "Archived non-system classes are disabled
because ...." message will not be printed a few lines below. Otherwise the user
will see two different error messages that say different things.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622677
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622615
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622522
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622803
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622734
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1757622394
