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.

Sign up for GitHub

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

Jump to bottom

[electron26] Severe performance regression when running Code OSS #1

Closed
kxxt opened this issue Jun 4, 2024 · 9 comments
Closed

[electron26] Severe performance regression when running Code OSS #1

kxxt opened this issue Jun 4, 2024 · 9 comments
Labels
bug Something isn't working need-bisection

Comments

@kxxt
Copy link

kxxt commented Jun 4, 2024 •
edited
Loading

When running Code OSS with electron 26, 27 and 28, a severe performance regression renders it unusable.

Performance results:

code-oss 1.79 with  electron22: performance is normal
code-oss 1.83 with  electron25: performance is normal
code-oss 1.83 with  electron26: performance regressed
code-oss 1.88 with  electron28: performance regressed
vscodium 1.88 with  electron27: performance regressed
vscodium 1.88 with  electron28: performance regressed

It appears that we need to bisect [electron25.9.8, electron26.6.7] (corresponds to [chromium 114, chromium116]) to find the problematic commit.
@kxxt kxxt added bug Something isn't working need-bisection labels Jun 4, 2024
@kxxt
Copy link
Author

kxxt commented Jun 7, 2024

electron v26.0.0 (chromium 116, swiftshader llvm 16) is bad
electron v26.0.0-nightly.20230524 (chromium 115, swiftshader llvm 10) is bad

@kxxt
Copy link
Author

kxxt commented Jun 7, 2024 •
edited
Loading

electron v26.0.0-nightly.20230510 (chromium 114, swiftshader llvm 10) is good (but this build crashes frequently).

Bisection range shrink to v26.0.0-nightly.20230510..v26.0.0-nightly.20230524 (chromium 114.0.5719.0..115.0.5786.0).

@kxxt
Copy link
Author

kxxt commented Jun 8, 2024

electron v26.0.0-nightly.20230510r1 (chromium 115.0.5760.0, swiftshader llvm 10) is good.

Bisection range is shrinked to v26.0.0-nightly.20230510r1..v26.0.0-nightly.20230524 (chromium 115.0.5760.0..115.0.5786.0).

@kxxt
Copy link
Author

kxxt commented Jun 8, 2024

electron v26.0.0-nightly.20230523 (chromium 115.0.5760.0, swiftshader llvm 10) is good.

The bisection of electron is almost done. It's mostly confirmed that the bad commit is in chromium 115.0.5760.0..115.0.5786.0.

The next step is to bisect chromium.

@kxxt
Copy link
Author

kxxt commented Jun 8, 2024

electron v26.0.0-nightly.20230523cr5773.5 (chromium 115.0.5773.5, swiftshader llvm 10) is good.

@kxxt
Copy link
Author

kxxt commented Jun 8, 2024 •
edited
Loading

electron v26.0.0-nightly.20230523cr5778.0 (chromium 115.0.5778.0, swiftshader llvm 10) is bad.
electron v26.0.0-nightly.20230523cr5776.0 (chromium 115.0.5776.0, swiftshader llvm 10) is good.
electron v26.0.0-nightly.20230523cr.d7b8251eecba3 is good.
electron-v26.0.0-nightly.20230523cr.cf83bbaf3d150 is good.
electron-v26.0.0-nightly.20230523cr.8d2a7df7515e7 is bad.
electron-v26.0.0-nightly.20230523cr.59ee43fe7a7c4 is bad.
electron-v26.0.0-nightly.20230523cr.6a609fb2d6c29 is bad.
electron-v26.0.0-nightly.20230523cr.c1e4fa961824b is good.
electron-v26.0.0-nightly.20230523cr.c78c1a491fa10 is good.
bisection range: chromium (c78c1a491fa10..6a609fb2d6c29)

electron-v26.0.0-nightly.20230523cr.6a609fb2d6c29 (v8 11.5.138 revision 0f44e4c2a6fe21c4ef4cf1f1fba2aa145209d91d) is bad.
electron-v26.0.0-nightly.20230523cr.64570d53f431b (v8 11.5.137 revision 5e9b891a870ace09011d19c21da997ad86333d77) is good.

The bisection of chromium is done. The bad commit is 6a609fb2d6c29f25c2dbce26afa165c0e80f4768:

commit 6a609fb2d6c29f25c2dbce26afa165c0e80f4768
Author: v8-ci-autoroll-builder <v8-ci-autoroll-builder@chops-service-accounts.iam.gserviceaccount.com>
Date:   Wed May 17 17:38:50 2023 +0000

    Update V8 to version 11.5.138.
    
    Summary of changes available at:
    https://chromium.googlesource.com/v8/v8/+log/5e9b891a..0f44e4c2
    
    Please follow these instructions for assigning/CC'ing issues:
    https://v8.dev/docs/triage-issues
    
    Please close rolling in case of a roll revert:
    https://v8-roll.appspot.com/
    This only works with a Google account.
    
    CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux-blink-rel
    CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_optional_gpu_tests_rel
    CQ_INCLUDE_TRYBOTS=luci.chromium.try:mac_optional_gpu_tests_rel
    CQ_INCLUDE_TRYBOTS=luci.chromium.try:win_optional_gpu_tests_rel
    CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel
    
    R=hablich@chromium.org,vahl@chromium.org,v8-waterfall-sheriff@grotations.appspotmail.com
    
    Change-Id: Ib5f6f43ba7ecbbbbc93867850d149a52bd692483
    Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4544498
    Bot-Commit: v8-ci-autoroll-builder <v8-ci-autoroll-builder@chops-service-accounts.iam.gserviceaccount.com>
    Commit-Queue: v8-ci-autoroll-builder <v8-ci-autoroll-builder@chops-service-accounts.iam.gserviceaccount.com>
    Cr-Commit-Position: refs/heads/main@{#1145460}

 DEPS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The next step is to bisect v8: https://chromium.googlesource.com/v8/v8/+log/5e9b891a..0f44e4c2

@kxxt
Copy link
Author

kxxt commented Jun 9, 2024

electron v26.0.0-nightly.20230523cr.6a609fb2d6c29.v8.8d8a6c1 is good.

V8 commits left to bisect:

f6d1991 [riscv] Implement probe mmu mode by Lu Yahan · 1 year, 1 month ago
004ee17 [heap] Remove dead code from sweeper by Omer Katz · 1 year, 1 month ago

I am almost certain f6d1991 is the bad commit by looking at the code.

@kxxt
Copy link
Author

kxxt commented Jun 10, 2024

electron-v26.0.0-nightly.20230523cr.6a609fb2d6c29 (v8 11.5.138) with patch 4195a2b is good.

@kxxt
Copy link
Author

kxxt commented Jun 10, 2024 •
edited
Loading

Upstream V8 patch: https://chromium-review.googlesource.com/c/v8/v8/+/5612950

Fixed in v29.4.0.riscv2, v29.4.2.riscv2, v30.1.0.riscv2, v30.1.0.riscv2, v27.3.11.riscv2, v26.6.10.riscv2, v28.3.3.riscv2

@kxxt kxxt closed this as completed Jun 10, 2024
@kxxt kxxt mentioned this issue Jun 10, 2024
Merged
kxxt added a commit to kxxt/archriscv-packages that referenced this issue Jun 11, 2024
@kxxt
updpatch: nodejs 22.2.0-1
7615556 
Fix performance regression bisected in
riscv-forks/electron#1
. Upstreamed to V8:
https://chromium-review.googlesource.com/c/v8/v8/+/5612950
. Will upstream it to nodejs once the V8 change is merged.
@kxxt kxxt mentioned this issue Jun 11, 2024
Merged
hubot pushed a commit to v8/v8 that referenced this issue Jun 11, 2024
@kxxt
[riscv] avoid cpu probing in li_ptr
a3cc852 
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}
kxxt added a commit to kxxt/node that referenced this issue Jun 11, 2024
@kxxt
deps: V8: cherry-pick a3cc8522a4c8
10d1566 
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
@kxxt kxxt mentioned this issue Jun 11, 2024
Closed
felixonmars pushed a commit to felixonmars/archriscv-packages that referenced this issue Jun 11, 2024
@kxxt @felixonmars
updpatch: nodejs 22.2.0-1
488128e 
Fix performance regression bisected in
riscv-forks/electron#1
. Upstreamed to V8:
https://chromium-review.googlesource.com/c/v8/v8/+/5612950
. Will upstream it to nodejs once the V8 change is merged.
nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Jun 13, 2024
@kxxt @nodejs-github-bot
deps: V8: cherry-pick a3cc8522a4c8
3597070 
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>
kxxt added a commit to kxxt/archriscv-packages that referenced this issue Jun 15, 2024
@kxxt
updpatch: chromium 126.0.6478.55-1
6bf7758 
Update chromium riscv patch set for 126
(also used in electron31 here: riscv-forks/electron@c8e556a)

Notably,
- Update sandbox patch to latest version in upstream gerrit to fix crash
  due to sandbox blocked riscv_hwprobe. (Actually this should be fixed in version 125)
- Retrieve fix-rust-target.patch from elsewhere due to GitHub patch
  checksum mismatch
- Fix v8 performance regression: riscv-forks/electron#1
@kxxt kxxt mentioned this issue Jun 15, 2024
Merged
felixonmars pushed a commit to felixonmars/archriscv-packages that referenced this issue Jun 15, 2024
@kxxt @felixonmars
updpatch: chromium 126.0.6478.55-1
5fc678b 
Update chromium riscv patch set for 126
(also used in electron31 here: riscv-forks/electron@c8e556a)

Notably,
- Update sandbox patch to latest version in upstream gerrit to fix crash
  due to sandbox blocked riscv_hwprobe. (Actually this should be fixed in version 125)
- Retrieve fix-rust-target.patch from elsewhere due to GitHub patch
  checksum mismatch
- Fix v8 performance regression: riscv-forks/electron#1
targos pushed a commit to nodejs/node that referenced this issue Jun 20, 2024
@kxxt @targos
deps: V8: cherry-pick a3cc8522a4c8
bd68888 
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>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this issue Jun 20, 2024
@kxxt @EliphazBouye
deps: V8: cherry-pick a3cc8522a4c8
fc9793d 
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 issue Jun 22, 2024
@kxxt @bmeck
deps: V8: cherry-pick a3cc8522a4c8
71a676f 
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 issue Jun 30, 2024
@kxxt @targos
deps: V8: cherry-pick a3cc8522a4c8
b747f41 
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 issue Jul 17, 2024
@kxxt @targos
deps: V8: cherry-pick a3cc8522a4c8
3adceb4 
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 issue Jul 17, 2024
@kxxt @targos
deps: V8: cherry-pick a3cc8522a4c8
b23f333 
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>
kxxt added a commit to kxxt/archriscv-packages that referenced this issue Aug 14, 2024
@kxxt
updpatch: qt6-webengine 6.7.2-2
ad3e85c 
Pick up some chromium/v8 patches to

- Fix v8 performance regression
  (riscv-forks/electron#1)
- Fix rdcycle SIGILL on linux >= 6.6 (riscv-forks/electron#2)

There's no riscv specific security patch needs backporting for this
release.

Use nodejs in makedepends again because it seems nodejs no longer hangs
when building this package in qemu.
kxxt added a commit to kxxt/archriscv-packages that referenced this issue Aug 14, 2024
@kxxt
updpatch: qt6-webengine 6.7.2-2
9dc8ad7 
Pick up some chromium/v8 patches to

- Fix v8 performance regression
  (riscv-forks/electron#1)
- Fix rdcycle SIGILL on linux >= 6.6 (riscv-forks/electron#2)

There's no riscv specific security patch needs backporting for this
release.

Fix rotten and use nodejs in makedepends again because it seems nodejs no longer hangs
when building this package in qemu.
@kxxt kxxt mentioned this issue Aug 14, 2024
Merged
felixonmars pushed a commit to felixonmars/archriscv-packages that referenced this issue Aug 14, 2024
@kxxt @felixonmars
updpatch: qt6-webengine 6.7.2-2
5d6bad0 
Pick up some chromium/v8 patches to

- Fix v8 performance regression
  (riscv-forks/electron#1)
- Fix rdcycle SIGILL on linux >= 6.6 (riscv-forks/electron#2)

There's no riscv specific security patch needs backporting for this
release.

Fix rotten and use nodejs in makedepends again because it seems nodejs no longer hangs
when building this package in qemu.
@kxxt kxxt mentioned this issue Aug 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working need-bisection
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@kxxt