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