On Tue, 15 Oct 2024 05:21:53 GMT, Ioi Lam <[email protected]> wrote:
>> This is the 7th and final PR for [JEP 483: Ahead-of-Time Class Loading &
>> Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>>
>> This PR implements the AOT-linking of invokedynamic callsites:
>> - We only link lambda expressions (`LambdaMetafactory::metafactory`) and
>> string concat (`StringConcatFactory::makeConcatWithConstants()`) as the
>> results of these bootstrap methods do not have environment dependencies and
>> can be safely cached.
>> - The resolved CallSites are represented as Java heap objects. Thus, this
>> optimizations is supported only for the static CDS archive, which can store
>> heap objects. The dynamic CDS archive is not supported.
>>
>> **Review Notes:**
>>
>> - Start with `AOTConstantPoolResolver::preresolve_indy_cp_entries()` -- it
>> checks all indys that were linked during the training run, and aot-links
>> only those for lambdas and string concats
>> - `SystemDictionaryShared::find_all_archivable_classes()` finds all the
>> hidden classes that are reachable from the indy CallSites
>> - In `MetaspaceShared::preload_and_dump_impl()` we call
>> `MethodType::createArchivedObjects()` to record all MethodTypes that were
>> created in the assembly phase. This is necessary because the identity of
>> MethodTypes is significant (they are compared with the `==` operator). Also
>> see MethodType.java for the corresponding code that are used in the
>> production run.
>> - Afterwards, we enter the safepoint (`VM_PopulateDumpSharedSpace::doit()`).
>> In this safepoint,
>> `ConstantPoolCache::remove_resolved_indy_entries_if_non_deterministic()` is
>> called to remove any resolved indy callsites that cannot be archived. (Such
>> CallSites may be created as a side effect of Java code execution in the
>> assembly phase. E.g., the bootstrap of the module system).
>>
>> **Rough Edges:**
>>
>> - Many archived CallSites references (directly or indirectly) the static
>> fields of the classes listed under
>> `AOTClassInitializer::can_archive_initialized_mirror()`, where the object
>> identity of these static fields is significant. Therefore, we must preserve
>> the initialized states of these classes. Otherwise, we might run into
>> problems such as [JDK-8340836](https://bugs.openjdk.org/browse/JDK-8340836).
>> Unfortunately, the list is created by trial-and-error, and may need to be
>> updated to match changes in the `java.lang.invoke` and
>> `jdk.internal.constant` packages. We expect Project Leyden to come with a
>> general solution to this problem.
>> - If the identity is significant for a static field in a complex class, but
>> not a...
>
> Ioi Lam has updated the pull request with a new target base due to a merge or
> a rebase. The pull request now contains 29 commits:
>
> - @DanHeidinga comments -- added ConcurrentHashMap::runtimeSetup() to init
> NCPU to runtime value; also use the same runtimeSetup() pattern to call
> registerNatives() for Class.java and Unsafe.java
> - Merge branch 'jep-483-step-06-8311071-avoid-soft-refs-in-java-lang-invoke'
> into jep-483-step-07-8293336-store-lambda-forms-in-cds-archive
> - Fixed JDK-8341988: jstack launched with AOT cache created with
> -XX:+AOTClassLinking crashes
> - 8341600: [premain] Automatic aot-init of classes used by java.lang.invoke
> - @adinn comments
> - improve checks for not changing <clinit> order for aot linking of lambda;
> added comprehensive test cases: AOTLinkedLambdasApp::testClinitOrder()
> - Clean up of aotClassInitializer and cdsHeaVerifier; added lambda test
> cases for <clinit> order of app classes
> - Merge branch 'jep-483-step-06-8311071-avoid-soft-refs-in-java-lang-invoke'
> into jep-483-step-07-8293336-store-lambda-forms-in-cds-archive
> - Require all <clinit> of supertypes of aot-inited classes to be executed in
> assembly phase
> - Limit the use of AOTHolder
> - ... and 19 more: https://git.openjdk.org/jdk/compare/e46b910a...382446d4
src/hotspot/share/cds/heapShared.cpp line 457:
> 455: if (HeapShared::is_archivable_hidden_klass(ik)) {
> 456: // We can't rerun the <clinit> method of hidden classes as we don't
> save
> 457: // the classData, so we must archive its mirror in initialized state.
Is this comment still accurate? It looks like we do save the class_data on
line 506
src/hotspot/share/cds/heapShared.cpp line 1140:
> 1138: return false;
> 1139: }
> 1140: }
This is conservatively correct in that it checks all implemented interfaces.
We could be less conservative and only check interfaces that have a
"non-abstract non-static" (aka default) method as those are the interfaces that
will be initialized along with the class as per JVMS 5.5
src/hotspot/share/cds/metaspaceShared.cpp line 873:
> 871: Symbol* method_sig = vmSymbols::void_method_signature();
> 872: JavaCalls::call_static(&result, vmClasses::Class_klass(),
> 873: method_name, method_sig, CHECK);
Is this a good candidate for a `runtimeResolve` helper method? Can we roll it
into the same mechanism as the other `runtimeResolve` classes use?
src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java line
599:
> 597:
> 598: /** Number of CPUS, to place bounds on some sizings */
> 599: static @Stable int NCPU;
I would prefer to not mark this `@Stable` at this time as it would have
different assembly and runtime values and instead add a followup RFE to
investigate adding it in later.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1801391257
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1801410321
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1801597570
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1801777860
