8304138: [JVMCI] Test FailedSpeculation existence before appending. #13022

mur47x111 · 2023-03-14T14:55:18Z

Upon uncommon_trap, JVMCI runtime appends a FailedSpeculation entry to the nmethod using an atomic operation. It becomes a performance bottleneck when there is a large amount of (virtual) threads deoptimizing in the nmethod. In this PR, we test if a FailedSpeculation exists in the list before appending it.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8304138: [JVMCI] Test FailedSpeculation existence before appending.

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Doug Simon (@dougxc - Committer) ⚠️ Review applies to e8c7eec4

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/13022/head:pull/13022
$ git checkout pull/13022

Update a local copy of the PR:
$ git checkout pull/13022
$ git pull https://git.openjdk.org/jdk pull/13022/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13022

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13022.diff

bridgekeeper · 2023-03-14T14:56:55Z

👋 Welcome back yzheng! 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 · 2023-03-14T14:58:58Z

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

mur47x111 · 2023-03-14T15:01:12Z

/label remove hotspot

mur47x111 · 2023-03-14T15:01:27Z

/label add hotspot-compiler

openjdk · 2023-03-14T15:03:16Z

@mur47x111
The hotspot label was successfully removed.

openjdk · 2023-03-14T15:05:38Z

@mur47x111
The hotspot-compiler label was successfully added.

mlbridge · 2023-03-14T15:09:24Z

Webrevs

vnkozlov

I agree that it is better than before.
But without some kind of lock several threads can add the same speculation data in the window between new check and adding data to the list.

vnkozlov · 2023-03-14T15:28:31Z

src/hotspot/share/oops/methodData.cpp

@@ -846,7 +855,7 @@ bool FailedSpeculation::add_failed_speculation(nmethod* nm, FailedSpeculation**
  guarantee(is_aligned(fs, sizeof(FailedSpeculation*)), "FailedSpeculation objects must be pointer aligned");
  guarantee_failed_speculations_alive(nm, failed_speculations_address);

-  FailedSpeculation** cursor = failed_speculations_address;
+  cursor = failed_speculations_address;


Why not continue from cursor value from previous check loop?
Can the list be modified by other threads in between?

It cannot be modified in between. Addressed in e8c7eec by continuing from cursor

mur47x111 · 2023-03-14T15:41:54Z

I agree that it is better than before. But without some kind of lock several threads can add the same speculation data in the window between new check and adding data to the list.

Right. We can bear a few redundant FailedSpeculation entries, and would like to avoid locking in this fast path.

vnkozlov

Good.
Please, re-test with latest changes.

openjdk · 2023-03-14T16:08:22Z

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

8304138: [JVMCI] Test FailedSpeculation existence before appending.

Reviewed-by: kvn, dnsimon

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

f8482c2: 8297638: Memory leak in case of many started-dead threads
c56f011: 8298995: tools/jpackage/share/AddLauncherTest.java#id1 failed "AddLauncherTest.test; checks=53"
254288a: 8014021: TreeMaker.Params behaves inconsistently when the owning method has the same number of parameters as the number of parameter types requested
8f5bb53: 8015831: Add lint check for calling overridable methods from a constructor
b085ab9: 8180387: com.sun.source.util.JavacTask should have a protected constructor.
bfb812a: 8292818: replace 96-bit representation for field metadata with variable-sized streams
932be35: 8298469: Obsolete legacy parallel class loading workaround for non-parallel-capable class loaders
02a4ee2: 8303921: serviceability/sa/UniqueVtableTest.java timed out
4486f1b: 8304367: jlink --include-locales=* attempts to parse non .class resource files with classfile reader
8d2ebf2: 8303697: ProcessTools doesn't print last line of process output
... and 50 more: https://git.openjdk.org/jdk/compare/55aa122462c34d8f4cafa58f4d1f2d900449c83e...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vnkozlov, @dougxc) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mur47x111 · 2023-03-16T21:15:09Z

@vnkozlov could you please review fdf93ae that avoids duplicated FailedSpeculation entries?

vnkozlov · 2023-03-17T00:05:59Z

Yes, it would do what you are saying but I don't like infinite loops like that with conditional exists.
How big length of failed_speculations_address list you can have?
Consider implementing it as recursive method if depth is not big:

bool FailedSpeculation::add_failed_speculation(nmethod* nm, FailedSpeculation** failed_speculations_address, address speculation, int speculation_len) {
  assert(failed_speculations_address != nullptr, "must be");
  guarantee_failed_speculations_alive(nm, failed_speculations_address);

  size_t fs_size = sizeof(FailedSpeculation) + speculation_len;
  FailedSpeculation* fs = new (fs_size) FailedSpeculation(speculation, speculation_len);
  if (fs == nullptr) {
    // no memory -> ignore failed speculation
    return false;
  }
  guarantee(is_aligned(fs, sizeof(FailedSpeculation*)), "FailedSpeculation objects must be pointer aligned");
  
  if (!add_failed_speculation_recursive(failed_speculations_address, fs)) {
    delete fs;
    return false;
  }
  return true;
}

bool add_failed_speculation_recursive(FailedSpeculation** cursor, FailedSpeculation* fs) {
  if (*cursor == nullptr) {
    FailedSpeculation* old_fs = Atomic::cmpxchg(cursor, (FailedSpeculation*) nullptr, fs);
    if (old_fs == nullptr) {
      // Successfully appended fs to end of the list
      return true;
    }
    guarantee(*cursor != nullptr, "cursor must point to non-null FailedSpeculation");
  }
  // check if the current entry matches this thread's failed speculation
  int speculation_len = fs->data_len();
  if ((*cursor)->data_len() == speculation_len && memcmp(fs->data(), (*cursor)->data(), speculation_len) == 0) {
    return false;
  }
  return add_failed_speculation_recursive((*cursor)->next_adr(), fs);
}

dougxc · 2023-03-17T09:06:49Z

@vnkozlov your suggestion eagerly allocates a new FailedSpeculation. I'm also generally allergic to infinite loops but I don't want to ever have to worry about a stack overflow in this code as it will crash the VM. I think we should leave Yudi's code in its current form.

vnkozlov · 2023-03-17T16:08:38Z

@vnkozlov your suggestion eagerly allocates a new FailedSpeculation. I'm also generally allergic to infinite loops but I don't want to ever have to worry about a stack overflow in this code as it will crash the VM. I think we should leave Yudi's code in its current form.

Okay. You are the "boss" for this code ;^)

mur47x111 · 2023-03-18T08:42:06Z

@vnkozlov @dougxc thanks for the review! I will keep it as is then.

mur47x111 · 2023-03-18T08:42:33Z

/integrate

openjdk · 2023-03-18T08:43:26Z

@mur47x111
Your change (at version fdf93ae) is now ready to be sponsored by a Committer.

dougxc · 2023-03-18T09:39:47Z

/sponsor

openjdk · 2023-03-18T09:42:00Z

Going to push as commit 7503ecc.
Since your change was applied there have been 60 commits pushed to the master branch:

f8482c2: 8297638: Memory leak in case of many started-dead threads
c56f011: 8298995: tools/jpackage/share/AddLauncherTest.java#id1 failed "AddLauncherTest.test; checks=53"
254288a: 8014021: TreeMaker.Params behaves inconsistently when the owning method has the same number of parameters as the number of parameter types requested
8f5bb53: 8015831: Add lint check for calling overridable methods from a constructor
b085ab9: 8180387: com.sun.source.util.JavacTask should have a protected constructor.
bfb812a: 8292818: replace 96-bit representation for field metadata with variable-sized streams
932be35: 8298469: Obsolete legacy parallel class loading workaround for non-parallel-capable class loaders
02a4ee2: 8303921: serviceability/sa/UniqueVtableTest.java timed out
4486f1b: 8304367: jlink --include-locales=* attempts to parse non .class resource files with classfile reader
8d2ebf2: 8303697: ProcessTools doesn't print last line of process output
... and 50 more: https://git.openjdk.org/jdk/compare/55aa122462c34d8f4cafa58f4d1f2d900449c83e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-18T09:42:16Z

@dougxc @mur47x111 Pushed as commit 7503ecc.

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

[JVMCI] Test FailedSpeculation existence before appending.

2f9874d

openjdk bot added the rfr Pull request is ready for review label Mar 14, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 14, 2023

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Mar 14, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 14, 2023

vnkozlov reviewed Mar 14, 2023

View reviewed changes

avoid iterating from beginning.

e8c7eec

vnkozlov reviewed Mar 14, 2023

View reviewed changes

dougxc approved these changes Mar 14, 2023

View reviewed changes

vnkozlov approved these changes Mar 14, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 14, 2023

avoid duplicated entry.

fdf93ae

vnkozlov approved these changes Mar 17, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 18, 2023

openjdk bot added the integrated Pull request has been integrated label Mar 18, 2023

openjdk bot closed this Mar 18, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 18, 2023

mur47x111 deleted the FailedSpeculation branch March 18, 2023 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8304138: [JVMCI] Test FailedSpeculation existence before appending. #13022

8304138: [JVMCI] Test FailedSpeculation existence before appending. #13022

mur47x111 commented Mar 14, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

mur47x111 commented Mar 14, 2023

mur47x111 commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

mlbridge bot commented Mar 14, 2023 •

edited

Loading

vnkozlov left a comment

vnkozlov Mar 14, 2023

mur47x111 Mar 14, 2023

mur47x111 commented Mar 14, 2023

vnkozlov left a comment

openjdk bot commented Mar 14, 2023 •

edited

Loading

mur47x111 commented Mar 16, 2023

vnkozlov commented Mar 17, 2023

dougxc commented Mar 17, 2023

vnkozlov commented Mar 17, 2023

mur47x111 commented Mar 18, 2023

mur47x111 commented Mar 18, 2023

openjdk bot commented Mar 18, 2023

dougxc commented Mar 18, 2023

openjdk bot commented Mar 18, 2023

openjdk bot commented Mar 18, 2023

8304138: [JVMCI] Test FailedSpeculation existence before appending. #13022

8304138: [JVMCI] Test FailedSpeculation existence before appending. #13022

Conversation

mur47x111 commented Mar 14, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

mur47x111 commented Mar 14, 2023

mur47x111 commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

openjdk bot commented Mar 14, 2023

mlbridge bot commented Mar 14, 2023 • edited Loading

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov Mar 14, 2023

Choose a reason for hiding this comment

mur47x111 Mar 14, 2023

Choose a reason for hiding this comment

mur47x111 commented Mar 14, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 14, 2023 • edited Loading

mur47x111 commented Mar 16, 2023

vnkozlov commented Mar 17, 2023

dougxc commented Mar 17, 2023

vnkozlov commented Mar 17, 2023

mur47x111 commented Mar 18, 2023

mur47x111 commented Mar 18, 2023

openjdk bot commented Mar 18, 2023

dougxc commented Mar 18, 2023

openjdk bot commented Mar 18, 2023

openjdk bot commented Mar 18, 2023

mur47x111 commented Mar 14, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 14, 2023 •

edited

Loading

openjdk bot commented Mar 14, 2023 •

edited

Loading