On Wed, 18 Sep 2024 02:57:47 GMT, Ioi Lam <ik...@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
