Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Performance regression in Array.forEach on V8 6.8 #22859

Closed
hashseed opened this issue Sep 14, 2018 · 4 comments
Closed

Performance regression in Array.forEach on V8 6.8 #22859

hashseed opened this issue Sep 14, 2018 · 4 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@hashseed
Copy link
Member

See upstream bug.

This is already fixed upstream and should be merged to master and the Node 10 branch.
@hashseed hashseed added the v8 engine Issues and PRs related to the V8 dependency. label Sep 14, 2018
@targos
Copy link
Member

targos commented Sep 14, 2018

array-foreach.tq doesn't exist in V8 6.8. Should we first backport the change that added it?

@ripsawridge
Copy link

Hi Targos, I'll make a patch...we did a fair bit of refactoring.

@ripsawridge ripsawridge mentioned this issue Sep 17, 2018
Closed
2 tasks
@ripsawridge
Copy link

Okay, pull request created, thanks for the help, Yang!

addaleax pushed a commit to ripsawridge/node that referenced this issue Sep 22, 2018
@ripsawridge @addaleax
deps: fix Array.prototype.forEach on v8 6.8
1e8be82 
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@e1163c1
Fixes: nodejs#22859
targos pushed a commit that referenced this issue Sep 25, 2018
@ripsawridge @targos
deps: fix Array.prototype.forEach on v8 6.8
5d70652 
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@e1163c1
Fixes: #22859
PR-URL: #22899
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@apapirovski
Copy link
Member

Fix got merged already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@apapirovski @hashseed @targos @ripsawridge