-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
perf_hooks: make nodeTiming a first-class object #35977
Conversation
Render all properties of nodeTiming enumerable so JSON.stringify and Object.keys can access them Fixes: nodejs#35936
Improve readability as per @aduh95 suggestion Refs: nodejs#35977
Transform the static values to plain properties (@addaleax review) Refs: nodejs#35977
Actually, I’d make all of these configurable … it can be nice to be able to override them for testing purposes if that’s ever necessary |
Render all properties configurable (@addaleax review) nodejs#35977
lib/perf_hooks.js
Outdated
|
||
idleTime: { | ||
...PerformanceNodeTimingProps, |
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.
Non-blocking: I would just include the properties directly here (rather than use the repeated `...PerformanceNodeTimingProps) even if they're going to be duplicated just to save an unnecessary performance cost.
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.
Hmm, do you think V8 keeps a reference to the initializing object when using the spread operator?
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.
Yeah, this is ECMA specified in fact
Render all properties of nodeTiming enumerable so JSON.stringify and Object.keys can access them Refs: nodejs#35977
Landed in feff385 |
Render all properties of nodeTiming enumerable so JSON.stringify and Object.keys can access them Fixes: #35936 PR-URL: #35977 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Does this obviate the need for |
No, you still need it to get a pretty console output, you see |
Render all properties of nodeTiming enumerable so JSON.stringify and Object.keys can access them Fixes: #35936 PR-URL: #35977 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Render all properties of nodeTiming enumerable so JSON.stringify and Object.keys can access them Fixes: #35936 PR-URL: #35977 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Render all properties of nodeTiming enumerable
so JSON.stringify and Object.keys can access them
Fixes: #35936
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes