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

deps: V8: cherry-pick cca9ae3c9a #27729

Closed
wants to merge 1 commit into from
Closed

deps: V8: cherry-pick cca9ae3c9a #27729

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

Original commit message:

Remove recursion from NeedsCheckHeapObject.

We use the predicate NeedsCheckHeapObject in the compiler frontend to
determine whether we can skip introducing CheckHeapObject nodes. But
this predicate would also walk up the graph in case of Phis, which can
result in really long compilation times (on the main thread). In the
report in https://github.com/nodejs/node/issues/27667, the compiler
frontend alone took around 4-5mins of main thread time for a single
function. With this patch the time goes down to 4-5ms.

Bug: v8:9250
Refs: nodejs/node#27667
Change-Id: I231eb780ff04f949fa1669714f9af6ebfbcade05
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1612897
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61503}

Fixes: #27667

Per the issue, this applies to v10.x-v12.x. v8.x is unaffected.

@bmeurer @bnoordhuis
deps: V8: cherry-pick cca9ae3c9a
5176a79 
Original commit message:

    Remove recursion from NeedsCheckHeapObject.

    We use the predicate NeedsCheckHeapObject in the compiler frontend to
    determine whether we can skip introducing CheckHeapObject nodes. But
    this predicate would also walk up the graph in case of Phis, which can
    result in really long compilation times (on the main thread). In the
    report in nodejs#27667, the compiler
    frontend alone took around 4-5mins of main thread time for a single
    function. With this patch the time goes down to 4-5ms.

    Bug: v8:9250
    Refs: nodejs#27667
    Change-Id: I231eb780ff04f949fa1669714f9af6ebfbcade05
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1612897
    Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61503}

Fixes: nodejs#27667
@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3538

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label May 16, 2019
cjihrig
cjihrig approved these changes May 16, 2019
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and thank you for keeping the original commit author in tact.

mhdawson
mhdawson approved these changes May 17, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 17, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23168/

@gengjiawen
Copy link
Member

In commit msg, V8 need to be lowercase

+npx -q core-validate-commit --no-validate-metadata 5176a795511b04d6ca7340120211f7a366e829e3
  ✖  5176a795511b04d6ca7340120211f7a366e829e3
     ✔  21:7     Valid fixes URL.                          fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✖  0:7      First word after subsystem(s) in title should be lowercase. title-format
     ✔  0:0      Title is <= 50 columns.

@bnoordhuis
Copy link
Member Author

@gengjiawen I followed the, for lack of a better word, "house style" for deps/v8:

$ git log --oneline deps/v8 | grep 'deps: V8' | head -10
377939eef8 deps: V8: cherry-pick 5d0cf6b
5b8434eebc deps: V8: cherry-pick 0188634
8cc181c8ee deps: V8: cherry-pick c8785d1
2ea9de2e85 deps: V8: cherry-pick f4b860d
0da7e99f98 deps: V8: un-cherry-pick bd019bd
b1015e0de8 deps: V8: cherry-pick 6 commits
8181811d73 deps: V8: cherry-pick d82c9af
1f03fb4d49 deps: V8: cherry-pick e5f01ba
e6af2207a9 deps: V8: cherry-pick d5f08e4
963061bc02 deps: V8: cherry-pick 6b09d21

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23205/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23216/

@addaleax
Copy link
Member

Landed in 6f7e8b4

@addaleax addaleax closed this May 19, 2019
addaleax pushed a commit that referenced this pull request May 19, 2019
@bmeurer @addaleax
deps: V8: cherry-pick cca9ae3c9a
6f7e8b4 
Original commit message:

    Remove recursion from NeedsCheckHeapObject.

    We use the predicate NeedsCheckHeapObject in the compiler frontend to
    determine whether we can skip introducing CheckHeapObject nodes. But
    this predicate would also walk up the graph in case of Phis, which can
    result in really long compilation times (on the main thread). In the
    report in #27667, the compiler
    frontend alone took around 4-5mins of main thread time for a single
    function. With this patch the time goes down to 4-5ms.

    Bug: v8:9250
    Refs: #27667
    Change-Id: I231eb780ff04f949fa1669714f9af6ebfbcade05
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1612897
    Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61503}

Fixes: #27667

PR-URL: #27729
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis bnoordhuis deleted the fix27667 branch May 20, 2019 11:06
targos pushed a commit that referenced this pull request May 20, 2019
@bmeurer @targos
deps: V8: cherry-pick cca9ae3c9a
83f2430 
Original commit message:

    Remove recursion from NeedsCheckHeapObject.

    We use the predicate NeedsCheckHeapObject in the compiler frontend to
    determine whether we can skip introducing CheckHeapObject nodes. But
    this predicate would also walk up the graph in case of Phis, which can
    result in really long compilation times (on the main thread). In the
    report in #27667, the compiler
    frontend alone took around 4-5mins of main thread time for a single
    function. With this patch the time goes down to 4-5ms.

    Bug: v8:9250
    Refs: #27667
    Change-Id: I231eb780ff04f949fa1669714f9af6ebfbcade05
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1612897
    Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61503}

Fixes: #27667

PR-URL: #27729
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
refack pushed a commit to targos/node that referenced this pull request May 22, 2019
@bmeurer @refack
deps: V8: cherry-pick cca9ae3c9a
451c238 
Original commit message:

    Remove recursion from NeedsCheckHeapObject.

    We use the predicate NeedsCheckHeapObject in the compiler frontend to
    determine whether we can skip introducing CheckHeapObject nodes. But
    this predicate would also walk up the graph in case of Phis, which can
    result in really long compilation times (on the main thread). In the
    report in nodejs#27667, the compiler
    frontend alone took around 4-5mins of main thread time for a single
    function. With this patch the time goes down to 4-5ms.

    Bug: v8:9250
    Refs: nodejs#27667
    Change-Id: I231eb780ff04f949fa1669714f9af6ebfbcade05
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1612897
    Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61503}

Fixes: nodejs#27667

PR-URL: nodejs#27729
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this pull request May 23, 2019
@bmeurer @cjihrig
deps: V8: cherry-pick cca9ae3c9a
cc7690f 
Original commit message:

    Remove recursion from NeedsCheckHeapObject.

    We use the predicate NeedsCheckHeapObject in the compiler frontend to
    determine whether we can skip introducing CheckHeapObject nodes. But
    this predicate would also walk up the graph in case of Phis, which can
    result in really long compilation times (on the main thread). In the
    report in nodejs#27667, the compiler
    frontend alone took around 4-5mins of main thread time for a single
    function. With this patch the time goes down to 4-5ms.

    Bug: v8:9250
    Refs: nodejs#27667
    Change-Id: I231eb780ff04f949fa1669714f9af6ebfbcade05
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1612897
    Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61503}

Fixes: nodejs#27667

PR-URL: nodejs#27729
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request May 26, 2019
@bmeurer @targos
deps: V8: cherry-pick cca9ae3c9a
59ec46d 
Original commit message:

    Remove recursion from NeedsCheckHeapObject.

    We use the predicate NeedsCheckHeapObject in the compiler frontend to
    determine whether we can skip introducing CheckHeapObject nodes. But
    this predicate would also walk up the graph in case of Phis, which can
    result in really long compilation times (on the main thread). In the
    report in nodejs#27667, the compiler
    frontend alone took around 4-5mins of main thread time for a single
    function. With this patch the time goes down to 4-5ms.

    Bug: v8:9250
    Refs: nodejs#27667
    Change-Id: I231eb780ff04f949fa1669714f9af6ebfbcade05
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1612897
    Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61503}

Fixes: nodejs#27667

PR-URL: nodejs#27729
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to targos/node that referenced this pull request Jun 1, 2019
@bmeurer @refack
deps: V8: cherry-pick cca9ae3c9a
cdd4439 
Original commit message:

    Remove recursion from NeedsCheckHeapObject.

    We use the predicate NeedsCheckHeapObject in the compiler frontend to
    determine whether we can skip introducing CheckHeapObject nodes. But
    this predicate would also walk up the graph in case of Phis, which can
    result in really long compilation times (on the main thread). In the
    report in nodejs#27667, the compiler
    frontend alone took around 4-5mins of main thread time for a single
    function. With this patch the time goes down to 4-5ms.

    Bug: v8:9250
    Refs: nodejs#27667
    Change-Id: I231eb780ff04f949fa1669714f9af6ebfbcade05
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1612897
    Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61503}

Fixes: nodejs#27667

PR-URL: nodejs#27729
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Jun 4, 2019
@bmeurer @targos
deps: V8: cherry-pick cca9ae3c9a
8b8fe87 
Original commit message:

    Remove recursion from NeedsCheckHeapObject.

    We use the predicate NeedsCheckHeapObject in the compiler frontend to
    determine whether we can skip introducing CheckHeapObject nodes. But
    this predicate would also walk up the graph in case of Phis, which can
    result in really long compilation times (on the main thread). In the
    report in nodejs#27667, the compiler
    frontend alone took around 4-5mins of main thread time for a single
    function. With this patch the time goes down to 4-5ms.

    Bug: v8:9250
    Refs: nodejs#27667
    Change-Id: I231eb780ff04f949fa1669714f9af6ebfbcade05
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1612897
    Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
    Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61503}

Fixes: nodejs#27667

Backport-PR-URL: nodejs#28005
PR-URL: nodejs#27729
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Performance issue in NodeJS >= 10.0.0