deps: reland "Remove --harmony-rab-gsab" by joyeecheung · Pull Request #53755 · nodejs/node · GitHub
Skip to content

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

Closed
joyeecheung wants to merge 2 commits into
nodejs:mainfrom
joyeecheung:gsab
Closed

deps: reland "Remove --harmony-rab-gsab"#53755
joyeecheung wants to merge 2 commits into
nodejs:mainfrom
joyeecheung:gsab

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Jul 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented Jul 7, 2024

Copy link
Copy Markdown
Collaborator

@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

joyeecheung commented Jul 7, 2024

Copy link
Copy Markdown
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

legendecas commented Jul 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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

joyeecheung commented Jul 8, 2024

Copy link
Copy Markdown
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

nodejs-github-bot commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60163/

@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6069/

@nodejs-github-bot

nodejs-github-bot commented Jul 8, 2024

Copy link
Copy Markdown
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

nodejs-github-bot commented Jul 9, 2024

Copy link
Copy Markdown
Collaborator

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
Merged
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>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
@joyeecheung @ehsankhfr
deps: V8: cherry-pick e061cf9970d9
5bf1c88 
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>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
@legendecas @ehsankhfr
deps: V8: cherry-pick 9ebca66a5740
c115d3d 
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>
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

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@legendecas legendecas legendecas approved these changes

Assignees

No one assigned

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test-snapshot-reproducible is failing in the main branch

6 participants

@joyeecheung @nodejs-github-bot @legendecas @benjamingr @targos @VoltrexKeyva