On Tue, 10 Sep 2024 00:53:32 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:
>
> @dholmes-ora comments: logging indents
src/hotspot/share/cds/aotClassLinker.cpp line 149:
> 147: add_candidate(ik);
> 148:
> 149: if (log_is_enabled(Info, cds, aot, load)) {
Is `load` the correct log tag to use in this class? Can it be replaced with
`link` tag?
src/hotspot/share/cds/aotClassLinker.hpp line 111:
> 109:
> 110: static int num_app_initiated_classes();
> 111: static int num_platform_initiated_classes();
I don't see these methods (num_app_initiated_classes and
num_platform_initiated_classes) used anywhere. Should they be removed?
src/hotspot/share/cds/aotConstantPoolResolver.cpp line 111:
> 109:
> 110: if (CDSConfig::is_dumping_aot_linked_classes()) {
> 111: if (AOTClassLinker::try_add_candidate(ik)) {
Are we relying on the call to `try_add_candidate` to add the class to the
candidate list? I guess that shouldn't be the case as the class have already
been added through
ArchiveBuilder::gather_klasses_and_symbols()->AOTClassLinker::add_candidates().
If so can we use AOTClassLinker::is_candidate(ik) here?
src/hotspot/share/cds/archiveBuilder.cpp line 766:
> 764: #define ADD_COUNT(x) \
> 765: x += 1; \
> 766: x ## _a += aotlinked;
Can we do this instead:
```x ## _a += (aotlinked ? 1 : 0)```
and make `aotlinked` a bool.
src/hotspot/share/cds/archiveBuilder.cpp line 779:
> 777: DECLARE_INSTANCE_KLASS_COUNTER(num_app_klasses);
> 778: DECLARE_INSTANCE_KLASS_COUNTER(num_hidden_klasses);
> 779: DECLARE_INSTANCE_KLASS_COUNTER(num_unlinked_klasses);
Nit-picking here - "unlinked" category doesn't need the "aot-linked" counter.
src/hotspot/share/cds/filemap.cpp line 2455:
> 2453: const char* prop =
> Arguments::get_property("java.system.class.loader");
> 2454: if (prop != nullptr) {
> 2455: if (has_aot_linked_classes()) {
Should this check be part of `FileMapInfo::validate_aot_class_linking`?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1752750296
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1752692333
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1752689932
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1752770372
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1752766131
PR Review Comment: https://git.openjdk.org/jdk/pull/20843#discussion_r1752780405
