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

[Andrew Dinn]

On Wed, 2 Oct 2024 01:03:04 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 incrementally with two additional commits since the last revision:
> 
>  - Adjust TEST.groups after merge with mainline
>  - For aot-inited classes, require the <clinit> of all supertypes to be executed in assembly phase as well

Marked as reviewed by adinn (Reviewer).

@iklam The update looks good but I suggested a few changes and comments for clarity.

src/hotspot/share/cds/aotClassInitializer.cpp line 39:

> 37:     : _class_name(class_name), _is_prefix(is_prefix)
> 38:   {
> 39:     _len = (class_name == nullptr) ? 0 : (int)strlen(class_name);

Do you really want to allow class_name to be passed as nullptr? Would it not be better to terminate the array of AllowedSpec with a nullptr entry?

If so then this constructor needs an assert `class_name != nullptr`

If not then method `matches` probably ought to assert `_class_name != nullptr`

src/hotspot/share/cds/aotClassInitializer.cpp line 52:

> 50: };
> 51: 
> 52: 

I was trying to work out how to improve the commenting of this method because it currently has to refer to 'the tables in can_archive_initialized_mirror' i.e. the comment needs to describe where and why this method is called in order to explain what it does, which has a bad smell. However, it seems to me the problem is really in the way the code is structured.

It being called `is_allowed` also highlights the same problem. That name is confusing because
1. it does not make it very clear that it can be called more than once from `can_archive_initialized_mirror` with alternative specs (hence the comment)
2. passing this test is only one of several checks on whether a class can be archived e.g. an direct subtype of enum gets a free pass
3. it does not clarify that this check currently also performs verification

I think it would be clearer to split this into two methods:

    bool AOTClassInitializer::is_allowed_by_spec(AllowedSpec* specs, ik);
    void verify_spec_class(ik);

The first method would just do the iterative match test and return true or false. The second one would only be called when the first call returns true and would include the recursive testing of super and interfaces, asserting on failure. That latter call could then also be placed under ifdef ASSERT.

This would allow the comment in the verify method to be rewritten to:

// Any class whose mirror is included by an AllowSpecification requires that
//  - all super class mirrors must be included
//  - all super interface mirrors that have <clinit> must be included.

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

PR Review: https://git.openjdk.org/jdk/pull/20958#pullrequestreview-2342474269
PR Comment: https://git.openjdk.org/jdk/pull/20958#issuecomment-2388527486
PR Review Comment: https://git.openjdk.org/jdk/pull/20958#discussion_r1784220255
PR Review Comment: https://git.openjdk.org/jdk/pull/20958#discussion_r1784414294