src: keep track of env properly in node_perf.cc#15391
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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}
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 added performance 
nodejs-github-bot added the lib / src 
jasnell approved these changes Sep 13, 2017
Member
jasnell left a comment
jasnell left a comment There was a problem hiding this comment.
Rubber stamp LGTM on the cherry-pick commits, and regular LGTM on the node_perf changes. Thank you!
targos approved these changes Sep 13, 2017
MemberAuthor
CI: https://ci.nodejs.org/job/node-test-commit/12340/ (edit: 👍 ) |
addaleaxforce-pushed the node-perf-env branch from 5c6463c to 0fba667Compare
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 :-) |
MemberAuthor
addaleax commented Sep 15, 2017
@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 ? |
Member
targos commented Sep 15, 2017
We usually don't bump the patch number if the version is still supported. |
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.
addaleaxforce-pushed the node-perf-env branch from 0fba667 to 70ddf36CompareMemberAuthor
addaleax commented Sep 15, 2017
@targos Thanks for clarifying! I’ve removed the version bump :) |
Member
jasnell commented Sep 15, 2017
Awesome... working on landing this now! |
jasnell pushed a commit that referenced this pull request Sep 15, 2017
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
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
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]>
Member
addaleax added a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
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
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
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 added a commit to addaleax/node that referenced this pull request Oct 15, 2017
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
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
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
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
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
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
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
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
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
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First commit is to avoid conflicts with the second, the second commit is needed for the “real” one.
Currently, measuring GC timing using
node_perfis 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), orvcbuild test(Windows) passesAffected core subsystem(s)
deps/v8, src/node_perf
@nodejs/v8 @jasnell