★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/25101Nouvelle recherche | Portail wanayoo
Skip to content

/node

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.

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

deps: V8: fix spread operator #25101

Closed
wants to merge 3 commits into from

Conversation

@BridgeAR
Copy link
Member

commented Dec 18, 2018

The spread operator currently mutates some input objects. This is already fixed in V8, so I just went ahead to backport these commits.

Fixes: #25089

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Dec 18, 2018

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1986/pipeline

@nodejs-github-bot nodejs-github-bot added build V8 Engine labels Dec 18, 2018

@richardlau
richardlau approved these changes Dec 18, 2018
View changes
@devsnek
devsnek approved these changes Dec 18, 2018
View changes
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 18, 2018

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

@BridgeAR BridgeAR added the author ready label Dec 18, 2018

@targos

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 18, 2018

It seems this depends on some other change
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 18, 2018

@targos seems so... the commits landed cleanly, that's why I expected it to work but that is obviously wrong.

@BridgeAR BridgeAR added work in progress (WIP) and removed author ready labels Dec 18, 2018

@jasnell
jasnell approved these changes Dec 18, 2018
View changes
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 18, 2018

Idea of landing this is +1 ... obviously need to figure out what other change may be needed :-)
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 18, 2018 •
edited

The linked CI above is for #22712. (Posting CI in the wrong window is something I've done many times myself.) The correct CI link is https://ci.nodejs.org/job/node-test-pull-request/19649/. Everything failed to compile so this definitely needs some adjustments (which is also indicated by the work in progress (WIP) label).
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 19, 2018

@Trott thanks for catching that!
BridgeAR added 3 commits Dec 20, 2018
@BridgeAR
deps: patch V8 to 7.1.302.33 
Refs: v8/v8@7.1.302.28...7.1.302.33
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
c9bbf5a
@BridgeAR
deps: V8: backport bf84766 
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers instead of referencing them

    Adds a helper macro "CloneIfMutablePrimitive", which tests if the
    operand is a MutableHeapNumber, and if so, clones it, otherwise
    returning the original value.

    Also modifies the signature of "CopyPropertyArrayValues" to take a
    "DestroySource" enum, indicating whether or not the resulting object is
    supplanting the source object or not, and removes all default
    parameters from that macro (which were not used anyways).

    This corrects the issue reported in chromium:901301, where
    StaNamedOwnProperty was replacing the value of a MutableHeapNumber
    referenced by both the cloned object and the source object.

    BUG=chromium:901301, v8:7611
    R=cbruni@chromium.org, jkummerow@chromium.org

    Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629
    Reviewed-on: https://chromium-review.googlesource.com/c/1318096
    Commit-Queue: Caitlin Potter <caitp@igalia.com>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57304}

Refs: v8/v8@bf84766
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
feba312
@BridgeAR
deps: V8: backport 3e010af 
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers only if !FLAG_unbox_double_fields

    Change the macros added in bf84766a2cd3e09070adcd6228a3a487c8dc4bbd to
    only do the hard work if FLAG_unbox_double_fields is unset (otherwise,
    they will attempt to dereference raw float64s, which is bad!)

    Also adds a write barrier in CopyPropertyArrayValues for each store if
    it's possible that a MutableHeapNumber is cloned.

    BUG=chromium:901301, chromium:902965, chromium:903070, v8:7611
    R=cbruni@chromium.org, jkummerow@chromium.org, ishell@chromium.org

    Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb
    Reviewed-on: https://chromium-review.googlesource.com/c/1323911
    Commit-Queue: Caitlin Potter <caitp@igalia.com>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57368}

Refs: v8/v8@3e010af
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
6096720

@BridgeAR BridgeAR force-pushed the BridgeAR:fix-spread-operator branch from 4d1ea2b to 6096720 Dec 20, 2018

@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 20, 2018

@GeorgNeis was so kind to provide a patch that resolved the build issues. Since the patch was build upon 7.1.302.32, I went ahead and updated V8 first and then applied the necessary backports on top of that including the changes from the mentioned patch.

This will therefore also improve the Array.prototype.splice performance and add some other minor improvements.

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

@BridgeAR BridgeAR removed the work in progress (WIP) label Dec 20, 2018

@targos

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 20, 2018

/cc @nodejs/v8-update
@ofrobots
ofrobots approved these changes Dec 20, 2018
View changes
@hashseed
hashseed approved these changes Dec 20, 2018
View changes

@BridgeAR BridgeAR added the author ready label Dec 20, 2018

@danbev

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 21, 2018

Landed in a981214, b5784fe, and 4884ca6

@danbev danbev closed this Dec 21, 2018

danbev added a commit that referenced this pull request Dec 21, 2018
@BridgeAR @danbev
deps: patch V8 to 7.1.302.33 
PR-URL: #25101
Refs: v8/v8@7.1.302.28...7.1.302.33
Fixes: #25089
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
a981214
danbev added a commit that referenced this pull request Dec 21, 2018
@BridgeAR @danbev
deps: V8: backport bf84766 
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers instead of referencing them

    Adds a helper macro "CloneIfMutablePrimitive", which tests if the
    operand is a MutableHeapNumber, and if so, clones it, otherwise
    returning the original value.

    Also modifies the signature of "CopyPropertyArrayValues" to take a
    "DestroySource" enum, indicating whether or not the resulting object is
    supplanting the source object or not, and removes all default
    parameters from that macro (which were not used anyways).

    This corrects the issue reported in chromium:901301, where
    StaNamedOwnProperty was replacing the value of a MutableHeapNumber
    referenced by both the cloned object and the source object.

    BUG=chromium:901301, v8:7611
    R=cbruni@chromium.org, jkummerow@chromium.org

    Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629
    Reviewed-on: https://chromium-review.googlesource.com/c/1318096
    Commit-Queue: Caitlin Potter <caitp@igalia.com>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57304}

PR-URL: #25101
Refs: v8/v8@bf84766
Fixes: #25089
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
b5784fe
danbev added a commit that referenced this pull request Dec 21, 2018
@BridgeAR @danbev
deps: V8: backport 3e010af 
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers only if !FLAG_unbox_double_fields

    Change the macros added in bf84766a2cd3e09070adcd6228a3a487c8dc4bbd to
    only do the hard work if FLAG_unbox_double_fields is unset (otherwise,
    they will attempt to dereference raw float64s, which is bad!)

    Also adds a write barrier in CopyPropertyArrayValues for each store if
    it's possible that a MutableHeapNumber is cloned.

    BUG=chromium:901301, chromium:902965, chromium:903070, v8:7611
    R=cbruni@chromium.org, jkummerow@chromium.org, ishell@chromium.org

    Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb
    Reviewed-on: https://chromium-review.googlesource.com/c/1323911
    Commit-Queue: Caitlin Potter <caitp@igalia.com>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57368}

PR-URL: #25101
Refs: v8/v8@3e010af
Fixes: #25089
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Loading status checks…
4884ca6
@MylesBorins

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 25, 2018

Should this be backported to v11.x?

@MylesBorins MylesBorins added the backport-requested-v11.x label Dec 25, 2018

@targos targos added this to Backport requested in v11.x Dec 28, 2018

@targos targos added backported-to-v11.x and removed author ready build backport-requested-v11.x labels Jan 1, 2019

@targos

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 1, 2019

cherry-picked to v11.x-staging without the V8 update
targos added a commit that referenced this pull request Jan 1, 2019
@BridgeAR @targos
deps: V8: backport bf84766 
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers instead of referencing them

    Adds a helper macro "CloneIfMutablePrimitive", which tests if the
    operand is a MutableHeapNumber, and if so, clones it, otherwise
    returning the original value.

    Also modifies the signature of "CopyPropertyArrayValues" to take a
    "DestroySource" enum, indicating whether or not the resulting object is
    supplanting the source object or not, and removes all default
    parameters from that macro (which were not used anyways).

    This corrects the issue reported in chromium:901301, where
    StaNamedOwnProperty was replacing the value of a MutableHeapNumber
    referenced by both the cloned object and the source object.

    BUG=chromium:901301, v8:7611
    R=cbruni@chromium.org, jkummerow@chromium.org

    Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629
    Reviewed-on: https://chromium-review.googlesource.com/c/1318096
    Commit-Queue: Caitlin Potter <caitp@igalia.com>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57304}

PR-URL: #25101
Refs: v8/v8@bf84766
Fixes: #25089
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
201cf97
targos added a commit that referenced this pull request Jan 1, 2019
@BridgeAR @targos
deps: V8: backport 3e010af 
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers only if !FLAG_unbox_double_fields

    Change the macros added in bf84766a2cd3e09070adcd6228a3a487c8dc4bbd to
    only do the hard work if FLAG_unbox_double_fields is unset (otherwise,
    they will attempt to dereference raw float64s, which is bad!)

    Also adds a write barrier in CopyPropertyArrayValues for each store if
    it's possible that a MutableHeapNumber is cloned.

    BUG=chromium:901301, chromium:902965, chromium:903070, v8:7611
    R=cbruni@chromium.org, jkummerow@chromium.org, ishell@chromium.org

    Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb
    Reviewed-on: https://chromium-review.googlesource.com/c/1323911
    Commit-Queue: Caitlin Potter <caitp@igalia.com>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57368}

PR-URL: #25101
Refs: v8/v8@3e010af
Fixes: #25089
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
f2abe7b

@targos targos moved this from Backport requested to Backported in v11.x Jan 1, 2019

refack added a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
deps: patch V8 to 7.1.302.33 
PR-URL: nodejs#25101
Refs: v8/v8@7.1.302.28...7.1.302.33
Fixes: nodejs#25089
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
64a0cd5
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
deps: V8: backport bf84766 
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers instead of referencing them

    Adds a helper macro "CloneIfMutablePrimitive", which tests if the
    operand is a MutableHeapNumber, and if so, clones it, otherwise
    returning the original value.

    Also modifies the signature of "CopyPropertyArrayValues" to take a
    "DestroySource" enum, indicating whether or not the resulting object is
    supplanting the source object or not, and removes all default
    parameters from that macro (which were not used anyways).

    This corrects the issue reported in chromium:901301, where
    StaNamedOwnProperty was replacing the value of a MutableHeapNumber
    referenced by both the cloned object and the source object.

    BUG=chromium:901301, v8:7611
    R=cbruni@chromium.org, jkummerow@chromium.org

    Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629
    Reviewed-on: https://chromium-review.googlesource.com/c/1318096
    Commit-Queue: Caitlin Potter <caitp@igalia.com>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57304}

PR-URL: nodejs#25101
Refs: v8/v8@bf84766
Fixes: nodejs#25089
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
8e2ba9e
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
deps: V8: backport 3e010af 
Original commit message:

    [CloneObjectIC] clone MutableHeapNumbers only if !FLAG_unbox_double_fields

    Change the macros added in bf84766a2cd3e09070adcd6228a3a487c8dc4bbd to
    only do the hard work if FLAG_unbox_double_fields is unset (otherwise,
    they will attempt to dereference raw float64s, which is bad!)

    Also adds a write barrier in CopyPropertyArrayValues for each store if
    it's possible that a MutableHeapNumber is cloned.

    BUG=chromium:901301, chromium:902965, chromium:903070, v8:7611
    R=cbruni@chromium.org, jkummerow@chromium.org, ishell@chromium.org

    Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb
    Reviewed-on: https://chromium-review.googlesource.com/c/1323911
    Commit-Queue: Caitlin Potter <caitp@igalia.com>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57368}

PR-URL: nodejs#25101
Refs: v8/v8@3e010af
Fixes: nodejs#25089
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
25a978e
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
Merged
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Merged
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed

@BethGriggs BethGriggs added the dont-land-on-v10.x label Jul 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofrobots ofrobots

@jasnell jasnell

@hashseed hashseed

@richardlau richardlau

@devsnek devsnek

Assignees
No one assigned
Labels
V8 Engine dont-land-on-v10.x
Projects
  
Backported in v11.x
Milestone
No milestone
You can’t perform that action at this time.