src: keep track of env properly in node_perf.cc by addaleax · Pull Request #15391 · nodejs/node · GitHub

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

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 <ulan@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#47601}

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`. TBR=hpayer@chromium.org 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 <yangguo@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#47923}

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

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

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

@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 ?

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...

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.

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

Awesome... working on landing this now!

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 <ulan@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/master@{#47601} PR-URL: #15391 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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`. TBR=hpayer@chromium.org 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 <yangguo@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#47923} PR-URL: #15391 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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 <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Landed in 01a1812, 8403d6b, and dcb24e3

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 <ulan@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/master@{#47601} PR-URL: nodejs/node#15391 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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`. TBR=hpayer@chromium.org 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 <yangguo@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#47923} PR-URL: nodejs/node#15391 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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 <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>