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: V8: backport 596d55a from upstream #19477

Closed
wants to merge 1 commit into from
Closed

deps: V8: backport 596d55a from upstream #19477

wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

Did not remove ActivationsFinder from src/runtime/runtime-compiler.cc
as in the original commit as the Class is still being used prior to
f0acede landing

Original Commit Message:

Deoptimization and multithreading.

When using Lockers and Unlockers it is possible to create a
scenario where multiple threads point to the same optimized
code object. When that happens, if one of the threads triggers
deoptimization, then the stack replacement needs to happen in
the stacks of all threads.
With this CL, the deoptimizer visits all threads to do so.
The CL also adds three tests where V8 used to crash due to this
issue.

Bug: v8:6563
Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
Reviewed-on: https://chromium-review.googlesource.com/670783
Reviewed-by: Jaroslav Sevcik <[email protected]>
Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
Cr-Commit-Position: refs/heads/master@{#48060}

Refs: v8/v8@596d55a
Refs: v8/v8@f0acede

/cc @nodejs/v8

@nodejs-github-bot nodejs-github-bot added v8 engine Issues and PRs related to the V8 dependency. v9.x labels Mar 20, 2018
@MylesBorins
deps: V8: backport 596d55a from upstream
f2a75d1 
Did not remove ActivationsFinder from `src/runtime/runtime-compiler.cc`
as in the original commit as the Class is still being used prior to
f0acede landing

Original Commit Message:

    Deoptimization and multithreading.

    When using Lockers and Unlockers it is possible to create a
    scenario where multiple threads point to the same optimized
    code object. When that happens, if one of the threads triggers
    deoptimization, then the stack replacement needs to happen in
    the stacks of all threads.
    With this CL, the deoptimizer visits all threads to do so.
    The CL also adds three tests where V8 used to crash due to this
    issue.

    Bug: v8:6563
    Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
    Reviewed-on: https://chromium-review.googlesource.com/670783
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#48060}

Refs: v8/v8@596d55a
Refs: v8/v8@f0acede
Fixes: nodejs#19274
@MylesBorins
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13767/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1277/

@MylesBorins
Copy link
Contributor Author

I'd like to fast track this to try and get it into today's 9.x release (and the upcoming 8.x rc)

@benjamn
Copy link
Contributor

benjamn commented Mar 20, 2018 •
edited
Loading

I will test this with my reproduction of #19274.

Update: It works when applied to v8.x-staging! 🎉

@MylesBorins
Copy link
Contributor Author

CI was being weird... one more TIME!!!

(V8-CI is green)

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

@AlinaNova21 AlinaNova21 mentioned this pull request Mar 20, 2018
Closed
benjamn added a commit to benjamn/node that referenced this pull request Mar 20, 2018
@benjamn
[v8.x backport] deps: cherry-pick 596d55a from upstream V8
b1a0556 
Analogous to this v9.x-staging PR submitted by @MylesBorins:
nodejs#19477

I can confirm this fixes nodejs#19274 for
the reproductions I've been using.

Original commit message:

    Deoptimization and multithreading.

    When using Lockers and Unlockers it is possible to create a
    scenario where multiple threads point to the same optimized
    code object. When that happens, if one of the threads triggers
    deoptimization, then the stack replacement needs to happen in
    the stacks of all threads.
    With this CL, the deoptimizer visits all threads to do so.
    The CL also adds three tests where V8 used to crash due to this
    issue.

    Bug: v8:6563
    Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
    Reviewed-on: https://chromium-review.googlesource.com/670783
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#48060}

Refs: v8/v8@596d55a
@benjamn benjamn mentioned this pull request Mar 20, 2018
Closed
@MylesBorins
Copy link
Contributor Author

CI is good and this is ready to land on 9.x and 8.x as soon as it gets sign off

bmeurer
bmeurer approved these changes Mar 21, 2018
View reviewed changes
Copy link
Member

@bmeurer bmeurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

MylesBorins added a commit that referenced this pull request Mar 21, 2018
@MylesBorins
deps: V8: backport 596d55a from upstream
36f664e 
Did not remove ActivationsFinder from `src/runtime/runtime-compiler.cc`
as in the original commit as the Class is still being used prior to
f0acede landing

Original Commit Message:

    Deoptimization and multithreading.

    When using Lockers and Unlockers it is possible to create a
    scenario where multiple threads point to the same optimized
    code object. When that happens, if one of the threads triggers
    deoptimization, then the stack replacement needs to happen in
    the stacks of all threads.
    With this CL, the deoptimizer visits all threads to do so.
    The CL also adds three tests where V8 used to crash due to this
    issue.

    Bug: v8:6563
    Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
    Reviewed-on: https://chromium-review.googlesource.com/670783
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#48060}

PR-URL: #19477
Fixes: #19274
Refs: v8/v8@596d55a
Refs: v8/v8@f0acede
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
MylesBorins added a commit that referenced this pull request Mar 21, 2018
@MylesBorins
deps: V8: backport 596d55a from upstream
d46fafc 
Did not remove ActivationsFinder from `src/runtime/runtime-compiler.cc`
as in the original commit as the Class is still being used prior to
f0acede landing

Original Commit Message:

    Deoptimization and multithreading.

    When using Lockers and Unlockers it is possible to create a
    scenario where multiple threads point to the same optimized
    code object. When that happens, if one of the threads triggers
    deoptimization, then the stack replacement needs to happen in
    the stacks of all threads.
    With this CL, the deoptimizer visits all threads to do so.
    The CL also adds three tests where V8 used to crash due to this
    issue.

    Bug: v8:6563
    Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
    Reviewed-on: https://chromium-review.googlesource.com/670783
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#48060}

PR-URL: #19477
Fixes: #19274
Refs: v8/v8@596d55a
Refs: v8/v8@f0acede
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
@MylesBorins
Copy link
Contributor Author

landed in 36f664e

@targos targos mentioned this pull request Mar 21, 2018
Merged
MylesBorins added a commit that referenced this pull request Mar 28, 2018
@MylesBorins
deps: V8: backport 596d55a from upstream
bc66f08 
Did not remove ActivationsFinder from `src/runtime/runtime-compiler.cc`
as in the original commit as the Class is still being used prior to
f0acede landing

Original Commit Message:

    Deoptimization and multithreading.

    When using Lockers and Unlockers it is possible to create a
    scenario where multiple threads point to the same optimized
    code object. When that happens, if one of the threads triggers
    deoptimization, then the stack replacement needs to happen in
    the stacks of all threads.
    With this CL, the deoptimizer visits all threads to do so.
    The CL also adds three tests where V8 used to crash due to this
    issue.

    Bug: v8:6563
    Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
    Reviewed-on: https://chromium-review.googlesource.com/670783
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#48060}

PR-URL: #19477
Fixes: #19274
Refs: v8/v8@596d55a
Refs: v8/v8@f0acede
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
benjamn pushed a commit to meteor/node that referenced this pull request Mar 29, 2018
@MylesBorins
deps: V8: backport 596d55a from upstream
639b332 
Did not remove ActivationsFinder from `src/runtime/runtime-compiler.cc`
as in the original commit as the Class is still being used prior to
f0acede landing

Original Commit Message:

    Deoptimization and multithreading.

    When using Lockers and Unlockers it is possible to create a
    scenario where multiple threads point to the same optimized
    code object. When that happens, if one of the threads triggers
    deoptimization, then the stack replacement needs to happen in
    the stacks of all threads.
    With this CL, the deoptimizer visits all threads to do so.
    The CL also adds three tests where V8 used to crash due to this
    issue.

    Bug: v8:6563
    Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
    Reviewed-on: https://chromium-review.googlesource.com/670783
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#48060}

PR-URL: nodejs#19477
Fixes: nodejs#19274
Refs: v8/v8@596d55a
Refs: v8/v8@f0acede
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
MylesBorins added a commit that referenced this pull request Mar 30, 2018
@MylesBorins
deps: V8: backport 596d55a from upstream
d02b72e 
Did not remove ActivationsFinder from `src/runtime/runtime-compiler.cc`
as in the original commit as the Class is still being used prior to
f0acede landing

Original Commit Message:

    Deoptimization and multithreading.

    When using Lockers and Unlockers it is possible to create a
    scenario where multiple threads point to the same optimized
    code object. When that happens, if one of the threads triggers
    deoptimization, then the stack replacement needs to happen in
    the stacks of all threads.
    With this CL, the deoptimizer visits all threads to do so.
    The CL also adds three tests where V8 used to crash due to this
    issue.

    Bug: v8:6563
    Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
    Reviewed-on: https://chromium-review.googlesource.com/670783
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#48060}

PR-URL: #19477
Fixes: #19274
Refs: v8/v8@596d55a
Refs: v8/v8@f0acede
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
benjamn pushed a commit to meteor/node that referenced this pull request Mar 30, 2018
@MylesBorins
deps: V8: backport 596d55a from upstream
7bcfbbd 
Did not remove ActivationsFinder from `src/runtime/runtime-compiler.cc`
as in the original commit as the Class is still being used prior to
f0acede landing

Original Commit Message:

    Deoptimization and multithreading.

    When using Lockers and Unlockers it is possible to create a
    scenario where multiple threads point to the same optimized
    code object. When that happens, if one of the threads triggers
    deoptimization, then the stack replacement needs to happen in
    the stacks of all threads.
    With this CL, the deoptimizer visits all threads to do so.
    The CL also adds three tests where V8 used to crash due to this
    issue.

    Bug: v8:6563
    Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
    Reviewed-on: https://chromium-review.googlesource.com/670783
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#48060}

PR-URL: nodejs#19477
Fixes: nodejs#19274
Refs: v8/v8@596d55a
Refs: v8/v8@f0acede
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
kentonv pushed a commit to sandstorm-io/node that referenced this pull request Apr 16, 2018
@MylesBorins @kentonv
deps: V8: backport 596d55a from upstream
0763074 
Did not remove ActivationsFinder from `src/runtime/runtime-compiler.cc`
as in the original commit as the Class is still being used prior to
f0acede landing

Original Commit Message:

    Deoptimization and multithreading.

    When using Lockers and Unlockers it is possible to create a
    scenario where multiple threads point to the same optimized
    code object. When that happens, if one of the threads triggers
    deoptimization, then the stack replacement needs to happen in
    the stacks of all threads.
    With this CL, the deoptimizer visits all threads to do so.
    The CL also adds three tests where V8 used to crash due to this
    issue.

    Bug: v8:6563
    Change-Id: I74e9af472d4833aa8d13e579df45133791f6a503
    Reviewed-on: https://chromium-review.googlesource.com/670783
    Reviewed-by: Jaroslav Sevcik <[email protected]>
    Commit-Queue: Juliana Patricia Vicente Franco <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#48060}

PR-URL: nodejs#19477
Fixes: nodejs#19274
Refs: v8/v8@596d55a
Refs: v8/v8@f0acede
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
@aboire aboire mentioned this pull request Apr 16, 2018
Merged
4 tasks
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaro-sevcik jaro-sevcik jaro-sevcik approved these changes

@devsnek devsnek devsnek approved these changes

@hashseed hashseed hashseed approved these changes

@bmeurer bmeurer bmeurer approved these changes

@fhinkel fhinkel fhinkel approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
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.
8 participants
@MylesBorins @benjamn @fhinkel @bmeurer @hashseed @devsnek @jaro-sevcik @nodejs-github-bot