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

src: keep track of env properly in node_perf.cc #15391

Closed
wants to merge 3 commits into from
Closed

src: keep track of env properly in node_perf.cc #15391

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

First commit is to avoid conflicts with the second, the second commit is needed for the “real” one.

Currently, measuring GC timing using node_perf is somewhat broken, because Isolates and Node Environments do not necessarily match 1:1; each environment adds its own hook, so possibly the hook code runs multiple times, but since it can’t reliably compute its corresponding event loop based on the Isolate, each run targets the same Environment right now.

This fixes that problem by using new overloads of the GC tracking APIs that can pass data to the callback through opaque pointers.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

deps/v8, src/node_perf

@nodejs/v8 @jasnell

@addaleax
deps: cherry-pick b6158eb6befae from V8 upstream
93886e0 
Original commit message:

    [heap] Move gc callbacks from List to std::vector

    Bug: v8:6333
    Change-Id: I4434c6cc59f886f1e37dfd315a3ad5fee28d3f63
    Reviewed-on: https://chromium-review.googlesource.com/634907
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#47601}
@addaleax
deps: cherry-pick 9b21865822243 from V8 upstream
c82d22e 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#47923}
@addaleax addaleax added performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency. labels Sep 13, 2017
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 13, 2017
jasnell
jasnell approved these changes Sep 13, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM on the cherry-pick commits, and regular LGTM on the node_perf changes. Thank you!

@addaleax
Copy link
Member Author

addaleax commented Sep 13, 2017 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-commit/12340/ (edit: 👍 )

@jasnell
Copy link
Member

jasnell commented Sep 15, 2017

Ping @addaleax ... I was going to land this but would feel better if you did so that you can squash the commits appropriately :-)

@addaleax
Copy link
Member Author

@jasnell I only have train wifi rn, but feel free to go for it. I am not actually sure about bumping the version number anymore, I asked @MylesBorins and he said I should bump but 6.1 isn’t abandoned yet, so, uh … @nodejs/v8 ?

@targos
Copy link
Member

targos commented Sep 15, 2017

We usually don't bump the patch number if the version is still supported.
I'll try to revive my CL to add an embedded version number. Last time we tried it broke chromium...

@addaleax
src: keep track of env properly in node_perf.cc
70ddf36 
Currently, measuring GC timing using `node_perf` is somewhat broken,
because Isolates and Node Environments do not necessarily match 1:1;
each environment adds its own hook, so possibly the hook code runs
multiple times, but since it can’t reliably compute its corresponding
event loop based on the Isolate, each run targets the same Environment
right now.

This fixes that problem by using new overloads of the GC tracking
APIs that can pass data to the callback through opaque pointers.
@addaleax
Copy link
Member Author

@targos Thanks for clarifying! I’ve removed the version bump :)

@jasnell
Copy link
Member

jasnell commented Sep 15, 2017

Awesome... working on landing this now!

jasnell pushed a commit that referenced this pull request Sep 15, 2017
@addaleax @jasnell
deps: cherry-pick b6158eb6befae from V8 upstream
01a1812 
Original commit message:

    [heap] Move gc callbacks from List to std::vector

    Bug: v8:6333
    Change-Id: I4434c6cc59f886f1e37dfd315a3ad5fee28d3f63
    Reviewed-on: https://chromium-review.googlesource.com/634907
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#47601}

PR-URL: #15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 15, 2017
@addaleax @jasnell
deps: cherry-pick 9b21865822243 from V8 upstream
8403d6b 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#47923}

PR-URL: #15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 15, 2017
@addaleax @jasnell
src: keep track of env properly in node_perf.cc
dcb24e3 
Currently, measuring GC timing using `node_perf` is somewhat broken,
because Isolates and Node Environments do not necessarily match 1:1;
each environment adds its own hook, so possibly the hook code runs
multiple times, but since it can’t reliably compute its corresponding
event loop based on the Isolate, each run targets the same Environment
right now.

This fixes that problem by using new overloads of the GC tracking
APIs that can pass data to the callback through opaque pointers.

PR-URL: #15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 15, 2017 •
edited
Loading

Landed in 01a1812, 8403d6b, and dcb24e3

@jasnell jasnell closed this Sep 15, 2017
@addaleax addaleax deleted the node-perf-env branch September 15, 2017 21:52
addaleax added a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
@addaleax
deps: cherry-pick b6158eb6befae from V8 upstream
c04350d 
Original commit message:

    [heap] Move gc callbacks from List to std::vector

    Bug: v8:6333
    Change-Id: I4434c6cc59f886f1e37dfd315a3ad5fee28d3f63
    Reviewed-on: https://chromium-review.googlesource.com/634907
    Reviewed-by: Ulan Degenbaev <[email protected]>
    Commit-Queue: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#47601}

PR-URL: nodejs/node#15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax added a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
@addaleax
deps: cherry-pick 9b21865822243 from V8 upstream
00f0d22 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#47923}

PR-URL: nodejs/node#15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax added a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
@addaleax
src: keep track of env properly in node_perf.cc
05c5956 
Currently, measuring GC timing using `node_perf` is somewhat broken,
because Isolates and Node Environments do not necessarily match 1:1;
each environment adds its own hook, so possibly the hook code runs
multiple times, but since it can’t reliably compute its corresponding
event loop based on the Isolate, each run targets the same Environment
right now.

This fixes that problem by using new overloads of the GC tracking
APIs that can pass data to the callback through opaque pointers.

PR-URL: nodejs/node#15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@addaleax addaleax mentioned this pull request Sep 18, 2017
Closed
addaleax added a commit to addaleax/node that referenced this pull request Oct 15, 2017
@addaleax
deps: cherry-pick 9b21865822243 from V8 upstream
8c09b68 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#47923}

PR-URL: nodejs#15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Oct 18, 2017
@addaleax @targos
deps: cherry-pick 9b21865822243 from V8 upstream
91b6349 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#47923}

PR-URL: nodejs#15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Oct 19, 2017
@addaleax @targos
deps: cherry-pick 9b21865822243 from V8 upstream
d7456ab 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#47923}

PR-URL: #15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@addaleax
deps: cherry-pick 9b21865822243 from V8 upstream
3720fdc 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#47923}

PR-URL: nodejs/node#15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@addaleax
deps: cherry-pick 9b21865822243 from V8 upstream
45a12b7 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#47923}

PR-URL: nodejs/node#15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jan 15, 2018
@addaleax @targos
deps: cherry-pick 9b21865822243 from V8 upstream
716eef4 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#47923}

PR-URL: nodejs#15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 16, 2018
@addaleax @gibfahn
deps: cherry-pick 9b21865822243 from V8 upstream
694291b 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#47923}

PR-URL: nodejs#15391
Backport-PR-URL: nodejs#16413
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jan 22, 2018
@addaleax @gibfahn
deps: cherry-pick 9b21865822243 from V8 upstream
94827f5 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#47923}

PR-URL: nodejs#15391
Backport-PR-URL: nodejs#16413
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit to targos/node that referenced this pull request Feb 7, 2018
@addaleax @MylesBorins
deps: cherry-pick 9b21865822243 from V8 upstream
3d3da4a 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#47923}

PR-URL: nodejs#15391
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
gibfahn pushed a commit that referenced this pull request Feb 18, 2018
@addaleax @gibfahn
deps: cherry-pick 9b21865822243 from V8 upstream
a7fc127 
Original commit message:

    [api] Add optional data pointer to GC callbacks

    This can be useful when there may be multiple callbacks attached by
    code that's not directly tied to a single isolate, e.g. working
    on a per-context basis.

    This also allows rephrasing the global non-isolate APIs in terms
    of this new API, rather than working around it inside `src/heap`.

    [email protected]

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I2e490ec40d1a34ea812f25f41ef9741d2116d965
    Reviewed-on: https://chromium-review.googlesource.com/647548
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Adam Klein <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#47923}

PR-URL: #15391
Backport-PR-URL: #16413
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. performance Issues and PRs related to the performance of Node.js. 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.
4 participants
@addaleax @jasnell @targos @nodejs-github-bot