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

Wed Sep 18 05:40:11 UTC 2024

On Wed, 18 Sep 2024 02:57:47 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:
> 
>   Fixed ZERO build

I have taken another pass through. A few queries and small items.

Thanks

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

> 64: 
> 65: void AOTLinkedClassBulkLoader::load_classes_in_loader(JavaThread* current, AOTLinkedClassCategory class_category, oop class_loader_oop) {
> 66:   ExceptionMark em(current);

Why do you need the EM when you are explicitly checking for exceptions?

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

> 65: void AOTLinkedClassBulkLoader::load_classes_in_loader(JavaThread* current, AOTLinkedClassCategory class_category, oop class_loader_oop) {
> 66:   ExceptionMark em(current);
> 67:   ResourceMark rm(current);

The RM should go where it is actually needed for the logging.

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

> 66:   ExceptionMark em(current);
> 67:   ResourceMark rm(current);
> 68:   HandleMark hm(current);

Why do you need a HM here?

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

> 93: 
> 94:   if (Universe::is_fully_initialized() && VerifyDuringStartup) {
> 95:     // Make sure we're still in a clean slate.

Suggestion:

    // Make sure we're still in a clean state.

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

> 130:     break;
> 131:   case AOTLinkedClassCategory::UNREGISTERED:
> 132:     ShouldNotReachHere(); // Currently aot-linked classes are not supported for this category.

Suggestion:

  case AOTLinkedClassCategory::UNREGISTERED:
  default:
    ShouldNotReachHere(); // Currently aot-linked classes are not supported for this category.

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

> 168:           log_error(cds)("Unable to resolve %s class from CDS archive: %s", category_name, ik->external_name());
> 169:           log_error(cds)("Expected: " INTPTR_FORMAT ", actual: " INTPTR_FORMAT, p2i(ik), p2i(actual));
> 170:           log_error(cds)("JVMTI class retransformation is not supported when archive was generated with -XX:+AOTClassLinking.");

Nit: use a `logStream` instead of the three separate calls.

src/hotspot/share/cds/aotLinkedClassTable.hpp line 34:

> 32: class SerializeClosure;
> 33: 
> 34: // Classes to be buik-loaded, in the "linked" state, at VM bootstrap.

Suggestion:

// Classes to be bulk-loaded, in the "linked" state, at VM bootstrap.

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

> 314: 
> 315:   if (CDSConfig::is_dumping_aot_linked_classes()) {
> 316:     _estimated_hashtable_bytes += _klasses->length() * 16 * sizeof(Klass*);

Why 16?

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

> 875:       if (ik->is_hidden()) {
> 876:         ADD_COUNT(num_hidden_klasses);
> 877:         hidden = " hidden";

Why not do this at the same time you do the other hidden class updates above?

src/hotspot/share/cds/cds_globals.hpp line 99:

> 97:                                                                             \
> 98:   /*========== New "AOT" flags =========================================*/  \
> 99:   /* The following 3 flags are aliases of -Xshare:dump, */                  \

Nit: align the `*/`.

src/hotspot/share/classfile/systemDictionary.cpp line 139:

> 137:   if (_java_platform_loader.is_empty()) {
> 138:     oop platform_loader = get_platform_class_loader_impl(CHECK);
> 139:     _java_platform_loader = OopHandle(Universe::vm_global(), platform_loader);

Why has the order been switched here?

test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/AOTClassLinkingVMOptions.java line 57:

> 55:         testCase("Archived full module graph must be enabled at runtime");
> 56:         TestCommon.run("-cp", appJar, "-Djdk.module.validation=1", "Hello")
> 57:             .assertAbnormalExit("CDS archive has aot-linked classes." +

Nit: align the dots

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

PR Review: https://git.openjdk.org/jdk/pull/20843#pullrequestreview-2311449272
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764390816
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764391068
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764391282
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764392237
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764393538
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764394331
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764395970
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764396906
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764402272
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764405309
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764412019
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1764414682