Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
On Wed, 19 Jul 2023 00:52:49 GMT, Chen Liang wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review feedback
>
> src/java.base/share/classes/java/lang/invoke/IndirectVarHandle.java line 93:
>
>> 91: @ForceInline
>> 92: boolean checkAccessModeThenIsDirect(VarHandle.AccessDescriptor ad) {
>> 93: 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`.
> src/java.base/share/classes/java/lang/invoke/VarHandle.java line 2020:
>
>> 2018:
>> 2019: static AccessMode valueFromOrdinal(int mode) {
>> 2020: 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14928#discussion_r1267459610
PR Review Comment: https://git.openjdk.org/jdk/pull/14928#discussion_r1267459744
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
> `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: > > 1. change `VarHandle::checkAccessModeThenIsDirect` to check if the access > mode is unsupported. This check is only needed for direct var handle. > 2. 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]] Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/14928/files - new: https://git.openjdk.org/jdk/pull/14928/files/cc66b08c..176df233 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14928&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14928&range=00-01 Stats: 15 lines in 2 files changed: 0 ins; 11 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14928.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14928/head:pull/14928 PR: https://git.openjdk.org/jdk/pull/14928
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
On Wed, 19 Jul 2023 02:19:58 GMT, Mandy Chung wrote: >> `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: >> >> 1. change `VarHandle::checkAccessModeThenIsDirect` to check if the access >> mode is unsupported. This check is only needed for direct var handle. >> 2. 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]] > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/14928#pullrequestreview-1536143735
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
On Wed, 19 Jul 2023 02:19:58 GMT, Mandy Chung wrote: >> `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: >> >> 1. change `VarHandle::checkAccessModeThenIsDirect` to check if the access >> mode is unsupported. This check is only needed for direct var handle. >> 2. 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]] > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback > 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? src/java.base/share/classes/java/lang/invoke/VarHandle.java line 36: > 34: import java.util.HashMap; > 35: import java.util.List; > 36: import java.util.Map; These two new imports seem unused. - PR Review: https://git.openjdk.org/jdk/pull/14928#pullrequestreview-1538189132 PR Review Comment: https://git.openjdk.org/jdk/pull/14928#discussion_r1268779284
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
On Wed, 19 Jul 2023 23:37:41 GMT, Jorn Vernee wrote: > 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. - PR Comment: https://git.openjdk.org/jdk/pull/14928#issuecomment-1642899720
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
On Wed, 19 Jul 2023 02:19:58 GMT, Mandy Chung wrote: >> `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: >> >> 1. change `VarHandle::checkAccessModeThenIsDirect` to check if the access >> mode is unsupported. This check is only needed for direct var handle. >> 2. 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]] > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback Just curious, why does Paul Sandoz recommend removing the VarHandle itself from the UOE message? - PR Comment: https://git.openjdk.org/jdk/pull/14928#issuecomment-1642900664
Re: RFR: 8199149: Improve the exception message thrown by VarHandle of unsupported operation [v2]
On Wed, 19 Jul 2023 02:19:58 GMT, Mandy Chung wrote: >> `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: >> >> 1. change `VarHandle::checkAccessModeThenIsDirect` to check if the access >> mode is unsupported. This check is only needed for direct var handle. >> 2. 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]] > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback 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. - PR Comment: https://git.openjdk.org/jdk/pull/14928#issuecomment-1642902785
