8255606: Enable concurrent stack processing on x86_32 platforms #945

zhengyu123 · 2020-10-29T20:40:30Z

Progress

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

Testing

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

Issue

JDK-8255606: Enable concurrent stack processing on x86_32 platforms

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Roman Kennke (@rkennke - Reviewer)
Erik Österlund (@fisk - Reviewer)

Download

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

bridgekeeper · 2020-10-29T20:41:38Z

👋 Welcome back zgu! 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-29T20:43:01Z

@zhengyu123 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-29T20:46:49Z

Webrevs

zhengyu123 · 2020-11-02T15:49:59Z

/label add hotspot-gc-dev
/label add shenandoah-dev

openjdk · 2020-11-02T15:52:08Z

@zhengyu123
The hotspot-gc label was successfully added.

openjdk · 2020-11-02T15:53:47Z

@zhengyu123
The shenandoah label was successfully added.

zhengyu123 · 2020-11-02T17:30:04Z

/cc hotspot-gc shenandoah

openjdk · 2020-11-02T17:33:38Z

@zhengyu123 The hotspot-gc label was already applied.
The shenandoah label was already applied.

shipilev

I have minor comments.

shipilev · 2020-11-03T12:09:36Z

src/hotspot/cpu/x86/x86_32.ad

@@ -653,8 +653,9 @@ void MachEpilogNode::format( PhaseRegAlloc *ra_, outputStream* st ) const {
  }
  st->print_cr("POPL   EBP"); st->print("\t");
  if (do_polling() && C->is_method_compilation()) {
-    st->print("TEST   PollPage,EAX\t! Poll Safepoint");
-    st->cr(); st->print("\t");
+    st->print("cmpptr   rsp, poll_offset[thread]  \n\t"


Let's say CMPL here? I don't think we reference cmpptr in outputs here. Also, note every instruction is capitalized in x86_32.ad.

Thanks for the review. Updated according to your suggestions.

shipilev · 2020-11-03T12:13:01Z

src/hotspot/cpu/x86/x86_64.ad

@@ -930,8 +930,8 @@ void MachEpilogNode::format(PhaseRegAlloc* ra_, outputStream* st) const
  st->print_cr("popq    rbp");
  if (do_polling() && C->is_method_compilation()) {
    st->print("\t");
-    st->print_cr("cmpq    poll_offset[r15_thread], rsp\n\t"
-                 "ja      #safepoint_stub\t"
+    st->print_cr("cmpptr   rsp, poll_offset[r15_thread] \n\t"


Ditto, leave cmpq here.

rkennke

Looks good to me! Thanks!

openjdk · 2020-11-03T16:50:45Z

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

8255606: Enable concurrent stack processing on x86_32 platforms

Reviewed-by: shade, rkennke, eosterlund

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

ca216ba: 8255810: Zero: build fails without JVMTI
d47336b: 8248320: Provide a unique accessible name for
64a9811: 8255798: Remove dead headless code in CompileJavaModules.gmk
364b0fe: 8255801: Race when building ct.sym build tools
f389a71: 8255737: Zero: DO_UPDATE_INSTRUCTION_COUNT should only update when relevant VM flags are set
93ef009: 8255762: Shenandoah: Consolidate/streamline interpreter LRBs
63461d5: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown
b8d4e02: 8255374: Add a dropReturn MethodHandle combinator
1d0bd50: 8254758: Change G1ServiceThread to be task based
9a0cf58: 8255615: Zero: demote ZeroStack::abi_stack_available guarantee to assert
... and 75 more: https://git.openjdk.java.net/jdk/compare/5c520c3fdd62745deb4145061e75949e74450a3f...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.

fisk

Looks good. Nice to see one more platform support this. Hoping that eventually all platforms will support this.

zhengyu123 · 2020-11-03T17:09:59Z

Looks good. Nice to see one more platform support this. Hoping that eventually all platforms will support this.

Thanks, Erik.

zhengyu123 · 2020-11-03T17:22:53Z

/integrate

openjdk · 2020-11-03T17:26:19Z

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

ca216ba: 8255810: Zero: build fails without JVMTI
d47336b: 8248320: Provide a unique accessible name for
64a9811: 8255798: Remove dead headless code in CompileJavaModules.gmk
364b0fe: 8255801: Race when building ct.sym build tools
f389a71: 8255737: Zero: DO_UPDATE_INSTRUCTION_COUNT should only update when relevant VM flags are set
93ef009: 8255762: Shenandoah: Consolidate/streamline interpreter LRBs
63461d5: 8255072: [TESTBUG] com/sun/jdi/EATests.java should not fail if expected VMOutOfMemoryException is not thrown
b8d4e02: 8255374: Add a dropReturn MethodHandle combinator
1d0bd50: 8254758: Change G1ServiceThread to be task based
9a0cf58: 8255615: Zero: demote ZeroStack::abi_stack_available guarantee to assert
... and 75 more: https://git.openjdk.java.net/jdk/compare/5c520c3fdd62745deb4145061e75949e74450a3f...master

Your commit was automatically rebased without conflicts.

Pushed as commit 134e22a.

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

Reviewed-by: shade, rkennke, eosterlund

Zhengyu Gu added 2 commits October 29, 2020 11:40

8255606: Enable concurrent stack processing on x86_32 platforms

cebf33c

Merge branch 'master' into JDK-8255606-conc-stack-x86_32

215f7f7

openjdk bot added hotspot hotspot-dev@openjdk.org rfr Pull request is ready for review labels Oct 29, 2020

Fix jump direction

c946b81

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Nov 2, 2020

openjdk bot added the shenandoah shenandoah-dev@openjdk.org label Nov 2, 2020

shipilev suggested changes Nov 3, 2020

View reviewed changes

Zhengyu Gu added 2 commits November 3, 2020 08:37

Reverted back to cmpq/cmpl

d6a8822

Capitalize instructions in x86_32.ad

28c69c4

rkennke approved these changes Nov 3, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 3, 2020

fisk approved these changes Nov 3, 2020

View reviewed changes

shipilev approved these changes Nov 3, 2020

View reviewed changes

openjdk bot closed this Nov 3, 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 Nov 3, 2020

zhengyu123 deleted the JDK-8255606-conc-stack-x86_32 branch November 3, 2020 17:24

openjdk-notifier bot referenced this pull request Nov 3, 2020

8255606: Enable concurrent stack processing on x86_32 platforms

134e22a

Reviewed-by: shade, rkennke, eosterlund

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8255606: Enable concurrent stack processing on x86_32 platforms #945

8255606: Enable concurrent stack processing on x86_32 platforms #945

zhengyu123 commented Oct 29, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 29, 2020

openjdk bot commented Oct 29, 2020

mlbridge bot commented Oct 29, 2020 •

edited

Loading

zhengyu123 commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

zhengyu123 commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

shipilev left a comment

shipilev Nov 3, 2020

zhengyu123 Nov 3, 2020

shipilev Nov 3, 2020

rkennke left a comment

openjdk bot commented Nov 3, 2020 •

edited

Loading

fisk left a comment

zhengyu123 commented Nov 3, 2020

zhengyu123 commented Nov 3, 2020

openjdk bot commented Nov 3, 2020

8255606: Enable concurrent stack processing on x86_32 platforms #945

8255606: Enable concurrent stack processing on x86_32 platforms #945

Conversation

zhengyu123 commented Oct 29, 2020 • edited by openjdk bot Loading

Progress

Testing

Issue

Reviewers

Download

bridgekeeper bot commented Oct 29, 2020

openjdk bot commented Oct 29, 2020

mlbridge bot commented Oct 29, 2020 • edited Loading

Webrevs

zhengyu123 commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

zhengyu123 commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

shipilev left a comment

Choose a reason for hiding this comment

shipilev Nov 3, 2020

Choose a reason for hiding this comment

zhengyu123 Nov 3, 2020

Choose a reason for hiding this comment

shipilev Nov 3, 2020

Choose a reason for hiding this comment

rkennke left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 3, 2020 • edited Loading

fisk left a comment

Choose a reason for hiding this comment

zhengyu123 commented Nov 3, 2020

zhengyu123 commented Nov 3, 2020

openjdk bot commented Nov 3, 2020

zhengyu123 commented Oct 29, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 29, 2020 •

edited

Loading

openjdk bot commented Nov 3, 2020 •

edited

Loading