8303431: [JVMCI] libgraal annotation API by dougxc · Pull Request #12810 · openjdk/jdk

dougxc · 2023-03-01T18:07:34Z

This PR extends JVMCI with new API (jdk.vm.ci.meta.Annotated) for accessing annotations. The main differences from java.lang.reflect.AnnotatedElement are:

All methods in the Annotated interface explicitly specify requested annotation type(s). That is, there is no equivalent of AnnotatedElement.getAnnotations().
Annotation data is returned in a map-like object (of type jdk.vm.ci.meta.AnnotationData) instead of in an Annotation object. This works better for libgraal as it avoids the need for annotation types to be loaded and included in libgraal.

To demonstrate the new API, here's an example in terms java.lang.reflect.AnnotatedElement (which ResolvedJavaType implements):

    ResolvedJavaMethod method = ...;
    ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
    return switch (a.kind()) {
        case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
        case FULL_UNROLL_UNTIL_RETURN -> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
        ...
    }

The same code using the new API:

    ResolvedJavaMethod method = ...;
    ResolvedJavaType explodeLoopType = ...;
    AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
    return switch (a.getEnum("kind").getName()) {
        case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
        case "FULL_UNROLL_UNTIL_RETURN" -> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
        ...
    }

The implementation relies on new methods in jdk.internal.vm.VMSupport for parsing annotations and serializing/deserializing to/from a byte array. This allows the annotation data to be passed from the HotSpot heap to the libgraal heap.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8303431: [JVMCI] libgraal annotation API

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to a85fa13a
Tom Rodriguez (@tkrodriguez - Reviewer) ⚠️ Review applies to a85fa13a
Joe Darcy (@jddarcy - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12810/head:pull/12810
$ git checkout pull/12810

Update a local copy of the PR:
$ git checkout pull/12810
$ git pull https://git.openjdk.org/jdk.git pull/12810/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12810

View PR using the GUI difftool:
$ git pr show -t 12810

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12810.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-03-01T18:09:16Z

👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-01T18:11:16Z

@dougxc The following labels will be automatically applied to this pull request:

core-libs
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-03-01T18:38:16Z

Webrevs

dougxc · 2023-03-07T20:50:08Z

This PR still needs work. I'll re-open it when ready.

openjdk · 2023-03-08T19:54:08Z

@dougxc this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8303431
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

vnkozlov · 2023-03-13T19:23:39Z

test/jdk/jdk/internal/vm/TestTranslatedException.java

    }
-    @SuppressWarnings("unchecked")
-    @Test
-    public void encodeDecodeTest2() throws Exception {
-        Throwable throwable = new ExceptionInInitializerError(new InvocationTargetException(new Untranslatable("test exception", new NullPointerException()), "invoke"));
-        for (int i = 0; i < 10; i++) {
-            throwable = new ExceptionInInitializerError(new InvocationTargetException(new RuntimeException(String.valueOf(i), throwable), "invoke"));
-        }
-        encodeDecode(throwable);
-    }



Why this was removed?

Because it does exactly the same thing as encodeDecodeTest. It should have been cleaned up in the original PR that introduced this test.

vnkozlov

I looked on Hotspot, JVMCI and VMSupport.java changes. But you need to ask Tom to look on JVMCI changes in details. And someone from core-libs who familiar with Annotations have to comment on your implementation in general because I am not expert.

openjdk · 2023-03-13T19:30:15Z

@dougxc This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8303431: [JVMCI] libgraal annotation API

Reviewed-by: kvn, never, darcy

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 26 new commits pushed to the master branch:

eb8d8cd: 8299129: Enhance NameService lookups
77df315: 8296692: Improve String platform support
f098b49: 8298310: Enhance TLS session negotiation
9e56d10: 8296832: Improve Swing platform support
2e5700a: 8288436: Improve Xalan supports
b1c34c0: 8296684: Improve String platform support
14aad78: 8294474: Better AES support
a0f7ae1: 8298667: Improved path handling
ec11971: 8296676: Improve String platform support
5ec0120: 8297371: Improve UTF8 representation redux
... and 16 more: https://git.openjdk.org/jdk/compare/525a91e3fac892c26b09cc1705d0909afe80c8f9...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

tkrodriguez

The JVMCI changes look ok to me.

tkrodriguez · 2023-03-14T06:28:20Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

+
+  typeArrayOop ba = typeArrayOop(res);
+  int ba_len = ba->length();
+  if (ba_len <= 256) {


Is this really necessary? Resource allocation is very cheap.

Ok, good point. I'll remove the optimization.

tkrodriguez · 2023-03-14T06:39:18Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/Annotated.java

+     * initialized. Class initialization is not triggered for enum types referenced by other
+     * annotations of this element.
+     *
+     * If this element is a class, then {@link Inherited} annotations are included in set of


dougxc · 2023-03-15T18:00:49Z

@jddarcy would you be able to help review this PR? Based on git log, you are knowledgeable in sun.reflect.annotation. If not, please suggest who else I can reach out to for a review.

src/java.base/share/classes/jdk/internal/vm/VMSupport.java

jddarcy · 2023-04-17T15:32:56Z

src/java.base/share/classes/jdk/internal/vm/VMSupport.java

+    /**
+     * Encodes a list of annotations to a byte array. The byte array can be decoded with {@link #decodeAnnotations(byte[], AnnotationDecoder)}.
+     */
+    public static byte[] encodeAnnotations(Collection<Annotation> annotations) {


I don't think it matters much in this use case, but it looks like encodeAnnotations could be changed to take a List rather than a Collection, as the comment implies.

Just above (line 228) you can see a call to this method where the argument comes from Map.values() whose type is Collection so I'd prefer to leave it as is rather than have to convert the argument to a List.

In that case, I think the comment on line 232 should be updated to not say "list".

I changed the comment to "Encodes annotations to a byte array."

jddarcy · 2023-04-17T15:48:53Z

src/java.base/share/classes/jdk/internal/vm/VMSupport.java

+     * @param <E> type of the object representing a decoded enum constant
+     * @param <X> type of the object representing a decoded error
+     */
+    public interface AnnotationDecoder<T, A, E, X> {


I think it would be better to include some bound on the type parameters to better capture their intention
A extends java.lang.Annotatoin
E extends java.lang.Enum
etc.

These types are alternatives to java.lang.Annotation, java.lang.Enum etc. That's the primary motivation for this PR, i.e. to be able to represent annotations without having to reify them as java.lang.Annotation objects.

jddarcy · 2023-04-17T16:50:47Z

A few higher-level comments:

From the long-term perspective, it is likely that the set of kinds of elements that can occur in an annotation will be expanded, for example, method references are a repeated request. Easing future maintenance to gives more inter-source linkage in this situation and error handling for this case in the libgraal code would be prudent IMO.

The java.lang.reflect.AnnotatedElement API (https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/lang/reflect/AnnotatedElement.html) defines different ways an annotation can be affiliated with an element:

"The terms directly present, indirectly present, present, and associated are used throughout this interface to describe precisely which annotations are returned by methods: "

tl;dr these terms relate to which of inheriting annotations and looking through repeated annotations the methods do on behalf of the caller. I think the methods should phrase their operations in terms of these concepts, such as "Construct the annotations present..." since inheritance is taken into account.

dougxc · 2023-04-17T20:46:36Z

From the long-term perspective, it is likely that the set of kinds of elements that can occur in an annotation will be expanded, for example, method references are a repeated request. Easing future maintenance to gives more inter-source linkage in this situation and error handling for this case in the libgraal code would be prudent IMO.

I'm not sure what you're suggesting in terms of how I should update this PR. Do you mean AnnotationData.get needs to somehow be more flexible? If so, could you please give a concrete example of what you're after.

…ons are returned

dougxc · 2023-04-17T20:57:32Z

the methods should phrase their operations in terms of these concepts...

I think this is what you're suggesting: 362738a

jddarcy · 2023-04-18T02:22:11Z

From the long-term perspective, it is likely that the set of kinds of elements that can occur in an annotation will be expanded, for example, method references are a repeated request. Easing future maintenance to gives more inter-source linkage in this situation and error handling for this case in the libgraal code would be prudent IMO.

I'm not sure what you're suggesting in terms of how I should update this PR. Do you mean AnnotationData.get needs to somehow be more flexible? If so, could you please give a concrete example of what you're after.

Let me explain my concerns in more detail.

We have (at least) two separate annotations implementations in the JDK, one in javac for compile-time and other in core reflection for runtime. Due to various technical constraints, the implementations are necessarily separate (although the annotation objects constructed by javac do also implement the java.lang.annotation.Annotation interface also used by core reflection).

This work is proposing to add another partial implementation to satisfy other technical goals, reusing portions of the existing core reflection machinery.

If at some point the universe of objects that can be encoded as annotation is expanded, e..g method literals as mentioned previously, it is well-understood that core reflection and javac will need to be updated. I think it would be relatively easy to overlook the need to make corresponding updates to libgraal API and all regression tests might still pass.

As a concrete suggestion, if such an unknown annotation was handed over to libgraal, I think the code should reject it (AssertionError("unexpected annotation component") etc.) in hope of the omission getting corrected sooner. Also, leaving some bread crumb comments to future maintainers of core reflection annotations in that implementation package would be helpful too, e.g. "// used by libgraal".

HTH

…ew annotation element types be added

dougxc · 2023-04-18T07:20:56Z

I think the code should reject it

The AnnotationData constructor already has a check for unknown annotation element types so I think this concern is covered.

leaving some bread crumb comments to future maintainers of core reflection annotations

Done here: bad23a0

dougxc · 2023-04-19T16:00:18Z

Thanks for the reviews @turbanoff , @vnkozlov and @jddarcy.

/integrate

openjdk · 2023-04-19T16:02:12Z

Going to push as commit 48fd4f2.
Since your change was applied there have been 35 commits pushed to the master branch:

c57af31: 8306038: SystemModulesPlugin generates code that doesn't pop when return value not used
a31a11f: 8306006: strace001.java fails due to unknown methods on stack
ddb8646: 8306123: Move InstanceKlass writeable flags
1a41e12: 8306310: Move is_shared Klass flag
c738c8e: 8306278: jvmtiAgentList.cpp:253 assert(offset >= 0) failed: invariant occurs on AIX after JDK-8257967
9fb53ad: 8305716: Enhancements for printing age tables
ebba42a: 8305993: Add handleSocketErrorWithMessage to extend nio Net.c exception message
42b7260: 8306111: PPC64: RT call after thaw with exception requires larger ABI section
c7faf60: 8305757: Call Method::compute_has_loops_flag() when creating CDS archive
eb8d8cd: 8299129: Enhance NameService lookups
... and 25 more: https://git.openjdk.org/jdk/compare/525a91e3fac892c26b09cc1705d0909afe80c8f9...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-19T16:02:26Z

@dougxc Pushed as commit 48fd4f2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

vnkozlov · 2023-04-20T01:03:30Z

@dougxc I see next 2 JVMCI tests failed when run with -XX:TypeProfileLevel=222 on all platforms (our stress testing):

compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaMethod.java
compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java

#  Internal Error (/workspace/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:829), pid=2430194, tid=2430218
#  fatal error: DEBUG MESSAGE: exact klass and actual klass differ

Current thread (0x00007f9ff8460480):  JavaThread "MainThread" [_thread_in_Java, id=2430218, stack(0x00007f9fe06b7000,0x00007f9fe07b8000)]

Stack: [0x00007f9fe06b7000,0x00007f9fe07b8000],  sp=0x00007f9fe07b44e0,  free space=1013k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x12b8d35]  MacroAssembler::debug64(char*, long, long*)+0x45  (macroAssembler_x86.cpp:829)
J 1935 c1 sun.reflect.annotation.AnnotationParser.parseAnnotation2(Ljava/nio/ByteBuffer;Ljdk/internal/reflect/ConstantPool;Ljava/lang/Class;Z[Ljava/lang/Class;)Ljava/lang/annotation/Annotation; java.base@21-internal (275 bytes) @ 0x00007f9fe10d4cc6 [0x00007f9fe10d4740+0x0000000000000586]

[error occurred during error reporting (printing native stack (with source info))

dougxc · 2023-04-20T09:08:41Z

I can reproduce this locally as well but really don't know where to start in terms of debugging this. Can you please provide hints as to what may cause this failure C1 compiled code?

vnkozlov · 2023-04-20T16:54:06Z

Filed JDK-8306581

openjdk bot added hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Mar 1, 2023

add annotation API to JVMCI

f867572

dougxc force-pushed the JDK-8303431 branch from 5e32fb7 to f867572 Compare March 1, 2023 18:15

made AnnotationDataDecoder package-private

fbc109d

dougxc marked this pull request as ready for review March 1, 2023 18:33

openjdk bot added the rfr Pull request is ready for review label Mar 1, 2023

dougxc added 2 commits March 5, 2023 22:41

Merge branch 'master' into JDK-8303431

d2b6bab

added support for inherited annotations

8743e8b

openjdk bot removed the rfr Pull request is ready for review label Mar 5, 2023

fixed whitespace

3dd5ef9

openjdk bot added the rfr Pull request is ready for review label Mar 5, 2023

dougxc closed this Mar 7, 2023

switched to use of lists and maps instead of arrays

948d3aa

dougxc reopened this Mar 8, 2023

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Mar 8, 2023

Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8303431

a85fa13

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Mar 8, 2023

vnkozlov reviewed Mar 13, 2023

View reviewed changes

vnkozlov approved these changes Mar 13, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 13, 2023

tkrodriguez approved these changes Mar 14, 2023

View reviewed changes

addressed review feedback

abaf237

[skip ci] formatting fixes

3213179

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Apr 3, 2023

jddarcy reviewed Apr 17, 2023

View reviewed changes

src/java.base/share/classes/jdk/internal/vm/VMSupport.java Outdated Show resolved Hide resolved

jddarcy reviewed Apr 17, 2023

View reviewed changes

Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8303431

4926216

dougxc added 2 commits April 17, 2023 22:53

fixed comment

6ab5bf2

rephrased javadoc Annotated to more precisely describe which annotati…

362738a

…ons are returned

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Apr 17, 2023

dougxc added 2 commits April 18, 2023 09:19

fixed javadoc comment

6d324f6

added breadcrumb in AnnotationParser about considering JVMCI should n…

bad23a0

…ew annotation element types be added

jddarcy approved these changes Apr 19, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 19, 2023

openjdk bot closed this Apr 19, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 19, 2023

dougxc deleted the JDK-8303431 branch August 20, 2024 06:20

Conversation

dougxc commented Mar 1, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 1, 2023

Uh oh!

openjdk bot commented Mar 1, 2023

Uh oh!

mlbridge bot commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dougxc commented Mar 7, 2023

Uh oh!

openjdk bot commented Mar 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tkrodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc commented Mar 15, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jddarcy commented Apr 17, 2023

Uh oh!

dougxc commented Apr 17, 2023

Uh oh!

dougxc commented Apr 17, 2023

Uh oh!

jddarcy commented Apr 18, 2023

Uh oh!

dougxc commented Apr 18, 2023

Uh oh!

dougxc commented Apr 19, 2023

Uh oh!

openjdk bot commented Apr 19, 2023

Uh oh!

openjdk bot commented Apr 19, 2023

Uh oh!

vnkozlov commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougxc commented Apr 20, 2023

Uh oh!

vnkozlov commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

dougxc commented Mar 1, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 1, 2023 •

edited

Loading

openjdk bot commented Mar 13, 2023 •

edited

Loading

vnkozlov commented Apr 20, 2023 •

edited

Loading