On Wed, 16 Oct 2024 02:42:52 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:
>
> 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
