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

deps: backport V8 fix related to WASM deadlock by targos · Pull Request #47610 · nodejs/node · GitHub

/ node Public

deps: backport V8 fix related to WASM deadlock - #47610

Closed
targos wants to merge 2 commits into
nodejs:mainfrom
targos:v8-deadlock
ConversationCommits (2)ChecksFiles changed
Closed

deps: backport V8 fix related to WASM deadlock#47610
targos wants to merge 2 commits into
nodejs:mainfrom
targos:v8-deadlock

Conversation

targos commented Apr 18, 2023

Copy link
Copy Markdown
Member

The first commit is just to avoid conflicts with the second one which contains the fix.

Refs: #47297

nodejs-github-bot commented Apr 18, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @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 Apr 18, 2023
targos added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2023

nodejs-github-bot commented Apr 18, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 18, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 18, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 18, 2023

Copy link
Copy Markdown
Collaborator

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

targos commented Apr 18, 2023

Copy link
Copy Markdown
Member Author

I'll remove the reverts because the tests are still failing.

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2023

nodejs-github-bot commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

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

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2023

nodejs-github-bot commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

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

This was referenced Apr 24, 2023
Open
Open
targos added 2 commits May 1, 2023 12:42
deps: V8: cherry-pick a8a11a87cb72
c63d47c 
Original commit message:

    [wasm] Simplify CompileJSToWasmWrapperJob

    This CL merges some of AsyncCompileJSToWasmWrapperJob and
    CompileJSToWasmWrapperJob into a common base class. Both jobs now
    loop on a vector with an atomic index.

    Bug: chromium:1423615
    Change-Id: I7bb9cb2a57d8b659766c01e20e38d1b859d3a987
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4347597
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#86675}

Refs: v8/v8@a8a11a8
deps: V8: cherry-pick 5f025d1ca2ca
a62c955 
Original commit message:

    [wasm] Fix deadlock in async wrapper compilation

    If compilation is cancelled while wrapper compilation is running, the
    tasks spawned for the {AsyncCompileJSToWasmWrapperJob} will return
    immediately, but {GetMaxConcurrency} will still return a positive
    value. Hence {Join()} will spawn another task, resulting in a
    livelock.

    We could fix this by checking for cancellation in {GetMaxConcurrency},
    but that requires taking the compilation state lock.
    So this CL fixes the issue by dropping the number of outstanding
    compilation units by to (basically) zero. We can't unconditionally drop
    to zero because another thread might concurrently execute a wrapper
    compilation and still call {CompleteUnit} afterwards. Hence only drop
    outstanding units by the amount of not-yet-started units.

    R=jkummerow@chromium.org

    Bug: v8:13858
    Change-Id: I5398ef370da2e7f212ca772fd1f87f659929dd6d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4437531
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87143}

Refs: v8/v8@5f025d1
targos added the request-ci Add this label to start a Jenkins CI on a PR. label May 1, 2023

targos commented May 1, 2023

Copy link
Copy Markdown
Member Author

rebased

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 1, 2023

nodejs-github-bot commented May 1, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 1, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 1, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 1, 2023

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request May 3, 2023
Open
33 tasks

nodejs-github-bot commented May 3, 2023

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request May 4, 2023
Open
28 tasks

nodejs-github-bot commented May 4, 2023

Copy link
Copy Markdown
Collaborator

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

This was referenced May 5, 2023
Open
Open
Open

targos commented May 10, 2023

Copy link
Copy Markdown
Member Author

Landed in 0736d0b...38d261a

targos added a commit that referenced this pull request May 10, 2023
deps: V8: cherry-pick a8a11a87cb72
4b4a0c4 
Original commit message:

    [wasm] Simplify CompileJSToWasmWrapperJob

    This CL merges some of AsyncCompileJSToWasmWrapperJob and
    CompileJSToWasmWrapperJob into a common base class. Both jobs now
    loop on a vector with an atomic index.

    Bug: chromium:1423615
    Change-Id: I7bb9cb2a57d8b659766c01e20e38d1b859d3a987
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4347597
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#86675}

Refs: v8/v8@a8a11a8
PR-URL: #47610
Refs: #47297
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request May 10, 2023
deps: V8: cherry-pick 5f025d1ca2ca
38d261a 
Original commit message:

    [wasm] Fix deadlock in async wrapper compilation

    If compilation is cancelled while wrapper compilation is running, the
    tasks spawned for the {AsyncCompileJSToWasmWrapperJob} will return
    immediately, but {GetMaxConcurrency} will still return a positive
    value. Hence {Join()} will spawn another task, resulting in a
    livelock.

    We could fix this by checking for cancellation in {GetMaxConcurrency},
    but that requires taking the compilation state lock.
    So this CL fixes the issue by dropping the number of outstanding
    compilation units by to (basically) zero. We can't unconditionally drop
    to zero because another thread might concurrently execute a wrapper
    compilation and still call {CompleteUnit} afterwards. Hence only drop
    outstanding units by the amount of not-yet-started units.

    R=jkummerow@chromium.org

    Bug: v8:13858
    Change-Id: I5398ef370da2e7f212ca772fd1f87f659929dd6d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4437531
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87143}

Refs: v8/v8@5f025d1
PR-URL: #47610
Refs: #47297
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos closed this May 10, 2023
targos deleted the v8-deadlock branch May 10, 2023 12:37
targos added a commit that referenced this pull request May 12, 2023
deps: V8: cherry-pick a8a11a87cb72
99f8fca 
Original commit message:

    [wasm] Simplify CompileJSToWasmWrapperJob

    This CL merges some of AsyncCompileJSToWasmWrapperJob and
    CompileJSToWasmWrapperJob into a common base class. Both jobs now
    loop on a vector with an atomic index.

    Bug: chromium:1423615
    Change-Id: I7bb9cb2a57d8b659766c01e20e38d1b859d3a987
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4347597
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#86675}

Refs: v8/v8@a8a11a8
PR-URL: #47610
Refs: #47297
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request May 12, 2023
deps: V8: cherry-pick 5f025d1ca2ca
b16f6da 
Original commit message:

    [wasm] Fix deadlock in async wrapper compilation

    If compilation is cancelled while wrapper compilation is running, the
    tasks spawned for the {AsyncCompileJSToWasmWrapperJob} will return
    immediately, but {GetMaxConcurrency} will still return a positive
    value. Hence {Join()} will spawn another task, resulting in a
    livelock.

    We could fix this by checking for cancellation in {GetMaxConcurrency},
    but that requires taking the compilation state lock.
    So this CL fixes the issue by dropping the number of outstanding
    compilation units by to (basically) zero. We can't unconditionally drop
    to zero because another thread might concurrently execute a wrapper
    compilation and still call {CompleteUnit} afterwards. Hence only drop
    outstanding units by the amount of not-yet-started units.

    R=jkummerow@chromium.org

    Bug: v8:13858
    Change-Id: I5398ef370da2e7f212ca772fd1f87f659929dd6d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4437531
    Commit-Queue: Clemens Backes <clemensb@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#87143}

Refs: v8/v8@5f025d1
PR-URL: #47610
Refs: #47297
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request May 15, 2023
Merged
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

jasnell jasnell approved these changes
richardlau richardlau approved these changes

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL