FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

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

Closed
joyeecheung wants to merge 2 commits into
nodejs:mainfrom
joyeecheung:gsab
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

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
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
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 commented Jul 7, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

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

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 added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2024

joyeecheung commented Jul 7, 2024

Copy link
Copy Markdown
Member Author

cc @legendecas

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2024

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

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.

This comment was marked as outdated.

Sign in to view

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 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 commented Jul 8, 2024

Copy link
Copy Markdown
Collaborator

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

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 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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 8, 2024
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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 9, 2024

nodejs-github-bot commented Jul 9, 2024

Copy link
Copy Markdown
Collaborator

Landed in 307430e...1a1639d

nodejs-github-bot pushed a commit that referenced this pull request Jul 9, 2024
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
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 mentioned this pull request Jul 9, 2024
Merged
aduh95 pushed a commit that referenced this pull request Jul 12, 2024
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
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 mentioned this pull request Jul 12, 2024
Merged
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
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
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
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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
legendecas legendecas approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL