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

[David Holmes]

On Tue, 17 Sep 2024 23:29:46 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> This is the 3rd PR for [JEP 483: Ahead-of-Time Class Loading & Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>> 
>> **Overview**
>> 
>> - A new `-XX:+AOTClassLinking` flag is added. See [JEP 498](https://bugs.openjdk.org/browse/JDK-8315737) and the [CSR](https://bugs.openjdk.org/browse/JDK-8339506) for a discussion of this command-line option, its default value, and its impact on compatibility.
>> - When this flag is enabled during the creation of an AOT cache (aka CDS archive), an `AOTLinkedClassTable` is added to the cache to include all classes that are AOT-linked. For this PR, only classes for the boot/platform/application loaders are eligible. The main logic is in `aotClassLinker.cpp`.
>> - When an AOT archive is loaded in a production run, all classes in the `AOTLinkedClassTable` are loaded into their respective class loaders at the earliest opportunity. The main logic is in `aotLinkedClassBulkLoader.cpp`.
>>   - The boot classes are loaded as part of `vmClasses::resolve_all()`
>>   - The platform/application classes are loaded after the module graph is restored (see changes in `threads.cpp`).
>> - Since all classes in a `AOTLinkedClassTable` are loaded before any user-code is executed, we can resolve constant pool entries that refer to these classes during AOT cache creation. See changes in `AOTConstantPoolResolver::is_class_resolution_deterministic()`.
>> 
>> **All-or-nothing Loading**
>> 
>> - Because AOT-linked classes can refer to each other, using direct C++ pointers, all AOT-linked classes must be loaded together. Otherwise we will have dangling C++ pointers in the class metadata structures. 
>> - During a production run, we check during VM start-up for incompatible VM options that would prevent some of the AOT-linked classes from being loaded. For example:
>>   - If the VM is started with an JVMTI agent that has ClassFileLoadHook capabilities, it could replace some of the AOT-linked classes with alternative versions.
>>   - If the VM is started with certain module options, such as `--patch-module` or `--module`, some AOT-linked classes may be replaced with patched versions, or may become invisible and cannot be loaded into the JVM.
>> - When incompatible VM options are detected, the JVM will refuse to load an AOT cache that has AOT-linked classes. See `FileMapInfo::validate_aot_class_linking()`.
>>   - For simplfication, `FileMapInfo::validate_aot_class_linking()` requires `CDSConfig::is_using_full_module_graph()` to be true. This means that the exa...
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   minor comment fix

src/hotspot/share/cds/aotClassLinker.cpp line 121:

> 119:   assert(is_initialized(), "sanity");
> 120: 
> 121:   if (!CDSConfig::is_dumping_aot_linked_classes() || !SystemDictionaryShared::is_builtin(ik)) {

Shouldn't the CDSConfig check just be an assert - the caller is expected to check before trying to add candidates?

src/hotspot/share/cds/aotClassLinker.cpp line 145:

> 143:       return false;
> 144:     }
> 145:   }

Are we concerned with the possibility that we might be able to add some interfaces but not all, hence returning false, but with a subset of interfaces already added to the candidate list? I don't think it should be possible, but the code structure makes it look like it could be possible.

src/hotspot/share/cds/aotClassLinker.cpp line 191:

> 189:     if (ik->class_loader() != class_loader) {
> 190:       continue;
> 191:     }

This seems very inefficient. We call `write_classes` 4 times, potentially with different loaders. Because the candidates are sorted the classes belonging to the same loader are likely to be grouped due to package names. So the app loader classes are likely to be right at end, and we have to traverse all the boot/platform classes first before we get to them. Conversely after we have encountered the last boot loader class (for example) we keep the scanning the entire list. If the set were ordered based on loader then name, we would be able to stop once we see the loader change to not being the desired one. And a binaery search would let you find the start of a section more quickly.

src/hotspot/share/cds/aotClassLinker.cpp line 194:

> 192:     if ((ik->module() == ModuleEntryTable::javabase_moduleEntry()) != is_javabase) {
> 193:       continue;
> 194:     }

Why do we process system loader classes (i.e. application loader classes) if they need to be in java.base, as the application classes will never be in java.base. ???

src/hotspot/share/cds/aotClassLinker.cpp line 198:

> 196:     if (ik->is_shared() && CDSConfig::is_dumping_dynamic_archive()) {
> 197:       if (CDSConfig::is_using_aot_linked_classes()) {
> 198:         // This class was recorded as a AOT-linked for the base archive,

Suggestion:

        // This class was recorded as AOT-linked for the base archive,

src/hotspot/share/cds/aotClassLinker.cpp line 212:

> 210:   } else {
> 211:     const char* category = class_category_name(list.at(0));
> 212:     log_info(cds, aot, link)("written %d class(es) for category %s", list.length(), category);

Suggestion:

    log_info(cds, aot, link)("wrote %d class(es) for category %s", list.length(), category);

src/hotspot/share/cds/aotClassLinker.hpp line 60:

> 58: //     - The visibility of C
> 59: //
> 60: // During an Production Run, the JVM can use an AOTCache with an AOTLinkedClassTable

Suggestion:

// During a Production Run, the JVM can use an AOTCache with an AOTLinkedClassTable

src/hotspot/share/cds/aotClassLinker.hpp line 100:

> 98:   static bool is_vm_class(InstanceKlass* ik);
> 99: 
> 100:   // When CDS is enabled, is ik guatanteed to be linked at deployment time (and

Suggestion:

  // When CDS is enabled, is ik guaranteed to be linked at deployment time (and

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764280936
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764279963
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764292759
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764293783
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764293980
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764294382
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764295867
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764298973