On Wed, 2 Jul 2025 18:47:58 GMT, Chen Liang <li...@openjdk.org> wrote:

>> I have updated this patch to avoid a redundant `runtimeSetup` annotation - 
>> we have agreed that the requirement for setup is a side effect of 
>> initialization, and such methods in AOTCI classes must be automatically 
>> recognized. This latest revision implements that model.
>> 
>> I intentionally avoided handling Class and ClassLoader `resetArchivedStates` 
>> and `MethodType::assemblySetup` - we talked about a generic 
>> `assemblyCleanup` method, but I did not find out where is the best place to 
>> call such a method in the assembly phase. We cna handle this in a subsequent 
>> patch.
>> 
>> In particular, please review the new AOT.md design document - I split it 
>> from the AOTCI annotation to prevent jamming; we can put general AOT 
>> information there when we have more AOT-specific annotations.
>> 
>> ---
>> 
>> Old description:
>> Currently, the list of classes that have <clinit> interdependencies and 
>> those that need runtimeSetup are maintained in a hardcoded list in CDS. This 
>> makes it risky for core library developers as they might introduce new 
>> interdependencies and observe CDS to fail. By moving the mechanism of these 
>> lists to core library annotations as a first step, we can gradually expose 
>> the AOT contracts as program semantics described by internal annotations, 
>> and also helps us to explore how we can expose these functionalities to the 
>> public later.
>
> Chen Liang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Documentation

Changes requested by iklam (Reviewer).

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

> 217:     // MethodHandleStatics is an example of a class that must NOT get 
> the aot-init treatment,
> 218:     // because of its strong reliance on (a) final fields which are (b) 
> environmentally determined.
> 219:     if (ik->has_aot_safe_initializer()) {

I think the above 4 lines of comments can be deleted, as this topic is already 
covered in AOTSafeClassInitializer.java.

src/hotspot/share/classfile/classFileParser.cpp line 5171:

> 5169:       InstanceKlass* intf = _transitive_interfaces->at(i);
> 5170:       if (intf->class_initializer() != nullptr) {
> 5171:         if (!intf->has_aot_safe_initializer()) {

I think this is better, so you don't need to annotate a supertypes that have no 
`<clinit>`


if (intf->class_initializer() != nullptr && !intf->has_aot_safe_initializer()) {

src/hotspot/share/oops/instanceKlassFlags.hpp line 58:

> 56:     flag(has_final_method                   , 1 << 13) /* True if klass 
> has final method */ \
> 57:     flag(has_aot_safe_initializer           , 1 << 14) /* has 
> @AOTSafeClassInitializer annotation */ \
> 58:     flag(is_runtime_setup_required          , 1 << 15) /* has a 
> runtimeSetup method to be called */ \

This information is useful only during CDS dumping. Since `_flags` has limited 
width (only 16 bits) we should avoid using these bits but rather put the 
information inside `DumpTimeClassInfo`. However, there's a complication here, 
as `DumpTimeClassInfo` is not yet created when you call 
`set_has_aot_safe_initializer()`.

I think you can go ahead with the current design. I will make a follow-up PR to 
move the creation of `DumpTimeClassInfo` to an earlier time. Then I'll move 
these two bits into `DumpTimeClassInfo`. That way we can accommodate more 
annotations in the future.

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

PR Review: https://git.openjdk.org/jdk/pull/25922#pullrequestreview-2980604461
PR Review Comment: https://git.openjdk.org/jdk/pull/25922#discussion_r2180955211
PR Review Comment: https://git.openjdk.org/jdk/pull/25922#discussion_r2180988960
PR Review Comment: https://git.openjdk.org/jdk/pull/25922#discussion_r2180982768

Reply via email to