Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271862: C2 intrinsic for Reference.refersTo() is often not used #5052

Closed

Conversation

@pliden
Copy link
Contributor

@pliden pliden commented Aug 9, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8271862: C2 intrinsic for Reference.refersTo() is often not used

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5052/head:pull/5052
$ git checkout pull/5052

Update a local copy of the PR:
$ git checkout pull/5052
$ git pull https://git.openjdk.java.net/jdk pull/5052/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5052

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5052.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 9, 2021

👋 Welcome back pliden! 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.

Loading

@pliden
Copy link
Contributor Author

@pliden pliden commented Aug 9, 2021

/cc core-libs hotspot-gc hotspot-compiler

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 9, 2021

@pliden 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 9, 2021

@pliden The core-libs label was already applied.

The hotspot-gc label was successfully added.

The hotspot-compiler label was successfully added.

Loading

@pliden pliden marked this pull request as ready for review Aug 9, 2021
@openjdk openjdk bot added the rfr label Aug 9, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 9, 2021

Webrevs

Loading

Copy link

@kimbarrett kimbarrett left a comment

Looks good.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 9, 2021

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

8271862: C2 intrinsic for Reference.refersTo() is often not used

Reviewed-by: kbarrett, mchung

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:

  • 1f88134: 8271869: AArch64: build errors with GCC11 in frame::saved_oop_result
  • 089e83b: 8266490: Extend the OSContainer API to support the pids controller of cgroups
  • 2384e12: 8270098: ZGC: ZBarrierSetC2::clone_at_expansion fails with "Guard against surprises" assert
  • d53d94b: 8271925: ZGC: Arraycopy stub passes invalid oop to load barrier
  • 3b899ef: 8272168: some hotspot runtime/logging tests don't check exit code
  • abdc107: 8270454: G1: Simplify region index comparison
  • eb6f3fe: 8272169: runtime/logging/LoaderConstraintsTest.java doesn't build test.Empty
  • 9654fd7: 8271892: mark hotspot runtime/PrintStringTableStats/PrintStringTableStatsTest.java test as ignoring external VM flags
  • 843943c: 8263567: gtests don't terminate the VM safely
  • 7fc99cf: 8225488: Examine ExecutableType.getReceiverType behavior when source receiver parameter is absent
  • ... and 16 more: https://git.openjdk.java.net/jdk/compare/b6a19f173bc6c07622633c9d6757d96a95b43398...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.

Loading

@openjdk openjdk bot added the ready label Aug 9, 2021
mlchung
mlchung approved these changes Aug 9, 2021
Copy link
Member

@mlchung mlchung left a comment

Looks good to me too.

Loading

*/
boolean refersToImpl(T obj) {
Copy link

@iwanowww iwanowww Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Copy link
Member

@mlchung mlchung Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want user-defined subclass of Reference overriding the refersTo implementation accidentally.

Loading

Copy link

@iwanowww iwanowww Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks for the clarification!

Loading

@IntrinsicCandidate
native boolean refersTo0(Object o);
private native final boolean refersTo0(Object o);
Copy link

@iwanowww iwanowww Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to have it both private and final. Just private should be enough.

Loading

Copy link
Contributor Author

@pliden pliden Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Fixed.

Loading

@pliden
Copy link
Contributor Author

@pliden pliden commented Aug 11, 2021

Thanks all for reviewing!

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 11, 2021

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

  • abebbe2: 8267186: Add string deduplication support to ZGC
  • 0d0f2d0: 8272216: G1: replace G1ParScanThreadState::_dest with a constant
  • 3215dbc: 8271928: ErroneousTree with start position -1
  • adba09b: 8272146: Disable Fibonacci test on memory constrained systems
  • 846cc88: 8272138: ZGC: Adopt relaxed ordering for self-healing
  • 5350b99: 8272131: PhaseMacroExpand::generate_slow_arraycopy crash when clone null CallProjections.fallthrough_ioproj
  • 1489352: 8271718: Crash when during color transformation the color profile is replaced
  • 2a9acc3: 8272050: typo in MachSpillCopyNode::implementation after JDK-8131362
  • b62e742: 8213714: AttachingConnector/attach/attach001 failed due to "bind failed: Address already in use"
  • 66d1faa: 8271601: Math.floorMod(int, int) and Math.floorMod(long, long) differ in their logic
  • ... and 31 more: https://git.openjdk.java.net/jdk/compare/b6a19f173bc6c07622633c9d6757d96a95b43398...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Aug 11, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Aug 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 11, 2021

@pliden Pushed as commit 3f723ca.

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

Loading

@pliden pliden deleted the 8271862_c2_intrinsic_for_refersTo branch Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants