On Fri, 18 Oct 2024 20:09:04 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:
>
> Added
> ClaimMetadataVisitingOopIterateClosure::assert_is_pending_aot_linked_class()
src/hotspot/share/memory/iterator.hpp line 177:
> 175: class ClaimMetadataVisitingOopIterateClosure : public OopIterateClosure {
> 176: private:
> 177: static void assert_is_pending_aot_linked_class(Klass* k)
> NOT_DEBUG_RETURN;
Suggestion:
private:
static void assert_is_pending_aot_linked_class(Klass* k) NOT_DEBUG_RETURN;
src/hotspot/share/memory/iterator.inline.hpp line 57:
> 55: ClaimMetadataVisitingOopIterateClosure::do_cld(cld);
> 56: } else {
> 57: assert_is_pending_aot_linked_class(k);
Would there be a drawback to just adding
`assert(AOTLinkedClassBulkLoader::is_pending_aot_linked_class(k), "sanity");`
here and skip the changes the both .hpp and .cpp files?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1808211577
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1808213774
