8199149: Improve the exception message thrown by VarHandle of unsupported operation #14928

mlchung · 2023-07-19T00:12:53Z

VarForm::getMemberName currently throws UOE with no information if the requested access mode is unsupported. To provide the var handle information, move the access mode check to VarHandle so that the exception message can include the var handle information. Changes include:

change VarHandle::checkAccessModeThenIsDirect to check if the access mode is unsupported. This check is only needed for direct var handle.
change VarHandle::getMethodHandleUncached to call getMemberNameOrNull and throw UOE with an informative exception message if the access mode is unsupported

The error message looks like:

java.lang.UnsupportedOperationException: compareAndSet is not supported for VarHandle[varType=java.lang.String, coord=[class Foo$Goo]]

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-8199149: Improve the exception message thrown by VarHandle of unsupported operation (Enhancement - P3)

Reviewers

Chen Liang (@liach - Author) ⚠️ Review applies to 9196ccb4
Jorn Vernee (@JornVernee - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14928

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

Using diff file

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

Webrev

Link to Webrev Comment

…rted operation

bridgekeeper · 2023-07-19T00:13:42Z

👋 Welcome back mchung! 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-07-19T00:14:51Z

@mlchung The following label will be automatically applied to this pull request:

core-libs

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

mlbridge · 2023-07-19T00:20:54Z

Webrevs

liach · 2023-07-19T00:49:24Z

src/java.base/share/classes/java/lang/invoke/VarHandle.java

+        }
+
+        static AccessMode valueFromOrdinal(int mode) {
+            return modeToAccessMode.get(mode);


Can't this be simplified to AccessMode.values()[mode] since this is only rarely used in the exception logic?

If we do need a cache, I would recommend a stable array like

private static final @Stable AccessMode[] VALUES = values();

and users call this accessor instead.

The code in getMethodHandleUncached can benefit from this caching mechanism.

Good suggestion.

liach · 2023-07-19T00:52:49Z

src/java.base/share/classes/java/lang/invoke/IndirectVarHandle.java

@@ -90,7 +90,9 @@ public VarHandle withInvokeBehavior() {
    @Override
    @ForceInline
    boolean checkAccessModeThenIsDirect(VarHandle.AccessDescriptor ad) {
-        super.checkAccessModeThenIsDirect(ad);
+        if (exact && accessModeType(ad.type) != ad.symbolicMethodTypeExact) {


Can we add a comment that the detailed UOE is thrown via directTarget.getMethodHandleUncached?

Actually VarHandle::checkAccessModeThenIsDirect can call isAccessModeSupported so that this will check properly for IndirectVarHandle.

JornVernee

change VarHandle::checkAccessModeThenIsDirect to check if the access mode is unsupported. This check is only needed for direct var handle.

Note that IndirectVarHandle calls super.checkAccessModeThenIsDirect, so it ends up doing the check any way, I think?

JornVernee · 2023-07-19T23:30:09Z

src/java.base/share/classes/java/lang/invoke/VarHandle.java

+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;


These two new imports seem unused.

liach · 2023-07-19T23:41:02Z

Note that IndirectVarHandle calls super.checkAccessModeThenIsDirect, so it ends up doing the check any way, I think?

In the initial patch, it was distinct; it was migrated to call isAccessModeSupported in a subsequent change, so the change in IndirectVarHandle was rolled back and the VHs share a more consistent behavior.

liach · 2023-07-19T23:42:29Z

Just curious, why does Paul Sandoz recommend removing the VarHandle itself from the UOE message?

mlchung · 2023-07-19T23:45:46Z

I got feedback from @PaulSandoz. The patch is updated to include only the access mode information.

checkAccessModeThenIsDirect is on a performance sensitive path. We only need to modify VarForm::getMemberName to print out the access mode. Also mark getMemberName as @Hidden which then shows the call site from the top of the stack trace. The user can find out which VarHandle from the callsite from the stack trace location.

liach

The enum values caching can be applied to AccessType as well; can be done in a separate patch, and I've created ticket at https://bugs.openjdk.org/browse/JDK-8312423.

liach · 2023-07-20T00:06:35Z

Also just noticed copyright year of VarForm is still 2019; should be updated to 2023 with this patch.

JornVernee · 2023-07-20T15:03:02Z

src/java.base/share/classes/java/lang/invoke/VarHandleGuards.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2023, Oracle and/or its affiliates. All rights reserved.


I think you've updated the wrong copyright header?

openjdk · 2023-07-20T15:12:06Z

@mlchung 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:

8199149: Improve the exception message thrown by VarHandle of unsupported operation

Reviewed-by: liach, jvernee

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 43 new commits pushed to the master branch:

354c660: 8307185: pkcs11 native libraries make JNI calls into java code while holding GC lock
bae2247: 8308591: JLine as the default Console provider
b772e67: 8312395: Improve assertions in growableArray
9fa944e: 8312019: Simplify and modernize java.util.BitSet.equals
fe41910: 8312459: Problem list java/awt/GraphicsDevice/DisplayModes/CycleDMImage.java for macOS
8d29329: 8312320: Remove javax/rmi/ssl/SSLSocketParametersTest.sh from ProblemList
94eb44b: 8312394: [linux] SIGSEGV if kernel was built without hugepage support
3283328: 8311130: AArch64: Sync SVE related CPU features with VM options
a742767: 8312246: NPE when HSDB visits bad oop
37c756a: 8305506: Add support for fractional values of SafepointTimeoutDelay
... and 33 more: https://git.openjdk.org/jdk/compare/af5bf81754072fa5879726cfacb7404892b553f0...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.

JornVernee · 2023-07-20T15:16:33Z

src/java.base/share/classes/java/lang/invoke/VarHandle.java

+        private static final @Stable AccessMode[] VALUES = values();
+        static AccessMode valueFromOrdinal(int mode) {
+            return VALUES[mode];
+        }


Also, I'll throw this out there, though I'm not sure how big of an issue it is: this array creation might have an effect on startup. But, since it is only used on a slow path right before we throw an exception (at least, at the moment), calling values() every time inside the valueFromOrdinal method, and avoiding the array creation on startup, might be 'better'.

I think this is a worthy change: see https://github.com/openjdk/jdk/pull/14943/files#diff-556b309ea2df3f5bfe8229de944183ef19750ce4511d0328cd59af2ce2b61ae2R135
that VarHandle.AccessMode.values() is called every time a polymorphic VH call is linked.

If the startup cost from array allocation is really problematic, we can make it stable and allocate VALUES on demand (at valueFromOrdinal calls).

Ah, so it looks like we are creating this array already, in adding the valueFromOrdinal method allows sharing. That sounds good!

The overhead is just one time allocation of an array of length 31, which is a very small array which I don't think such small array creation impacts as much.

I should have replaced other uses of AccessMode.values(). I will include that change in this patch.

@liach I don't know about AccessType.values() which seems to me not worth the change to add valueFromOrdinal as only calls that 6 times.

mlchung · 2023-07-20T20:20:31Z

/integrate

openjdk · 2023-07-20T20:21:34Z

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

354c660: 8307185: pkcs11 native libraries make JNI calls into java code while holding GC lock
bae2247: 8308591: JLine as the default Console provider
b772e67: 8312395: Improve assertions in growableArray
9fa944e: 8312019: Simplify and modernize java.util.BitSet.equals
fe41910: 8312459: Problem list java/awt/GraphicsDevice/DisplayModes/CycleDMImage.java for macOS
8d29329: 8312320: Remove javax/rmi/ssl/SSLSocketParametersTest.sh from ProblemList
94eb44b: 8312394: [linux] SIGSEGV if kernel was built without hugepage support
3283328: 8311130: AArch64: Sync SVE related CPU features with VM options
a742767: 8312246: NPE when HSDB visits bad oop
37c756a: 8305506: Add support for fractional values of SafepointTimeoutDelay
... and 33 more: https://git.openjdk.org/jdk/compare/af5bf81754072fa5879726cfacb7404892b553f0...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-07-20T20:21:41Z

@mlchung Pushed as commit d7b9416.

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

8199149: Improve the exception message thrown by VarHandle of unsuppo…

cc66b08

…rted operation

openjdk bot added the rfr Pull request is ready for review label Jul 19, 2023

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jul 19, 2023

liach reviewed Jul 19, 2023

View reviewed changes

review feedback

176df23

liach approved these changes Jul 19, 2023

View reviewed changes

JornVernee reviewed Jul 19, 2023

View reviewed changes

Review feedback from psandoz

9196ccb

liach approved these changes Jul 19, 2023

View reviewed changes

update copyright header

f82b214

JornVernee approved these changes Jul 20, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 20, 2023

JornVernee reviewed Jul 20, 2023

View reviewed changes

replace other use of AccessMode.values() with valueFromOrdinal

cd46a75

JornVernee approved these changes Jul 20, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 20, 2023

openjdk bot closed this Jul 20, 2023

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

liach mentioned this pull request Jul 20, 2023

8312423: Cache the enum values for VarHandle.AccessType #14943

Closed

3 tasks

mlchung deleted the JDK-8199149 branch August 2, 2023 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8199149: Improve the exception message thrown by VarHandle of unsupported operation #14928

8199149: Improve the exception message thrown by VarHandle of unsupported operation #14928

mlchung commented Jul 19, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 19, 2023

openjdk bot commented Jul 19, 2023

mlbridge bot commented Jul 19, 2023 •

edited

Loading

liach Jul 19, 2023

mlchung Jul 19, 2023

liach Jul 19, 2023

mlchung Jul 19, 2023

JornVernee left a comment

JornVernee Jul 19, 2023 •

edited

Loading

liach commented Jul 19, 2023

liach commented Jul 19, 2023

mlchung commented Jul 19, 2023

liach left a comment

liach commented Jul 20, 2023

JornVernee Jul 20, 2023

openjdk bot commented Jul 20, 2023 •

edited

Loading

JornVernee Jul 20, 2023

liach Jul 20, 2023

JornVernee Jul 20, 2023

mlchung Jul 20, 2023

mlchung commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

8199149: Improve the exception message thrown by VarHandle of unsupported operation #14928

8199149: Improve the exception message thrown by VarHandle of unsupported operation #14928

Conversation

mlchung commented Jul 19, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 19, 2023

openjdk bot commented Jul 19, 2023

mlbridge bot commented Jul 19, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JornVernee left a comment

Choose a reason for hiding this comment

JornVernee Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

liach commented Jul 19, 2023

liach commented Jul 19, 2023

mlchung commented Jul 19, 2023

liach left a comment

Choose a reason for hiding this comment

liach commented Jul 20, 2023

Choose a reason for hiding this comment

openjdk bot commented Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlchung commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

mlchung commented Jul 19, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 19, 2023 •

edited

Loading

JornVernee Jul 19, 2023 •

edited

Loading

openjdk bot commented Jul 20, 2023 •

edited

Loading