RFR: 8329706: Implement -XX:+AOTClassLinking [v3]

[Ioi Lam]

On Tue, 10 Sep 2024 21:31:31 GMT, Ashutosh Mehra <asmehra at 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