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

[RISC-V] Fix performance regression #53412

Closed
wants to merge 2 commits into from

Conversation

kxxt
Copy link
Contributor

@kxxt kxxt commented Jun 11, 2024

Fix a performance regression in riscv64 v8, as bisected in riscv-forks/electron#1. It affects nodejs 21, 22 and the main branch. Other branches are not affected.

kxxt added 2 commits June 11, 2024 13:25
Original commit message:

    [riscv] Skip check sv57 when enable pointer compress

    Change-Id: I4332d3849d113af105630c0e20cd2b5e3deb9392
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5430889
    Commit-Queue: Ji Qiu <qiuji@iscas.ac.cn>
    Reviewed-by: Ji Qiu <qiuji@iscas.ac.cn>
    Cr-Commit-Position: refs/heads/main@{#93244}

Refs: v8/v8@6ea594f
Original commit message:

    [riscv] avoid cpu probing in li_ptr

    CPU probing is an expensive thing to do and we should avoid doing it upon every li_ptr call.

    Fixes performance regresion bisected in riscv-forks/electron#1

    Change-Id: Ib5ff89b2a730e08de6735123ae60adeffe811ed8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5612950
    Commit-Queue: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94349}

Refs: v8/v8@a3cc852
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Jun 11, 2024
@gengjiawen gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2024
@targos targos added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Jun 11, 2024
@targos targos added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. lts-watch-v20.x PRs that may need to be released in v20.x labels Jun 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS
Copy link
Member

FWIW v21 is now EOL, so it's unlikely to have this patch. It should land on main and v22 instead.

@richardlau richardlau added the riscv64 Issues and PRs related to the riscv64 architecture. label Jun 11, 2024
@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 12, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 13, 2024
nodejs-github-bot pushed a commit that referenced this pull request Jun 13, 2024
Original commit message:

    [riscv] Skip check sv57 when enable pointer compress

    Change-Id: I4332d3849d113af105630c0e20cd2b5e3deb9392
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5430889
    Commit-Queue: Ji Qiu <qiuji@iscas.ac.cn>
    Reviewed-by: Ji Qiu <qiuji@iscas.ac.cn>
    Cr-Commit-Position: refs/heads/main@{#93244}

Refs: v8/v8@6ea594f
PR-URL: #53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

Landed in 5a19a9b...3597070

nodejs-github-bot pushed a commit that referenced this pull request Jun 13, 2024
Original commit message:

    [riscv] avoid cpu probing in li_ptr

    CPU probing is an expensive thing to do and we should avoid doing it upon every li_ptr call.

    Fixes performance regresion bisected in riscv-forks/electron#1

    Change-Id: Ib5ff89b2a730e08de6735123ae60adeffe811ed8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5612950
    Commit-Queue: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94349}

Refs: v8/v8@a3cc852
PR-URL: #53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Jun 20, 2024
Original commit message:

    [riscv] Skip check sv57 when enable pointer compress

    Change-Id: I4332d3849d113af105630c0e20cd2b5e3deb9392
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5430889
    Commit-Queue: Ji Qiu <qiuji@iscas.ac.cn>
    Reviewed-by: Ji Qiu <qiuji@iscas.ac.cn>
    Cr-Commit-Position: refs/heads/main@{#93244}

Refs: v8/v8@6ea594f
PR-URL: #53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Jun 20, 2024
Original commit message:

    [riscv] avoid cpu probing in li_ptr

    CPU probing is an expensive thing to do and we should avoid doing it upon every li_ptr call.

    Fixes performance regresion bisected in riscv-forks/electron#1

    Change-Id: Ib5ff89b2a730e08de6735123ae60adeffe811ed8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5612950
    Commit-Queue: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94349}

Refs: v8/v8@a3cc852
PR-URL: #53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
Original commit message:

    [riscv] Skip check sv57 when enable pointer compress

    Change-Id: I4332d3849d113af105630c0e20cd2b5e3deb9392
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5430889
    Commit-Queue: Ji Qiu <qiuji@iscas.ac.cn>
    Reviewed-by: Ji Qiu <qiuji@iscas.ac.cn>
    Cr-Commit-Position: refs/heads/main@{#93244}

Refs: v8/v8@6ea594f
PR-URL: nodejs#53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
Original commit message:

    [riscv] avoid cpu probing in li_ptr

    CPU probing is an expensive thing to do and we should avoid doing it upon every li_ptr call.

    Fixes performance regresion bisected in riscv-forks/electron#1

    Change-Id: Ib5ff89b2a730e08de6735123ae60adeffe811ed8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5612950
    Commit-Queue: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94349}

Refs: v8/v8@a3cc852
PR-URL: nodejs#53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Original commit message:

    [riscv] Skip check sv57 when enable pointer compress

    Change-Id: I4332d3849d113af105630c0e20cd2b5e3deb9392
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5430889
    Commit-Queue: Ji Qiu <qiuji@iscas.ac.cn>
    Reviewed-by: Ji Qiu <qiuji@iscas.ac.cn>
    Cr-Commit-Position: refs/heads/main@{#93244}

Refs: v8/v8@6ea594f
PR-URL: nodejs#53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Original commit message:

    [riscv] avoid cpu probing in li_ptr

    CPU probing is an expensive thing to do and we should avoid doing it upon every li_ptr call.

    Fixes performance regresion bisected in riscv-forks/electron#1

    Change-Id: Ib5ff89b2a730e08de6735123ae60adeffe811ed8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5612950
    Commit-Queue: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94349}

Refs: v8/v8@a3cc852
PR-URL: nodejs#53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jun 30, 2024
Original commit message:

    [riscv] avoid cpu probing in li_ptr

    CPU probing is an expensive thing to do and we should avoid doing it upon every li_ptr call.

    Fixes performance regresion bisected in riscv-forks/electron#1

    Change-Id: Ib5ff89b2a730e08de6735123ae60adeffe811ed8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5612950
    Commit-Queue: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94349}

Refs: v8/v8@a3cc852
PR-URL: nodejs#53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jul 17, 2024
Original commit message:

    [riscv] avoid cpu probing in li_ptr

    CPU probing is an expensive thing to do and we should avoid doing it upon every li_ptr call.

    Fixes performance regresion bisected in riscv-forks/electron#1

    Change-Id: Ib5ff89b2a730e08de6735123ae60adeffe811ed8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5612950
    Commit-Queue: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94349}

Refs: v8/v8@a3cc852
PR-URL: nodejs#53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jul 17, 2024
Original commit message:

    [riscv] avoid cpu probing in li_ptr

    CPU probing is an expensive thing to do and we should avoid doing it upon every li_ptr call.

    Fixes performance regresion bisected in riscv-forks/electron#1

    Change-Id: Ib5ff89b2a730e08de6735123ae60adeffe811ed8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5612950
    Commit-Queue: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Yahan Lu <yahan@iscas.ac.cn>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94349}

Refs: v8/v8@a3cc852
PR-URL: nodejs#53412
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@marco-ippolito marco-ippolito added dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. and removed lts-watch-v20.x PRs that may need to be released in v20.x labels Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. riscv64 Issues and PRs related to the riscv64 architecture. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants