On Mon, 9 Aug 2021 13:13:33 GMT, Per Liden <pliden@openjdk.org> wrote:

While fixing JDK-8270626 it was discovered that the C2 intrinsic for `Reference.refersTo()` is not used as often as one would think. Instead C2 often prefers using the native implementation, which is much slower which defeats the purpose of having an intrinsic in the first place.

The problem arise from having `refersTo0()` be virtual, native and @IntrinsicCandidate. This can be worked around by introducing an virtual method layer and so that `refersTo0()` can be made `final`. That's what this patch does, by adding a virtual `refersToImpl()` which in turn calls the now final `refersTo0()`.

It should be noted that `Object.clone()` and `Object.hashCode()` are also virtual, native and @IntrinsicCandidate and these methods get special treatment by C2 to get the intrinsic generated more optimally. We might want to do a similar optimization for `Reference.refersTo0()`. In that case, it is suggested that such an improvement could come later and be handled as a separate enhancement, and @kimbarrett has already filed JDK-8272140 to track that.

Testing: I found this hard to test without instrumenting Hotspot to tell me that the intrinsic was called instead of the native method. So, for that reason testing was ad-hoc, using an instrumented Hotspot in combination with the test (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) that initially uncovered the problem. See JDK-8270626 for additional information.

Looks good. ------------- Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5052

On Mon, 9 Aug 2021 13:13:33 GMT, Per Liden <pliden@openjdk.org> wrote:

While fixing JDK-8270626 it was discovered that the C2 intrinsic for `Reference.refersTo()` is not used as often as one would think. Instead C2 often prefers using the native implementation, which is much slower which defeats the purpose of having an intrinsic in the first place.

The problem arise from having `refersTo0()` be virtual, native and @IntrinsicCandidate. This can be worked around by introducing an virtual method layer and so that `refersTo0()` can be made `final`. That's what this patch does, by adding a virtual `refersToImpl()` which in turn calls the now final `refersTo0()`.

It should be noted that `Object.clone()` and `Object.hashCode()` are also virtual, native and @IntrinsicCandidate and these methods get special treatment by C2 to get the intrinsic generated more optimally. We might want to do a similar optimization for `Reference.refersTo0()`. In that case, it is suggested that such an improvement could come later and be handled as a separate enhancement, and @kimbarrett has already filed JDK-8272140 to track that.

Testing: I found this hard to test without instrumenting Hotspot to tell me that the intrinsic was called instead of the native method. So, for that reason testing was ad-hoc, using an instrumented Hotspot in combination with the test (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) that initially uncovered the problem. See JDK-8270626 for additional information.

src/java.base/share/classes/java/lang/ref/Reference.java line 374:

372: * to call the native implementation over the intrinsic. 373: */ 374: boolean refersToImpl(T obj) {

I'm curious why can't you get rid of `refersToImpl` (virtual method) and just override `refersTo` on `PhantomReference`. Am I missing something important about keeping `refersTo` final? src/java.base/share/classes/java/lang/ref/Reference.java line 379:

377: 378: @IntrinsicCandidate 379: private native final boolean refersTo0(Object o);

No need to have it both `private` and `final`. Just `private` should be enough. ------------- PR: https://git.openjdk.java.net/jdk/pull/5052

While fixing JDK-8270626 it was discovered that the C2 intrinsic for `Reference.refersTo()` is not used as often as one would think. Instead C2 often prefers using the native implementation, which is much slower which defeats the purpose of having an intrinsic in the first place.

The problem arise from having `refersTo0()` be virtual, native and @IntrinsicCandidate. This can be worked around by introducing an virtual method layer and so that `refersTo0()` can be made `final`. That's what this patch does, by adding a virtual `refersToImpl()` which in turn calls the now final `refersTo0()`.

It should be noted that `Object.clone()` and `Object.hashCode()` are also virtual, native and @IntrinsicCandidate and these methods get special treatment by C2 to get the intrinsic generated more optimally. We might want to do a similar optimization for `Reference.refersTo0()`. In that case, it is suggested that such an improvement could come later and be handled as a separate enhancement, and @kimbarrett has already filed JDK-8272140 to track that.

Testing: I found this hard to test without instrumenting Hotspot to tell me that the intrinsic was called instead of the native method. So, for that reason testing was ad-hoc, using an instrumented Hotspot in combination with the test (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) that initially uncovered the problem. See JDK-8270626 for additional information.

Per Liden has updated the pull request incrementally with one additional commit since the last revision: Private implies final ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5052/files - new: https://git.openjdk.java.net/jdk/pull/5052/files/66743f76..3bc3d5e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5052&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5052&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5052.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5052/head:pull/5052 PR: https://git.openjdk.java.net/jdk/pull/5052

On Mon, 9 Aug 2021 13:13:33 GMT, Per Liden <pliden@openjdk.org> wrote:

While fixing JDK-8270626 it was discovered that the C2 intrinsic for `Reference.refersTo()` is not used as often as one would think. Instead C2 often prefers using the native implementation, which is much slower which defeats the purpose of having an intrinsic in the first place.

The problem arise from having `refersTo0()` be virtual, native and @IntrinsicCandidate. This can be worked around by introducing an virtual method layer and so that `refersTo0()` can be made `final`. That's what this patch does, by adding a virtual `refersToImpl()` which in turn calls the now final `refersTo0()`.

It should be noted that `Object.clone()` and `Object.hashCode()` are also virtual, native and @IntrinsicCandidate and these methods get special treatment by C2 to get the intrinsic generated more optimally. We might want to do a similar optimization for `Reference.refersTo0()`. In that case, it is suggested that such an improvement could come later and be handled as a separate enhancement, and @kimbarrett has already filed JDK-8272140 to track that.

Testing: I found this hard to test without instrumenting Hotspot to tell me that the intrinsic was called instead of the native method. So, for that reason testing was ad-hoc, using an instrumented Hotspot in combination with the test (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) that initially uncovered the problem. See JDK-8270626 for additional information.

This pull request has now been integrated. Changeset: 3f723ca4 Author: Per Liden <pliden@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/3f723ca4577b9cffeb6153ee386edd75f1df... Stats: 14 lines in 2 files changed: 11 ins; 0 del; 3 mod 8271862: C2 intrinsic for Reference.refersTo() is often not used Reviewed-by: kbarrett, mchung ------------- PR: https://git.openjdk.java.net/jdk/pull/5052