RFR: 8344140: Refactor the discovery of AOT cache artifacts [v5]

[Calvin Cheung]

On Sat, 11 Jan 2025 19:23:53 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> This is a clean up after the initial integration for [JEP 483](https://bugs.openjdk.org/browse/JDK-8315737)
>> 
>> - Merged 3 loops into 1 for discovering the classes and oops that should be stored in the AOT cache. About 250 lines of code are deleted.
>> - Added comments in aotArtifactFinder.hpp to describe the logic, which is much simpler than before.
>> - Enum classes are no longer unconditionally AOT-initialized -- an Enum class is AOT-initialized only if we find an archived oop of this type.
>> 
>> Note to reviewers:
>> 
>> One consequence of this refactoring is that archived oops are now discovered (by heapShared.cpp) before the metadata are copied (by ArchiveBuilder). There are some functions that depend on the old order (metadata were copied before oop discovery), so I had to shuffle them around. Examples are `Modules::check_archived_module_oop()`, or the new code in `Klass::remove_java_mirror()`.
>
> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
> 
>  - Updated copyright
>  - Merge branch 'master' into 8344140-consolidate-aot-artifact-gathering-discovery
>  - @ashu-mehra comments
>  - Merge branch 'master' into 8344140-consolidate-aot-artifact-gathering-discovery
>  - Merge branch 'master' of https://github.com/openjdk/jdk into 8344140-consolidate-aot-artifact-gathering-discovery
>  - Fixe 32-bit builds; removed unused function
>  - more clean up
>  - tmp
>  - More clean up
>  - Removed unnecessary code
>  - ... and 1 more: https://git.openjdk.org/jdk/compare/31452788...e246276c

Some minor comments below.

src/hotspot/share/cds/aotArtifactFinder.hpp line 71:

> 69: // Note1: See TODO comments in HeapShared::archive_object() for exceptions to this rule.
> 70: //
> 71: // Note2: The scanning of Java objects is done in heapShared.cpp. Please see calls into the HeapShared class

Note2 isn't referenced by the comment above. Perhaps line 62 should reference it?

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

> 318:     if (k->is_instance_klass()) {
> 319:       // Whenever we see a non-array Java object of type X, we mark X to be aot-initialized.
> 320:       // This ensures that during the production run, whenever Java code seens a cached object

typo: s/seens/sees

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

> 325:         // we must store them as AOT-initialized.
> 326:         AOTArtifactFinder::add_aot_inited_class(InstanceKlass::cast(k));
> 327:       } else if (subgraph_info == _dump_time_special_subgraph) {

Can this be an "or" condition rather than "else if" since the same function is called for either on of the conditions?

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

> 782: 
> 783:   if (orig_k->is_instance_klass()) {
> 784:     InstanceKlass* ik = InstanceKlass::cast(orig_k);

Should this be inside a "#if ASSERT" since `ik` is used only in the `assert` statements?

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

PR Review: https://git.openjdk.org/jdk/pull/22291#pullrequestreview-2547949837
PR Review Comment: https://git.openjdk.org/jdk/pull/22291#discussion_r1913827272
PR Review Comment: https://git.openjdk.org/jdk/pull/22291#discussion_r1913828722
PR Review Comment: https://git.openjdk.org/jdk/pull/22291#discussion_r1913830983
PR Review Comment: https://git.openjdk.org/jdk/pull/22291#discussion_r1913833806