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

[Ashutosh Mehra]

On Wed, 16 Oct 2024 02:42:52 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 incrementally with one additional commit since the last revision:
> 
>   Fixed typo in last commit; fixed header inclusion order

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

> 673:     const char* klass_name = info->klass_name;
> 674:     for (; fields[i].valid(); i++) {
> 675:       ArchivableStaticFieldInfo* f = &fields[i];

It looks like the pattern of nested for-loops is copied from HeapShared::archive_object_subgraphs()
but this pattern doesn't seem to give any advantage here. If so can we replace it with just single for loop to make it easier to understand?

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

> 715: };
> 716: 
> 717: void HeapShared::find_archivable_hidden_classes_in_object(oop root) {

If this function find archivable hidden classes, shouldn't there be a check for`is_hidden()` on the InstanceKlass being marked as `required`. Am I missing something?

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

> 788: 
> 789:   init_seen_objects_table();
> 790:   {

Why was this scope introduced?

src/hotspot/share/classfile/systemDictionaryShared.cpp line 675:

> 673: 
> 674: // Returns true if the class should be excluded. This can be called before
> 675: // SystemDictionaryShared::check_excluded_classes().

There are a couple of references to check_excluded_classes() in the comments but this function no longer exists. Can you please update the comment accordingly.

src/hotspot/share/classfile/systemDictionaryShared.cpp line 759:

> 757:       if (k->is_hidden() && should_hidden_class_be_archived(k)) {
> 758:         SystemDictionaryShared::check_for_exclusion(k, &info);
> 759:         if (info.is_excluded()) {

Is it possible that a hidden class for which `should_hidden_class_be_archived()` returns true is marked for exclusion by `SDS::check_for exclusion`? I guess only possibility is if for some reason its super or interface is excluded. Is that possible?

src/hotspot/share/classfile/systemDictionaryShared.cpp line 775:

> 773:   // Now, all hidden classes that have not yet been scanned must be marked as excluded
> 774:   auto exclude_remaining_hidden = [&] (InstanceKlass* k, DumpTimeClassInfo& info) {
> 775:     if (k->is_hidden() && !info.has_checked_exclusion()) {

Suggestion: Check for `has_checked_exclusion()` is not really required as the call to `SDS::check_for_exclusion()` does that. And the condition in `guarantee()` can be updated as `info.is_excluded() || info.is_required()`.

src/hotspot/share/oops/constantPool.cpp line 340:

> 338:     src_cp->iterate_archivable_resolved_references([&](int rr_index) {
> 339:       keep_resolved_refs.at_put(rr_index, true);
> 340:       });

Indentation seems off here

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1803888327
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1803887158
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1803887567
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1803887919
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1803887979
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1803888011
PR Review Comment: https://git.openjdk.org/jdk/pull/21143#discussion_r1803889248