8254793: [JVMCI] improve speculation encoding #667

dougxc · 2020-10-14T22:01:19Z

This PR changes the encoding of a jdk.vm.ci.hotspot.HotSpotSpeculationLog.HotSpotSpeculation from a long to an int. The Thread::_pending_failed_speculation field remains as a jlong since it is already exposed to JVMCI Java code already via VMStructs and this PR does not update its usage in Graal.

Progress

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

Testing

	Linux x64	Windows x64	macOS x64
Build	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

JDK-8254793: [JVMCI] improve speculation encoding

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Dean Long (@dean-long - Reviewer) ⚠️ Review applies to b0d93bd
Tom Rodriguez (@tkrodriguez - Reviewer) ⚠️ Review applies to b0d93bd

Download

$ git fetch https://git.openjdk.java.net/jdk pull/667/head:pull/667
$ git checkout pull/667

bridgekeeper · 2020-10-14T22:01:47Z

👋 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 · 2020-10-14T22:03:56Z

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

hotspot

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 · 2020-10-14T22:09:15Z

Webrevs

04: Full - Incremental (c34e25d)
03: Full - Incremental (b0d93bd)
02: Full - Incremental (2e4e4521d76007c3d018385109fa15e1bb5c13d3)
01: Full - Incremental (2fb4c99e6f44610b53b3252906c662033c234c4a)
00: Full (175c179da4d61d88a2aaf0f08257c003ddf22631)

vnkozlov · 2020-10-15T18:56:38Z

src/hotspot/share/runtime/thread.hpp

+  // uniquely identify the  speculative optimization guarded by the uncommon trap.
+  // The id value is only 32-bits but since this field is exposed via VMStructs to
+  // JVMCI as a jlong, it needs to be kept as a long to maintain backwards compatibility
+  // with JVMCI based compilers that emit code to update the field directly.
  jlong     _pending_failed_speculation;


I am confusing about backword compatibility comment. It said that old Graal (link in current JDK) generate code which writes 64 bits into this word. Will it use [32:32] index:length format or it will use new [0:27:5] format?
I don't see changes to Graal in this PR.

The version of Graal in the JDK does not change. It is agnostic about the encoding format. All is does is write a value to Thread::_pending_failed_speculation where said value is provided by JVMCI. The width of the write is determined by the width of the value. You can follow the code that does this here.

Based on some of my comments from elsewhere we've undone some of the original changes so it just produces int friendly long constants. Changing the actual encoding size poses some compatibility problems because we weren't careful enough to be completely size agnostic in Graal code.

Okay, that it what I looked for - the value encoding is provided by JVMCI. Good.

dougxc · 2020-10-16T16:34:52Z

I've updated this PR such that an encoded speculation value is always stored/transported in a long. The encoding still only uses 31 bits which means the instruction sequence emitted by Graal can still be optimized to a single store (e.g. on x86 a MOVESLQ can write a 32 bit value sign extended to a long into a long memory location).

vnkozlov

Good.

openjdk · 2020-10-16T17:34:55Z

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

8254793: [JVMCI] improve speculation encoding

Reviewed-by: kvn, dlong, never

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

e2e11d3: 8254955: x86: MethodHandlesAdapterBlob is too big
0b51016: 8253660: Need better error report when artifact resolution fails in AotCompiler.java
52cb329: 8254862: lldb in devkit doesn't work
60f63ec: 8254796: Cleanup pervasive unnecessary parameter
953e472: 8254967: com.sun.net.HttpsServer spins on TLS session close
1da28de: 8255009: delta apply fixes for JDK-8246774 and JDK-8253455, pushed too soon
a0382cd: 8253235: JFR.dump does not respect maxage parameter
cb7701b: 8253970: Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *' invalid)
4ffed32: 8254940: AArch64: Cleanup non-product thread members
cd66e0f: 8253877: gc/g1/TestGCLogMessages.java fails - missing "Evacuation failure" message
... and 43 more: https://git.openjdk.java.net/jdk/compare/03fa733e8b22eeeda8b381e33a6437bd03a28ac0...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 · 2020-10-16T16:56:01Z

src/hotspot/share/jvmci/jvmciRuntime.hpp

@@ -51,6 +52,21 @@ class JVMCINMethodData {
  // is appended when it causes a deoptimization.
  FailedSpeculation** _failed_speculations;

+  // A speculation id is an index (high 26 bits) and a length (low 5 bits).


We don't really have to enforce that it fits in an int any more. I think it would be more natural to allow to use all the remaining bits even though we'll never actually use that space in practice. Doing so makes the code look a little odd I think since there's no obvious reason for limit. We just want an encoding that's int friendly for the normal case.

All good points. Please confirm that 2 most recent commits to this PR align with your suggestions.

tkrodriguez · 2020-10-16T16:57:57Z

src/hotspot/share/runtime/thread.hpp

+  // uniquely identify the  speculative optimization guarded by the uncommon trap.
+  // The id value is only 32-bits but since this field is exposed via VMStructs to
+  // JVMCI as a jlong, it needs to be kept as a long to maintain backwards compatibility
+  // with JVMCI based compilers that emit code to update the field directly.
  jlong     _pending_failed_speculation;


Based on some of my comments from elsewhere we've undone some of the original changes so it just produces int friendly long constants. Changing the actual encoding size poses some compatibility problems because we weren't careful enough to be completely size agnostic in Graal code.

…peculation is reduced (to 5 bits)

dougxc · 2020-10-19T12:06:07Z

@dean-long @vnkozlov I've made a few new changes based on @tkrodriguez 's comments. Please let me know if it still looks good. I've also rerun mach5 testing on the new changes and everything passes.

tkrodriguez · 2020-10-19T18:09:16Z

New version looks good.

vnkozlov

Still good.

dougxc · 2020-10-19T18:54:27Z

Thanks for the reviews.

dougxc · 2020-10-19T19:00:47Z

/integrate

openjdk · 2020-10-19T19:07:00Z

@dougxc Since your change was applied there have been 54 commits pushed to the master branch:

74ac77e: 8247666: Support Lambda proxy classes in static CDS archive
e2e11d3: 8254955: x86: MethodHandlesAdapterBlob is too big
0b51016: 8253660: Need better error report when artifact resolution fails in AotCompiler.java
52cb329: 8254862: lldb in devkit doesn't work
60f63ec: 8254796: Cleanup pervasive unnecessary parameter
953e472: 8254967: com.sun.net.HttpsServer spins on TLS session close
1da28de: 8255009: delta apply fixes for JDK-8246774 and JDK-8253455, pushed too soon
a0382cd: 8253235: JFR.dump does not respect maxage parameter
cb7701b: 8253970: Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *' invalid)
4ffed32: 8254940: AArch64: Cleanup non-product thread members
... and 44 more: https://git.openjdk.java.net/jdk/compare/03fa733e8b22eeeda8b381e33a6437bd03a28ac0...master

Your commit was automatically rebased without conflicts.

Pushed as commit f42c032.

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

dougxc force-pushed the JDK-8254793 branch from 0886513 to 175c179 Compare October 14, 2020 22:02

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 14, 2020

dougxc changed the title ~~8254793: encode a HotSpotSpeculation in an int instead of a long~~ 8254793: [JVMCI] improve speculation encoding Oct 14, 2020

dougxc marked this pull request as ready for review October 14, 2020 22:05

openjdk bot added the rfr Pull request is ready for review label Oct 14, 2020

dougxc force-pushed the JDK-8254793 branch 2 times, most recently from 2fb4c99 to 2e4e452 Compare October 15, 2020 09:22

vnkozlov reviewed Oct 15, 2020

View reviewed changes

8254793: encode a HotSpotSpeculation in 31 bits

b0d93bd

dougxc force-pushed the JDK-8254793 branch from 2e4e452 to b0d93bd Compare October 16, 2020 16:20

vnkozlov approved these changes Oct 16, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 16, 2020

dean-long approved these changes Oct 16, 2020

View reviewed changes

tkrodriguez approved these changes Oct 16, 2020

View reviewed changes

dougxc added 2 commits October 18, 2020 23:44

simplified changes such that only the length component of an encode s…

a1b5d32

…peculation is reduced (to 5 bits)

require SHA1 algorithm to be present

c34e25d

vnkozlov approved these changes Oct 19, 2020

View reviewed changes

openjdk bot closed this Oct 19, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 19, 2020

dougxc deleted the JDK-8254793 branch October 20, 2020 08:45

yftsai mentioned this pull request Feb 17, 2022

8005885: enhance PrintCodeCache to print more data #7389

Closed

3 tasks

8254793: [JVMCI] improve speculation encoding #667

8254793: [JVMCI] improve speculation encoding #667

dougxc commented Oct 14, 2020 •

edited by openjdk bot

bridgekeeper bot commented Oct 14, 2020

openjdk bot commented Oct 14, 2020

mlbridge bot commented Oct 14, 2020 •

edited

vnkozlov Oct 15, 2020

dougxc Oct 16, 2020

tkrodriguez Oct 16, 2020

vnkozlov Oct 16, 2020

dougxc commented Oct 16, 2020

vnkozlov left a comment

openjdk bot commented Oct 16, 2020 •

edited

tkrodriguez Oct 16, 2020

dougxc Oct 18, 2020

tkrodriguez Oct 16, 2020

dougxc commented Oct 19, 2020

tkrodriguez commented Oct 19, 2020

vnkozlov left a comment

dougxc commented Oct 19, 2020

dougxc commented Oct 19, 2020

openjdk bot commented Oct 19, 2020

8254793: [JVMCI] improve speculation encoding #667

8254793: [JVMCI] improve speculation encoding #667

Conversation

dougxc commented Oct 14, 2020 • edited by openjdk bot

Progress

Testing

Issue

Reviewers

Download

bridgekeeper bot commented Oct 14, 2020

openjdk bot commented Oct 14, 2020

mlbridge bot commented Oct 14, 2020 • edited

Webrevs

vnkozlov Oct 15, 2020

Choose a reason for hiding this comment

dougxc Oct 16, 2020

Choose a reason for hiding this comment

tkrodriguez Oct 16, 2020

Choose a reason for hiding this comment

vnkozlov Oct 16, 2020

Choose a reason for hiding this comment

dougxc commented Oct 16, 2020

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 16, 2020 • edited

tkrodriguez Oct 16, 2020

Choose a reason for hiding this comment

dougxc Oct 18, 2020

Choose a reason for hiding this comment

tkrodriguez Oct 16, 2020

Choose a reason for hiding this comment

dougxc commented Oct 19, 2020

tkrodriguez commented Oct 19, 2020

vnkozlov left a comment

Choose a reason for hiding this comment

dougxc commented Oct 19, 2020

dougxc commented Oct 19, 2020

openjdk bot commented Oct 19, 2020

dougxc commented Oct 14, 2020 •

edited by openjdk bot

mlbridge bot commented Oct 14, 2020 •

edited

openjdk bot commented Oct 16, 2020 •

edited