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

[Andrew Dinn]

On Mon, 23 Sep 2024 18:45:49 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 all of the static fields of this cl...

src/hotspot/share/cds/aotConstantPoolResolver.cpp line 450:

> 448: 
> 449:   Symbol* mh_sig = cp->method_handle_signature_ref_at(mh_index);
> 450:   if (!check_lambda_metafactory_signature(cp, mh_sig, false)) {

This appears to be rerunning the previous check without a log message -- or am I missing something here?

src/hotspot/share/cds/archiveBuilder.cpp line 234:

> 232:       _klasses->append(klass);
> 233:       if (klass->is_hidden() && klass->is_instance_klass()) {
> 234:         update_hidden_class_loader_type(InstanceKlass::cast(klass));

Can you explain why this 'update' is needed. Are the shared classloader type and classpath index not already set? Or do they need adjusting? Perhaps a comment would help.

src/hotspot/share/cds/cdsConfig.cpp line 486:

> 484: bool CDSConfig::allow_only_single_java_thread() {
> 485:   // See comments in JVM_StartThread()
> 486:   return is_dumping_static_archive();

The test in `JVM_StartThread()` calls `CDSConfig::is_dumping_static_archive()`. Should it be updated to call `CDSConfig::allow_only_single_java_thread()`?

src/hotspot/share/cds/cdsHeapVerifier.cpp line 126:

> 124: 
> 125:   // Integer for 0 and 1 are in java/lang/Integer$IntegerCache and are archived
> 126:   ADD_EXCL("sun/invoke/util/ValueConversions",           "ONE_INT",                // E

At line 46 the example that explains how the verifier works discusses save and restore of field `Foo#archivedFoo`. I believe that field was supposed to be declared as `static` but it is actually declared as an instance field.

src/hotspot/share/cds/cdsHeapVerifier.cpp line 274:

> 272:     char* class_name = info->_holder->name()->as_C_string();
> 273:     char* field_name = info->_name->as_C_string();
> 274:     if (strstr(class_name, "java/lang/invoke/BoundMethodHandle$Species_") == class_name &&

Can we have a comment to explain this special case?

src/hotspot/share/cds/classListParser.cpp line 609:

> 607:   // We store a pre-generated version of the lambda proxy class in the AOT cache,
> 608:   // which will be loaded via JVM_LookupLambdaProxyClassFromArchive().
> 609:   // This eliminate dynamic class generation of the proxy class, but we still need to

"This eliminate" --> "This eliminates"

src/hotspot/share/cds/heapShared.cpp line 893:

> 891: void KlassSubGraphInfo::check_allowed_klass(InstanceKlass* ik) {
> 892:   if (CDSConfig::is_dumping_invokedynamic()) {
> 893:     // We allow LambdaProxy classes in platform and app loaders as well.

Do we not want an assert here to describe the extra classes we are allowing?

Also, does the comment at line 1975 now need to be updated -- it refers to this check which has been widened.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1790514322
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1791663789
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1791897685
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1791917239
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1791947118
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1791952296
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1792002443