-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
module: print amount of load time of a module #52213
Conversation
Review requested:
|
Could this perhaps get its own label in |
82249f3
to
838237a
Compare
@GeoffreyBooth I changed to I didn't found any good alternative for this name, so I'm open to change this to another one if needed. |
Conceptually this feels more like something for trace_events than NODE_DEBUG? |
b80bab3
to
ac6e394
Compare
@richardlau Maybe I'm not familiar with trace_events but from what I see in the source code, I think it won't be as useful as having it as The benefits of emitting these events through |
|
Actually |
@joyeecheung I changed the implementation to match what we discussed, I liked this new way, it will print in the I still have some tests to fix but in general the implementation is good enough to get some reviews. |
I fixed the tests and I also changed a little bit the visualization for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the PR to include only the changes this PR is about? I can see some linting and type changes.
Commit Queue failed- Loading data for nodejs/node/pull/52213 ✔ Done loading data for nodejs/node/pull/52213 ----------------------------------- PR info ------------------------------------ Title module: print amount of load time of a module (#52213) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch H4ad:trace-load-module -> nodejs:main Labels semver-minor, author ready, needs-ci, review wanted, commit-queue-squash Commits 29 - module: print amount of load time of a cjs module - try support both node_debug and trace by introducing a new module - add trace label to better visualization - fix lint issue with max line - lazy load trace_timer - fix lint issues introduce when lazy load - cleanup and added more tests - remove unecessary changes - remove unecessary changes - resolve rebase conflicts - fixing nits - resolve conflicts after rebase - fix lint issues - handle unexpected errors during _load - add wrap module load function - removed unused spaces - fixing tests on last modification - escape backslash when is label - fix references for _load - add test to check against un-escaped backslash - fix issues with test & missing fn - making eslint happy with trailing space - minor changes - fix missing changing the name of the module - try get more info from ci - try get more info from ci & make lint happy - fix import of common and spawnSync - do not cache process.execPath on import - thanks richard, you are amazing Committers 1 - Vinícius Lourenço PR-URL: https://github.com/nodejs/node/pull/52213 Reviewed-By: Joyee Cheung Reviewed-By: James M Snell Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52213 Reviewed-By: Joyee Cheung Reviewed-By: James M Snell Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - module: print amount of load time of a cjs module ⚠ - try support both node_debug and trace by introducing a new module ⚠ - add trace label to better visualization ⚠ - fix lint issue with max line ⚠ - lazy load trace_timer ⚠ - fix lint issues introduce when lazy load ⚠ - cleanup and added more tests ⚠ - remove unecessary changes ⚠ - remove unecessary changes ⚠ - resolve rebase conflicts ⚠ - fixing nits ⚠ - resolve conflicts after rebase ⚠ - fix lint issues ⚠ - handle unexpected errors during _load ⚠ - add wrap module load function ⚠ - removed unused spaces ⚠ - fixing tests on last modification ⚠ - escape backslash when is label ⚠ - fix references for _load ⚠ - add test to check against un-escaped backslash ⚠ - fix issues with test & missing fn ⚠ - making eslint happy with trailing space ⚠ - minor changes ⚠ - fix missing changing the name of the module ⚠ - try get more info from ci ⚠ - try get more info from ci & make lint happy ⚠ - fix import of common and spawnSync ⚠ - do not cache process.execPath on import ⚠ - thanks richard, you are amazing ℹ This PR was created on Tue, 26 Mar 2024 01:41:52 GMT ✔ Approvals: 3 ✔ - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/52213#pullrequestreview-1997745510 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52213#pullrequestreview-2026657587 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52213#pullrequestreview-2035376215 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-05-29T21:59:32Z: https://ci.nodejs.org/job/node-test-pull-request/59531/ - Querying data for job/node-test-pull-request/59531/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/9294909258 |
@joyeecheung We are closer, just one (re)approval :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! LGTM
Landed in f883865 |
PR-URL: #52213 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: TODO
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: #53379
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) #53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: #53379
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) #53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: #53379
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) #53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: #53379
PR-URL: nodejs#52213 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) nodejs#53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) nodejs#53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) nodejs#53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) nodejs#52762 * add StefanStojanovic to collaborators (StefanStojanovic) nodejs#53118 * add Marco Ippolito to TSC (Rafael Gonzaga) nodejs#53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) nodejs#53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) nodejs#51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) nodejs#52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) nodejs#52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) nodejs#53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) nodejs#52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) nodejs#53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) nodejs#53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) nodejs#52848 PR-URL: nodejs#53379
PR-URL: nodejs#52213 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) nodejs#53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) nodejs#53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) nodejs#53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) nodejs#52762 * add StefanStojanovic to collaborators (StefanStojanovic) nodejs#53118 * add Marco Ippolito to TSC (Rafael Gonzaga) nodejs#53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) nodejs#53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) nodejs#51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) nodejs#52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) nodejs#52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) nodejs#53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) nodejs#52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) nodejs#53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) nodejs#53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) nodejs#52848 PR-URL: nodejs#53379
@H4ad Is it possible to backport this to node 20? |
I don't know, usually we need someone from the release team to select this PR first to see if there is any conflict, only if we have conflicts we need a manual backport. |
I was investigating why
npm
is slow and I started to figure out that was caused by it loading a lot of modules/js files, but I didn't know what are those modules or even how much time it took to load them.So, this implementation came to my mind, and I used with the following commands:
The log looks like this:
If we try to clean a little bit:
The output was:
In this way, is very clear what is taking a lot of time to load
npm
, and I actually already opened a PR to fix one of the issues that appeared in this log (npm/cli#7314)@joyeecheung suggest to include also the support for
trace_events
, so I created thedebugWithTimer
to introduce support for both ways extract information, via log or via trace:This will generate a
trace-events.log
file that can be imported onchrome://tracing
or Perfetto UI to generate the following graph:To be easier to read, I introduce two labels on the
debugWithTimer
, the first one will be for logging viaNODE_DEBUG
and the second one will be used at thetrace
./cc @nodejs/performance