-
Notifications
You must be signed in to change notification settings - Fork 30k
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: reduce overhead of new performance_entries #49803
Conversation
@jasnell Do you have any reason to add I saw the |
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.
lgtm
impressive |
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.
oh wow we were passing a new constructor each time?
This is a subtly breaking change (if someone relied on them having different constructors) but I think we never made that commitment so this is fine to me as minor/patch
gj
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.
🚀
9691ead
to
36627f2
Compare
Sorry for the force-push but I read another PR of @rluvaton, this new way is a faster (793%->872%) and cleaner way to do the same thing. |
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.
LGTM. Out of curiosity though, with the skip throw thing in the constructor, can the perf be improved even further by putting the init behaviour directly in the constructor after the skipped throw?
@Qard, I need to try, but it may not change significantly as it will be essentially the same operations. Also, I don't know if there is any spec compliance with the constructor I avoid changing the arguments of the Performance Entry, without |
Is there something I can do about the failing checks? |
@anonrig has to start them over and over again, till they are green :P |
Landed in a6ad048 |
PR-URL: #49803 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
PR-URL: #49803 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
PR-URL: nodejs#49803 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
PR-URL: nodejs#49803 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
Continuing the work started on nodejs/performance#109
Removed two references for
ReflectConstruct
, the improvements forcreatePerformanceNodeEntry
can be seen using thetimerfied
function:The
createPerformanceEntry
is not used, should I remove it?/cc @nodejs/performance