On Wed, 9 Jul 2025 13:25:22 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 with a new target base due to a merge > or a rebase. The pull request now contains 24 commits: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > exp/cds-mh-anno > - Merge branch 'master' of https://github.com/openjdk/jdk into > exp/cds-mh-anno > - Reviews > - Documentation > - Don't fail for patching tests > - Remove design document from code > - Some more from reviews > - Reviewed the diff on github > - Stage > - Missed comment updates > - ... and 14 more: https://git.openjdk.org/jdk/compare/a201be85...d2dd466b Good fix. I added some minor comments. No logic changes requested! I'm a little surprised to see the second annotation has survived; I'd prefer to see it totally nuked. But I am not asking you to remove it; there is an argument that it is helpful. I requested better documdentation and checking that the two annotations are properly interdependent. The "runtime setup" annotation should not be allowed except in classes marked as AOT safe. In a few places you removed an unrelated "non-public" comment. That is not a good cleanup to incorporate in passing, since the comment is actually functional. If somebody is arguing that such comments are somehow detrimental, let them make a separate argument and a separate PR to purge them. src/hotspot/share/classfile/classFileParser.cpp line 5179: > 5177: log_info(aot, init)("Found @AOTSafeClassInitializer class %s", > ik->external_name()); > 5178: } > 5179: } This would be a good place to validate that the runtime-setup annotation is only placed on `@AOTSCI` classes. src/java.base/share/classes/java/lang/invoke/BoundMethodHandle.java line 47: > 45: * All bound arguments are encapsulated in dedicated species. > 46: */ > 47: /*non-public*/ please keep this comment intact; it reminds maintainers that the non-public access of this class is not accidental src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java line 67: > 65: * @param <S> species data type. > 66: */ > 67: /*non-public*/ please keep this comment intact; it reminds maintainers that the non-public access of this class is not accidental src/java.base/share/classes/java/lang/invoke/DelegatingMethodHandle.java line 39: > 37: * @author jrose > 38: */ > 39: /*non-public*/ please keep this comment intact; it reminds maintainers that the non-public access of this class is not accidental src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 75: > 73: * @author jrose > 74: */ > 75: /*non-public*/ please keep this comment intact; it reminds maintainers that the non-public access of this class is not accidental src/java.base/share/classes/jdk/internal/vm/annotation/AOTRuntimeSetup.java line 39: > 37: /// > 38: /// `classFileParser.cpp` performs checks on the annotated method - if the > 39: /// annotated method's signature differs from that described above, a add: /// `classFileParser.cpp` performs checks on the annotated method - if the /// annotated method's signature differs from that described above, +/// or if the enclosing class is not annotated `@AOTSafeClassInitializer`, /// a [ClassFormatError] will be thrown. Otherwise, there are just some late-firing asserts that catch the error. But the dependency of one annotation on the other should be documented up front. ------------- Changes requested by jrose (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25922#pullrequestreview-3050063235 PR Review Comment: https://git.openjdk.org/jdk/pull/25922#discussion_r2227419561 PR Review Comment: https://git.openjdk.org/jdk/pull/25922#discussion_r2227480716 PR Review Comment: https://git.openjdk.org/jdk/pull/25922#discussion_r2227481353 PR Review Comment: https://git.openjdk.org/jdk/pull/25922#discussion_r2227482049 PR Review Comment: https://git.openjdk.org/jdk/pull/25922#discussion_r2227484671 PR Review Comment: https://git.openjdk.org/jdk/pull/25922#discussion_r2227401171
