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

[Andrew Dinn]

On Tue, 10 Sep 2024 00:53:32 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:
> 
>   @dholmes-ora comments: logging indents

Looks good as far as I can see -- although I only have a limited familiarity with the CDS archive code.

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

> 69: //
> 70: class AOTClassLinker :  AllStatic {
> 71:   using ClassesTable = ResourceHashtable<InstanceKlass*, bool, 15889, AnyObj::C_HEAP, mtClassShared>;

Can we have a symbolic name for this (prime) magic number here and in other places in this patch? I realise there is existing code which uses the raw number bit it is also consumed symbolically (e.g. in archiveBuilder.hpp, metaspaceClosure.hpp)

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

> 189:   }
> 190: 
> 191:   ClassLoaderData* loader_data = ClassLoaderData::class_loader_data(loader());

Can we assert here that loader() != nullptr?

src/hotspot/share/cds/archiveBuilder.cpp line 433:

> 431:   }
> 432: 
> 433:   remember_embedded_pointer_in_enclosing_obj(ref);

I'm not clear why this was moved up. Was this just an omission (bug) in the earlier version or do we now need to remember a reference location that we could previously safely ignore?

src/hotspot/share/cds/cdsConfig.cpp line 551:

> 549: }
> 550: 
> 551: void CDSConfig::set_has_aot_linked_classes(bool is_static_archive, bool has_aot_linked_classes) {

Why does this need to take `is_static_archive` as an argument?

src/hotspot/share/cds/dynamicArchive.cpp line 138:

> 136:     verify_estimate_size(_estimated_metaspaceobj_bytes, "MetaspaceObjs");
> 137: 
> 138:     sort_methods();

Could we have a comment to note that sorting and making shareable need to be done before calling `AOTClassLinker::write_to_archive();`

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

Marked as reviewed by adinn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20843#pullrequestreview-2291991335
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1751666092
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1751745625
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1751916420
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1751952246
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1751964685