On Mon, 23 Sep 2024 17:07:56 GMT, Ioi Lam <[email protected]> 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 with a new target base due to a merge or
> a rebase. The pull request now contains 15 commits:
>
> - Merge branch
> 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into
> jep-483-step-03-8329706-implement-xx-aot-class-linking
> - @dholmes-ora comments
> - @dholmes-ora comments
> - Fixed ZERO build
> - minor comment fix
> - @ashu-mehra comment: move code outside of call_initPhase2(); also renamed
> BOOT/BOOT2 to BOOT1/BOOT2 and refactored code related to
> AOTLinkedClassCategory
> - @ashu-mehra reviews
> - @ashu-mehra comments
> - @adinn comments
> - @dholmes-ora comments: logging indents
> - ... and 5 more: https://git.openjdk.org/jdk/compare/49dbfa6a...6029b35f
Spotted a few nits.
src/hotspot/share/cds/metaspaceShared.cpp line 1536:
> 1534: if (lsh.is_enabled()) {
> 1535: lsh.print("Using AOT-linked classes: %s (static archive: %s
> aot-linked classes",
> 1536: CDSConfig::is_using_aot_linked_classes() ? "true" :
> "false",
Suggestion:
`BOOL_TO_STR(CDSConfig::is_using_aot_linked_classes()),`
test/hotspot/jtreg/runtime/cds/appcds/jvmti/ClassFileLoadHookTest.java line 100:
> 98: TestCommon.checkExec(out);
> 99:
> 100: // JEP 483: if dumped with -XX:+AOTClassLinking, cannot use
> archive when CFLH
Suggestion:
`..., cannot use archive when CFLH is enabled`
test/lib/jdk/test/lib/cds/CDSAppTester.java line 50:
> 48: public CDSAppTester(String name) {
> 49: if (CDSTestUtils.DYNAMIC_DUMP) {
> 50: throw new jtreg.SkippedException("Tests based on CDSAppTester
> should be excluded when -Dtest.dynamic.cds.archive is specified");
You could omit the `jtreg.` in the above throw statement.
-------------
PR Review: https://git.openjdk.org/jdk/pull/20843#pullrequestreview-2322962760
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1771865540
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1771878531
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1771871838
