RFR: 8293336: AOT-linking of invokedynamic for lambda expression and string concat [v5]

Tue Oct 15 19:11:19 UTC 2024

On Tue, 15 Oct 2024 05:21:53 GMT, Ioi Lam <iklam at openjdk.org> 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