On Thu, 17 Oct 2024 04:19:27 GMT, Ioi Lam <ik...@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 incrementally with one additional commit > since the last revision: > > @liach comments -- added comments; added asserts for HashMap safety in > MethodType.java
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](https://github.com/user-attachments/files/17408940/aot-indy-21143-commentary.diff.txt) The first lines of the patch file state which revision the patch applies to, which is 4be6a255cdd from Tuesday. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21143#issuecomment-2418914733
