deps: V8: fix spread operator#25101
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
nodejs-github-bot added build 
richardlau approved these changes Dec 18, 2018
devsnek approved these changes Dec 18, 2018
MemberAuthor
BridgeAR commented Dec 18, 2018
BridgeAR added the author ready 
Member
targos commented Dec 18, 2018
It seems this depends on some other change |
MemberAuthor
BridgeAR 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 added wip 
jasnell approved these changes Dec 18, 2018
Member
jasnell commented Dec 18, 2018
Idea of landing this is +1 ... obviously need to figure out what other change may be needed :-) |
Member
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 |
MemberAuthor
BridgeAR commented Dec 19, 2018
@Trott thanks for catching that! |
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 [email protected], [email protected] Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629 Reviewed-on: https://chromium-review.googlesource.com/c/1318096 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#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 [email protected], [email protected], [email protected] Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb Reviewed-on: https://chromium-review.googlesource.com/c/1323911 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57368} Refs: v8/v8@3e010af
BridgeARforce-pushed the fix-spread-operator branch from 4d1ea2b to 6096720CompareMemberAuthor
BridgeAR 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/ |
Member
targos commented Dec 20, 2018
/cc @nodejs/v8-update |
ofrobots approved these changes Dec 20, 2018
hashseed approved these changes Dec 20, 2018
BridgeAR added the author ready 
Contributor
danbev commented Dec 21, 2018
danbev pushed a commit that referenced this pull request Dec 21, 2018
PR-URL: #25101 Refs: v8/v8@7.1.302.28...7.1.302.33Fixes: #25089 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Yang Guo <[email protected]>
danbev pushed a commit that referenced this pull request Dec 21, 2018
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[email protected], [email protected] Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629 Reviewed-on: https://chromium-review.googlesource.com/c/1318096 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#57304} PR-URL: #25101 Refs: v8/v8@bf84766Fixes: #25089 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Yang Guo <[email protected]>
danbev pushed a commit that referenced this pull request Dec 21, 2018
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[email protected], [email protected], [email protected] Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb Reviewed-on: https://chromium-review.googlesource.com/c/1323911 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/master@{#57368} PR-URL: #25101 Refs: v8/v8@3e010afFixes: #25089 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Yang Guo <[email protected]>
Contributor
MylesBorins commented Dec 25, 2018
Should this be backported to v11.x? |
targos added backported-to-v11.x and removed author ready Member
targos commented Jan 1, 2019
cherry-picked to v11.x-staging without the V8 update |
targos pushed a commit that referenced this pull request Jan 1, 2019
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[email protected], [email protected] Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629 Reviewed-on: https://chromium-review.googlesource.com/c/1318096 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#57304} PR-URL: #25101 Refs: v8/v8@bf84766Fixes: #25089 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Yang Guo <[email protected]>
targos pushed a commit that referenced this pull request Jan 1, 2019
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[email protected], [email protected], [email protected] Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb Reviewed-on: https://chromium-review.googlesource.com/c/1323911 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/master@{#57368} PR-URL: #25101 Refs: v8/v8@3e010afFixes: #25089 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Yang Guo <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#25101 Refs: v8/v8@7.1.302.28...7.1.302.33Fixes: nodejs#25089 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Yang Guo <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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 [email protected], [email protected] Change-Id: I43df1ddc84dfa4840e680b6affeba452ce0b6629 Reviewed-on: https://chromium-review.googlesource.com/c/1318096 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57304} PR-URL: nodejs#25101 Refs: v8/v8@bf84766Fixes: nodejs#25089 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Yang Guo <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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 [email protected], [email protected], [email protected] Change-Id: I224d3c4e7b0a887684bff68985b4d97021ba4cfb Reviewed-on: https://chromium-review.googlesource.com/c/1323911 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57368} PR-URL: nodejs#25101 Refs: v8/v8@3e010afFixes: nodejs#25089 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Yang Guo <[email protected]>
Merged
Merged
This was referenced Apr 23, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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), orvcbuild test(Windows) passes