-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: fix Array.prototype.forEach on v8 6.8 #22899
Conversation
@nodejs/v8-update |
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. |
The pull request should target the master branch or, if the slowdown does not affect V8 6.9, |
@ripsawridge is currently on vacation. |
We are about to merge V8 7.0 into |
2caf1c0
to
d885262
Compare
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. TBR=tebbi@chromium.org Bug: v8:8112 Change-Id: I781e8b1bbe2eb56db961cf33b0dca8523868b83d Reviewed-on: https://chromium-review.googlesource.com/1213207 Commit-Queue: Michael Stanton <mvstanton@chromium.org> Reviewed-by: Michael Stanton <mvstanton@chromium.org> Reviewed-by: Tobias Tebbi <tebbi@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#55733} Refs: v8/v8@e1163c1 Fixes: nodejs#22859
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/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rubberstamp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but this needs a rebase again
The embedder string can be fixed while landing |
Needs a quick rebase |
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. TBR=tebbi@chromium.org Bug: v8:8112 Change-Id: I781e8b1bbe2eb56db961cf33b0dca8523868b83d Reviewed-on: https://chromium-review.googlesource.com/1213207 Commit-Queue: Michael Stanton <mvstanton@chromium.org> Reviewed-by: Michael Stanton <mvstanton@chromium.org> Reviewed-by: Tobias Tebbi <tebbi@chromium.org> Cr-Commit-Position: refs/heads/master@{#55733} Refs: v8/v8@e1163c1 Fixes: #22859 PR-URL: #22899 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in 5d70652 |
Refs: #22859
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes