deps: fix Array.prototype.forEach on v8 6.8#22899
Closed
+9 −8
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 
Member
Trott commented Sep 18, 2018
@nodejs/v8-update |
Member
BridgeAR commented Sep 19, 2018
The title and description seems different than what we normally do. An example of such a commit is ab15016 Right now I am not sure what commit this will include from V8. |
Member
targos commented Sep 19, 2018
The pull request should target the master branch or, if the slowdown does not affect V8 6.9, |
Member
hashseed commented Sep 21, 2018
@ripsawridge is currently on vacation. |
Member
targos commented Sep 21, 2018
We are about to merge V8 7.0 into |
addaleaxforce-pushed the patchy branch 2 times, most recently from 2caf1c0 to d885262Compare
This applies a variant of v8/v8@e1163c14f7e4fef2c549 to V8 6.8. Original commit message: [Builtins] Array.prototype.forEach perf regression on dictionaries An unnecessary call to ToString() on the array index caused trips to the runtime. The fix also includes performance micro-benchmarks so we'll have a harder time regressing this case in future. [email protected] Bug: v8:8112 Change-Id: I781e8b1bbe2eb56db961cf33b0dca8523868b83d Reviewed-on: https://chromium-review.googlesource.com/1213207 Commit-Queue: Michael Stanton <[email protected]> Reviewed-by: Michael Stanton <[email protected]> Reviewed-by: Tobias Tebbi <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#55733} Refs: v8/v8@e1163c1Fixes: nodejs#22859
Member
addaleax commented Sep 22, 2018
I’ve rebased against v10.x-staging, and update the commit message to point to v8/v8@e1163c1 (this is not a clean cherry-pick, though). CI: https://ci.nodejs.org/job/node-test-pull-request/17368/ |
addaleax added author ready 
addaleax approved these changes Sep 25, 2018
Member
addaleax left a comment
addaleax left a comment There was a problem hiding this comment.
LGTM but this needs a rebase again
Member
targos commented Sep 25, 2018
The embedder string can be fixed while landing |
Member
jasnell commented Sep 25, 2018
Needs a quick rebase |
jasnell approved these changes Sep 25, 2018
targos pushed a commit that referenced this pull request Sep 25, 2018
This applies a variant of v8/v8@e1163c14f7e4fef2c549 to V8 6.8. Original commit message: [Builtins] Array.prototype.forEach perf regression on dictionaries An unnecessary call to ToString() on the array index caused trips to the runtime. The fix also includes performance micro-benchmarks so we'll have a harder time regressing this case in future. [email protected] Bug: v8:8112 Change-Id: I781e8b1bbe2eb56db961cf33b0dca8523868b83d Reviewed-on: https://chromium-review.googlesource.com/1213207 Commit-Queue: Michael Stanton <[email protected]> Reviewed-by: Michael Stanton <[email protected]> Reviewed-by: Tobias Tebbi <[email protected]> Cr-Commit-Position: refs/heads/master@{#55733} Refs: v8/v8@e1163c1Fixes: #22859 PR-URL: #22899 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Member
targos commented Sep 25, 2018
Landed in 5d70652 |
This was referenced Oct 10, 2018
This was referenced Oct 11, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
8 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.
Refs: #22859
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes