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

[iklam]

This is the 7th and final PR for JEP 483: Ahead-of-Time Class Loading & Linking.

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. 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 class can be archived, I use the AOTHolder pattern to break out the fields that need to be archived. See discussion on leyden-dev.

---

See here for the sequence of dependent RFEs for implementing JEP 483.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Integration blocker

 ⚠️ Dependency #21049 must be integrated first

Issue

• JDK-8293336: AOT-linking of invokedynamic for lambda expression and string concat (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21143/head:pull/21143
$ git checkout pull/21143

Update a local copy of the PR:
$ git checkout pull/21143
$ git pull https://git.openjdk.org/jdk.git pull/21143/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21143

View PR using the GUI difftool:
$ git pr show -t 21143

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21143.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into pr/21049 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

⚠️ @iklam This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[openjdk]

@iklam The following labels will be automatically applied to this pull request:

• core-libs
• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[rose00]

I read through it fairly thoroughly. I made a lot of suggestions. Rather than edit comments into lots of spots in this PR, I made the following patch file.
aot-indy-21143-commentary.diff.txt

The first lines of the patch file state which revision the patch applies to, which is 4be6a25 from Tuesday.

[dholmes-ora]

I read through it fairly thoroughly. I made a lot of suggestions. Rather than edit comments into lots of spots in this PR, I made the following patch file. aot-indy-21143-commentary.diff.txt

+bool InstanceKlass::is_enum_subclass(bool direct_only) const {
+  InstanceKlass* s = java_super();
+  return (s == vmClasses::Enum_klass() ||
+          (s != null && s->java_super() == vmClasses::Enum_klass()));
+}

Just happened to notice you aren't checking direct_only.

[iklam]

I read through it fairly thoroughly. I made a lot of suggestions. Rather than edit comments into lots of spots in this PR, I made the following patch file. aot-indy-21143-commentary.diff.txt

The first lines of the patch file state which revision the patch applies to, which is 4be6a25 from Tuesday.

Hi John,

Thank you so much for the code. I have merged most of your diffs into the PR. There are a few items left which I will go over tomorrow. I simplified the InstanceKlass::interface_needs_clinit_execution_as_super() as I don't need to check against non-interface classes, so I worry about adding code that's not used by this JEP.

I also moved the InstanceKlass::assert_no_clinit_will_run_for_aot_initialized_class() function to be closed to InstanceKlass::initialize_with_aot_initialized_mirror() -- the latter has been simplified as we would never need to take the "quicker path" from the previous version of the code.

I also cleaned up the "firewall" check in mark_for_aot_initialization().

[iklam]

+bool InstanceKlass::is_enum_subclass(bool direct_only) const {
+  InstanceKlass* s = java_super();
+  return (s == vmClasses::Enum_klass() ||
+          (s != null && s->java_super() == vmClasses::Enum_klass()));
+}

Just happened to notice you aren't checking direct_only.

I consulted with John and removed the direct_only parameter as it's no longer needed.

[iklam]

@iklam Changes to move main module name from modules.cpp to elsewhere do not look related to AOT-linking of indy. If so, can they be done in a separate PR.

The change is actually necessary -- the main module name used to be restored inside MetaspaceShared::serialize(), which happens after we decide to accept the CDS archive. When the main module doesn't match, we will disable the "archived full module graph" and keep on using the rest of the CDS archive.

With aot-linked classes, the main module is needed early to decide whether to reject a CDS archive altogether. Therefore, I needed to lift its processing our of MetaspaceShared::serialize() and put its information directly inside the archive file header.

(This change could have been included in #20843 , but that might be too much work now that the review for JEP 483 is winding down).

[mlbridge]

Mailing list message from John Rose on core-libs-dev:

On 15 Oct 2024, at 12:35, Ioi Lam wrote:

On Tue, 15 Oct 2024 19:08:20 GMT, Dan Heidinga <heidinga at openjdk.org> wrote:

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.

We have been archiving `ConcurrentHashMap` objects for many JDK releases (to support archiving of modules) and `NCPU` would change between CDS archive dump time and program run time. We have not seen any problems, so I think it's safe to use the `@Stable` annotation.

Is it necessary to get constant folding from NCPU? I guess a division is slow, but it does not seem to be a hot path, at all. The division is not in any loop. If there is no performance issue, it?s perhaps more honest just to use a mutable variable. I guess what we need here is something that means ?stable except for AOT activity?.

Normally, making something stable means it should be used according to the contract of stable, which is only one second value, and no third, after the initial default.

There is a sub-rosa contract, as well, which is that we can add third values if they do not conflict with the second values. But we have to convince ourselves that is safe, because the second and third values can (in general) co-exist as folded constants in JIT code.

We currently refuse to fold stable constants in AOT code (and there?s no AOT code in this JEP anyway) so the AOT-only stable binding is tolerable.

Here?s what I think we should do later (not in this PR): Amend the contract for @Stable that the value is allowed to be different during the AOT assembly phase than later. Working out the details will require some discussion: Should the AOT-AP reset stable values that need new bindings? (Probably??) Or should there be a warmup method that takes full responsibility for managing the stable value during the production run?

For now, I?m content to make this guy NCPU either stable or not. But it is clear that, along with other details, we will need an amended story for the life-cycle of a stable variable that includes the AOT assembly phase.

Eventually, something like this may well play into a similar Leyden story for user-visible stables. Those would be declared by an object API, not the private annotation we use in the JDK. User-visible stable objects are likely to be wrappers for JDK-level stable-annotated fields, and that?s how the turtles tend to stack up.

No PR change requested here!

[iklam]

Closing this PR as its changes have been combined into #21642.

[openjdk-notifier]

The parent pull request that this pull request depends on has been closed without being integrated and the target branch of this pull request has been updated as the previous branch was deleted. This means that changes from the parent pull request will start to show up in this pull request. If closing the parent pull request was done in error, it will need to be re-opened and this pull request will need to manually be retargeted again.