RFR: 8293187: Store initialized Enum classes in AOTCache [v10]

[Andrew Dinn]

On Mon, 30 Sep 2024 03:25:26 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> This is the 4th PR for [JEP 483: Ahead-of-Time Class Loading & Linking](https://bugs.openjdk.org/browse/JDK-8315737).
>> 
>> **Problem:**
>> 
>> This PR is necessary to support [JDK-8293336: AOT-linking of invokedynamic for lambda expression and string concat](https://bugs.openjdk.org/browse/JDK-8293336), which needs to store [`sun.invoke.util.Wrapper`](https://github.com/openjdk/jdk/blob/c3711dc90980fb3e63ff199612c201c4464626bf/src/java.base/share/classes/sun/invoke/util/Wrapper.java) enums in the AOT cache. Although CDS has some limited support for storing enums, the `Wrapper` type is too complex for the existing solution to handle. Please see the JBS issue for details.
>> 
>> **Solution:**
>> 
>> In the assembly phase, we store the initialized states of the `Wrapper` class (captured in a `java.lang.Class` object, a.k.a. the *mirror* of this class) into the AOT cache. 
>> 
>> In production run, we no longer execute `Wrapper::<clinit>`, because all the static fields (contained in its mirror) are already initialized.
>> 
>> **Review Notes:**
>> 
>> - The new capability is controlled by `CDSConfig::is_initing_classes_at_dump_time()`. We can aot-initialize classes only if `-XX:+AOTClassLinking`  is enabled.
>>     - The old (more limited) support for enums is still there (it's required when `AOTClassLinking` is disabled). See the call to `CDSEnumKlass::handle_enum_obj()` in heapShared.cpp. 
>> - `AOTClassInitializer::can_archive_initialized_mirror()` decides what classes can be aot-initialized. This is currently a very small set of classes, but will expand in [JDK-8293336](https://bugs.openjdk.org/browse/JDK-8293336)
>> - Before, `HeapShared::archive_java_mirrors()` would clear out all the states in the archived mirrors. With this PR, the states of aot-initialized classes are preserved via `HeapShared::copy_aot_initialized_mirror()`.
>> - During the early state of the production run, `AOTLinkedClassBulkLoader::init_required_classes_for_loader()` is called to make sure that:
>>     - all aot-initialized classes are moved into the `initialized` state (without executing its `<clinit>` method). This is done in `InstanceKlass::initialize_from_cds()`
>>     - the classes of all the objects that are reachable from the aot-initialized mirrors are initialized. See comments above ` HeapShared::init_classes_reachable_from_archived_mirrors()` 
>> 
>> **Caveats:**
>> 
>> Not all Enum classes can be stored in the initialized state. E.g., some Enums might have static fields that depend on the e...
>
> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 109 commits:
> 
>  - Merge branch 'jep-483-step-03-8329706-implement-xx-aot-class-linking' of /jdk3/yak/open into jep-483-step-04-8293187-support-sun-invoke-util-wrapper-in-cds-archive-heap
>  - Merge branch 'jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver' into jep-483-step-03-8329706-implement-xx-aot-class-linking
>  - Merge branch 'jep-483-step-01-8338017-add-aot-command-line-aliases' into jep-483-step-02-8338018-rename-class-prelinker-to-aot-cp-resolver
>  - Merge branch 'master' of https://github.com/openjdk/jdk into jep-483-step-01-8338017-add-aot-command-line-aliases
>  - 8340864: Remove unused lines related to vmClasses
>    
>    Reviewed-by: shade, kvn
>  - 8340831: Simplify simple validation for class definition in MethodHandles.Lookup
>    
>    Reviewed-by: redestad
>  - 8340838: Clean up MutableCallSite to use explicit release fence instead of AtomicInteger
>    
>    Reviewed-by: jrose, redestad, shade
>  - 8340956: ProblemList 4 java/nio/channels/DatagramChannel tests on macosx-all
>    
>    Reviewed-by: liach, alanb, darcy, dfuchs
>  - 8340228: Open source couple more miscellaneous AWT tests
>    
>    Reviewed-by: prr
>  - 8340684: Reading from an input stream backed by a closed ZipFile has no test coverage
>    
>    Reviewed-by: lancea
>  - ... and 99 more: https://git.openjdk.org/jdk/compare/6029b35f...563bccb3

I could not spot any issues. Just a few recommendations for clarifying comments and naming.

src/hotspot/share/cds/aotLinkedClassBulkLoader.cpp line 253:

> 251:       }
> 252:       if (ik->has_aot_initialized_mirror()) {
> 253:         ik->initialize_from_cds(CHECK);

Can we put a comment in here to explain that this call may link class ik and may link+initialize its supers and/or implemented interfaces but it will not initialize ik itself because we are relying on the mirror to provide static field data computed via an init run at assembly time when the archive was created. It really helps to underline that this is the point where we rely on init in a previous VM, bypassing (repeated) init in this VM.

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

> 937: 
> 938:   _run_time_subgraph_info_table.serialize_header(soc);
> 939:   soc->do_ptr(&_runtime_default_subgraph_info);

It would help to have a comment here explaining that 1) before the do_ptr call the specific subgraph_info passed into this call holds all the classes that need to be initialized on behalf of java.lang.Object 2) after the call it is includes all the extra classes that need initializing on behalf of some archived java.lang.Class mirror. This would help to clarify why it was picked as the holder for these extra classes (i.e. it is the obvious root class from which to start running initializations).

If field `_runtime_default_subgraph_info` was renamed to identify it as being the root subgraph associated with class java.lang.Object this would also be clearer (see related comment)

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

> 1007: //
> 1008: // The set of classes that are required to be initialized for the archived
> 1009: // java mirrors are recorded in _runtime_default_subgraph_info (which probably

_runtime_default_subgraph_info is derived from _default_subgraph_info, the latter being the subgraph associated with class java.lang.Object. Perhaps using the prefix `root` or `jlobject` would make this clearer i.e. `_root_subgraph_info` and `_runtime_root_subgraph_info` or `_jlobject_subgraph_info` and `_runtime_jlobject_subgraph_info`.

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

Marked as reviewed by adinn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20958#pullrequestreview-2337368712
PR Review Comment: https://git.openjdk.org/jdk/pull/20958#discussion_r1781024490
PR Review Comment: https://git.openjdk.org/jdk/pull/20958#discussion_r1781115343
PR Review Comment: https://git.openjdk.org/jdk/pull/20958#discussion_r1781090464