deps: V8: fix spread operator #25101

BridgeAR · 2018-12-18T01:05:07Z

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 · 2018-12-18T01:05:10Z

@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

BridgeAR · 2018-12-18T10:32:01Z

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

targos · 2018-12-18T10:37:43Z

It seems this depends on some other change

BridgeAR · 2018-12-18T10:44:55Z

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

jasnell · 2018-12-18T15:28:19Z

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

Trott · 2018-12-18T19:23:12Z

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 · 2018-12-19T10:44:12Z

@Trott thanks for catching that!

Refs: v8/v8@7.1.302.28...7.1.302.33

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

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

BridgeAR · 2018-12-20T15:18:25Z

@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/

targos · 2018-12-20T15:35:01Z

/cc @nodejs/v8-update

danbev · 2018-12-21T06:40:37Z

Landed in a981214, b5784fe, and 4884ca6

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>

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>

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>

MylesBorins · 2018-12-25T06:50:23Z

Should this be backported to v11.x?

targos · 2019-01-01T13:57:26Z

cherry-picked to v11.x-staging without the V8 update

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>

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>

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>

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>

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>

nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Dec 18, 2018

richardlau approved these changes Dec 18, 2018

View reviewed changes

devsnek approved these changes Dec 18, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 18, 2018

BridgeAR added wip Issues and PRs that are still a work in progress. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 18, 2018

jasnell approved these changes Dec 18, 2018

View reviewed changes

BridgeAR added 3 commits December 20, 2018 14:34

deps: patch V8 to 7.1.302.33

c9bbf5a

Refs: v8/v8@7.1.302.28...7.1.302.33

BridgeAR force-pushed the fix-spread-operator branch from 4d1ea2b to 6096720 Compare December 20, 2018 15:00

BridgeAR removed the wip Issues and PRs that are still a work in progress. label Dec 20, 2018

ofrobots approved these changes Dec 20, 2018

View reviewed changes

hashseed approved these changes Dec 20, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 20, 2018

danbev closed this Dec 21, 2018

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

targos added backported-to-v11.x and removed 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. backport-requested-v11.x labels Jan 1, 2019

BridgeAR mentioned this pull request Jan 16, 2019

v11.7.0 proposal #25537

Merged

MylesBorins mentioned this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

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

BridgeAR deleted the fix-spread-operator branch January 20, 2020 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: V8: fix spread operator #25101

deps: V8: fix spread operator #25101

BridgeAR commented Dec 18, 2018

nodejs-github-bot commented Dec 18, 2018

BridgeAR commented Dec 18, 2018

targos commented Dec 18, 2018

BridgeAR commented Dec 18, 2018

jasnell commented Dec 18, 2018

Trott commented Dec 18, 2018 •

edited

Loading

BridgeAR commented Dec 19, 2018

BridgeAR commented Dec 20, 2018

targos commented Dec 20, 2018

danbev commented Dec 21, 2018

MylesBorins commented Dec 25, 2018

targos commented Jan 1, 2019

deps: V8: fix spread operator #25101

deps: V8: fix spread operator #25101

Conversation

BridgeAR commented Dec 18, 2018

Checklist

nodejs-github-bot commented Dec 18, 2018

BridgeAR commented Dec 18, 2018

targos commented Dec 18, 2018

BridgeAR commented Dec 18, 2018

jasnell commented Dec 18, 2018

Trott commented Dec 18, 2018 • edited Loading

BridgeAR commented Dec 19, 2018

BridgeAR commented Dec 20, 2018

targos commented Dec 20, 2018

danbev commented Dec 21, 2018

MylesBorins commented Dec 25, 2018

targos commented Jan 1, 2019

Trott commented Dec 18, 2018 •

edited

Loading