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

deps: reland "Remove --harmony-rab-gsab" #53755

Closed
wants to merge 2 commits into from
Closed

deps: reland "Remove --harmony-rab-gsab" #53755

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jul 7, 2024 •
edited
Loading

The second commit was previously reverted in 8e33f20 because it broke the snapshot reproducibility test. It should be working now together with https://chromium-review.googlesource.com/c/v8/v8/+/5662576 which is the first commit in this PR (the commit order matters).

Fixes: #53579
joyeecheung and others added 2 commits July 7, 2024 20:22
@joyeecheung
deps: V8: cherry-pick e061cf9970d9
1db1737 
Original commit message:

    [arraybuffers] initialize max byte length of empty array buffers

    Without initializing the max byte length field, any empty array
    buffer captured in the snapshot can make the snapshot unreproducible.

    Refs: nodejs#53579

    Change-Id: I2489ab2e57ecbb405ec431a87d0acc92822b777c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5662576
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94754}

Refs: v8/v8@e061cf9
@legendecas @joyeecheung
deps: V8: cherry-pick 9ebca66a5740
7b774d6 
Original commit message:

    [rab/gsab] Remove --harmony-rab-gsab (has been on by default for a while)

    Bug: v8:11111
    Change-Id: Ie74e7737f3e2e8730820cf00f1cbc7ae02b515af
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5516580
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Shu-yu Guo <syg@chromium.org>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#93848}

Refs: v8/v8@9ebca66
PR-URL: nodejs#53522
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@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 Jul 7, 2024
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2024
@joyeecheung
Copy link
Member Author

cc @legendecas
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@legendecas
Copy link
Member

legendecas commented Jul 7, 2024 •
edited
Loading

The PR LGTM. However, the original PR CI was green: #53522 (comment). Is there anything missing from the CI? Or it could just be a flaky case.
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@joyeecheung
Copy link
Member Author

Or it could just be a flaky case.

I think there might be some subtle factors that affected the reproduction of that flake. For example, when I tried to debug that failure locally, either compiling the builds with symbols, or linking to an OpenSSL without overriding the preload paths would make it unreproducible. Considering that it's uninitialized memory that's making it fail, it kind of make sense since these may all affect the memory allocation behavior subtly. I guess the best we can do is to wait and see how green the CI is after this lands.
@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6069/
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60163/
@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6069/
@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6069/
@VoltrexKeyva VoltrexKeyva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 8, 2024
@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Jul 9, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 9, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 307430e...1a1639d
nodejs-github-bot pushed a commit that referenced this pull request Jul 9, 2024
@joyeecheung @nodejs-github-bot
deps: V8: cherry-pick e061cf9970d9
d15f514 
Original commit message:

    [arraybuffers] initialize max byte length of empty array buffers

    Without initializing the max byte length field, any empty array
    buffer captured in the snapshot can make the snapshot unreproducible.

    Refs: #53579

    Change-Id: I2489ab2e57ecbb405ec431a87d0acc92822b777c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5662576
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94754}

Refs: v8/v8@e061cf9
PR-URL: #53755
Fixes: #53579
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Jul 9, 2024
@legendecas @nodejs-github-bot
deps: V8: cherry-pick 9ebca66a5740
1a1639d 
Original commit message:

    [rab/gsab] Remove --harmony-rab-gsab (has been on by default for a while)

    Bug: v8:11111
    Change-Id: Ie74e7737f3e2e8730820cf00f1cbc7ae02b515af
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5516580
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Shu-yu Guo <syg@chromium.org>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#93848}

Refs: v8/v8@9ebca66
PR-URL: #53522
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #53755
Fixes: #53579
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@legendecas legendecas mentioned this pull request Jul 9, 2024
Open
aduh95 pushed a commit that referenced this pull request Jul 12, 2024
@joyeecheung @aduh95
deps: V8: cherry-pick e061cf9970d9
3be89cc 
Original commit message:

    [arraybuffers] initialize max byte length of empty array buffers

    Without initializing the max byte length field, any empty array
    buffer captured in the snapshot can make the snapshot unreproducible.

    Refs: #53579

    Change-Id: I2489ab2e57ecbb405ec431a87d0acc92822b777c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5662576
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94754}

Refs: v8/v8@e061cf9
PR-URL: #53755
Fixes: #53579
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
aduh95 pushed a commit that referenced this pull request Jul 12, 2024
@legendecas @aduh95
deps: V8: cherry-pick 9ebca66a5740
f1c30a3 
Original commit message:

    [rab/gsab] Remove --harmony-rab-gsab (has been on by default for a while)

    Bug: v8:11111
    Change-Id: Ie74e7737f3e2e8730820cf00f1cbc7ae02b515af
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5516580
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Shu-yu Guo <syg@chromium.org>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#93848}

Refs: v8/v8@9ebca66
PR-URL: #53522
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #53755
Fixes: #53579
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@aduh95 aduh95 mentioned this pull request Jul 12, 2024
Merged
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
@joyeecheung @aduh95
deps: V8: cherry-pick e061cf9970d9
29a734c 
Original commit message:

    [arraybuffers] initialize max byte length of empty array buffers

    Without initializing the max byte length field, any empty array
    buffer captured in the snapshot can make the snapshot unreproducible.

    Refs: #53579

    Change-Id: I2489ab2e57ecbb405ec431a87d0acc92822b777c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5662576
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94754}

Refs: v8/v8@e061cf9
PR-URL: #53755
Fixes: #53579
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
@legendecas @aduh95
deps: V8: cherry-pick 9ebca66a5740
8542ace 
Original commit message:

    [rab/gsab] Remove --harmony-rab-gsab (has been on by default for a while)

    Bug: v8:11111
    Change-Id: Ie74e7737f3e2e8730820cf00f1cbc7ae02b515af
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5516580
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Shu-yu Guo <syg@chromium.org>
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#93848}

Refs: v8/v8@9ebca66
PR-URL: #53522
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #53755
Fixes: #53579
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jul 17, 2024
@joyeecheung @targos
deps: V8: cherry-pick e061cf9970d9
1de19de 
Original commit message:

    [arraybuffers] initialize max byte length of empty array buffers

    Without initializing the max byte length field, any empty array
    buffer captured in the snapshot can make the snapshot unreproducible.

    Refs: nodejs#53579

    Change-Id: I2489ab2e57ecbb405ec431a87d0acc92822b777c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5662576
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94754}

Refs: v8/v8@e061cf9
PR-URL: nodejs#53755
Fixes: nodejs#53579
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jul 17, 2024
@joyeecheung @targos
deps: V8: cherry-pick e061cf9970d9
e8c5877 
Original commit message:

    [arraybuffers] initialize max byte length of empty array buffers

    Without initializing the max byte length field, any empty array
    buffer captured in the snapshot can make the snapshot unreproducible.

    Refs: nodejs#53579

    Change-Id: I2489ab2e57ecbb405ec431a87d0acc92822b777c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5662576
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#94754}

Refs: v8/v8@e061cf9
PR-URL: nodejs#53755
Fixes: nodejs#53579
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
5 participants
@joyeecheung @nodejs-github-bot @legendecas @benjamingr @VoltrexKeyva