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.

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

vm,trace_events: add node.vm trace events category #20728

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 14, 2018

Add basic trace events for node_contextify. These generally
align very well with V8.ScriptCompile and V8.ScriptExecute
trace events and provide good additional context. For instance,
using the node.vm trace category at startup allows us to see
exactly how long compilation and init of each of our core
modules adds to the startup time.

image

/cc @nodejs/diagnostics @nodejs/trace-events @nodejs/vm

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels May 14, 2018
@jasnell jasnell requested a review from addaleax May 14, 2018 21:45
@jasnell
Copy link
Member Author

jasnell commented May 14, 2018

@devsnek
Copy link
Member

devsnek commented May 14, 2018

could you possibly rename it to node.vm.script or the events to ScriptRunInContext or something similar?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Can’t really review the tracing semantics, but code-wise this seems fine to me

@jasnell
Copy link
Member Author

jasnell commented May 14, 2018

@devsnek ... done. Just curious why tho :-)

@devsnek
Copy link
Member

devsnek commented May 14, 2018

@jasnell because we have vm.module as well with its own parsing and running and such

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 15, 2018
@jasnell
Copy link
Member Author

jasnell commented May 15, 2018

CI is good. One unrelated flaky failure.

@jasnell jasnell mentioned this pull request May 16, 2018
2 tasks
jasnell added 3 commits May 15, 2018 22:57
Add basic trace events for node_contextify. These generally
align very well with V8.ScriptCompile and V8.ScriptExecute
trace events and provide good additional context. For instance,
using the node.vm trace category at startup allows us to see
exactly how long compilation and init of each of our core
modules adds to the startup time.
@jasnell jasnell force-pushed the contextify-trace-events branch from 42aec9f to 2a9679a Compare May 16, 2018 05:58
jasnell added a commit that referenced this pull request May 16, 2018
Add basic trace events for node_contextify. These generally
align very well with V8.ScriptCompile and V8.ScriptExecute
trace events and provide good additional context. For instance,
using the node.vm.script trace category at startup allows us to
see exactly how long compilation and init of each of our core
modules adds to the startup time.

PR-URL: #20728
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented May 16, 2018

Landed in 8630eea

@jasnell jasnell closed this May 16, 2018
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Add basic trace events for node_contextify. These generally
align very well with V8.ScriptCompile and V8.ScriptExecute
trace events and provide good additional context. For instance,
using the node.vm.script trace category at startup allows us to
see exactly how long compilation and init of each of our core
modules adds to the startup time.

PR-URL: #20728
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax addaleax mentioned this pull request May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants