-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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: fix timing #18993
perf_hooks: fix timing #18993
Conversation
lib/perf_hooks.js
Outdated
@@ -145,6 +146,13 @@ function now() { | |||
return hr[0] * 1000 + hr[1] / 1e6; | |||
} | |||
|
|||
function getMilestoneTimestamp(milestoneIdx) { | |||
const us = milestones[milestoneIdx]; |
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.
us
? I think you want ns
for nanoseconds, right?
(If I’m wrong: Can you spell ‘microseconds’ out? We can’t use µ in our source code because it’s not ASCII, but it might not be obvious to everybody that it’s the “American” spelling of µs.)
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.
You're right, this is counted in nanoseconds.
but it might not be obvious to everybody that it’s the “American” spelling of µs
Nah it's not the American spelling; it's the US spelling ;)
gettimeofday(&tp, nullptr); | ||
return MICROS_PER_SEC * tp.tv_sec + tp.tv_usec; | ||
#endif | ||
} |
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.
Is it guaranteed that the result of this call is compatible with uv_hrtime
that we use for PERFORMANCE_NOW
? I guess the test ensures that, but I feel like I’m missing something here…
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.
This is not compatible with ur_hrtime()
, nor is it intended to be compatible. This function returns the real Unix time at which the function is called, while uv_hrtime()
returns a monotonic timestamp based on some arbitrary time in the past.
The only place this is used is in the newly created timeOriginTimestamp
global variable, to which performance.timeOrigin
is set. performance.timeOrigin
is intended to be a Unix timestamp (see #17893).
Further reading: a write-up for my userland implementation of performance
, especially the last two paragraphs
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.
Just a couple of very small things / questions, other than that LGTM.
root) {} | ||
root) { | ||
for (size_t i = 0; i < milestones.Length(); i++) | ||
milestones[i] = -1.; |
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.
Is there a reason this is set to -1.
and not -1
? I'm far from a C++ expert so would love to better understand what's going if there is. Thanks!
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.
They are one and the same in this case. -1 (an integer literal) will get converted to a double anyway. I just wanted to make it explicit that we are assigning a floating point value -1 here.
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.
I figured that might've been the intention but wanted to make sure. Thanks.
test/parallel/test-performance.js
Outdated
assert(Math.abs(performance.timeOrigin - Date.now()) < 300); | ||
|
||
const inited = performance.now(); | ||
assert(inited < 300); |
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.
This has me just slightly concerned since it's somewhat arbitrary and the test is in parallel. Anything that can be done about that? I'm guessing not but figured would check...
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.
Not much, though this indeed failed on some Windows CIs. More relaxed bounds incoming.
8e8e09a
to
497b54d
Compare
CI after fixing Windows (hopefully): https://ci.nodejs.org/job/node-test-pull-request/13522/ |
Fixes: #17892 Fixes: #17893 Fixes: #18992 PR-URL: #18993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Landed in 9256dbb |
Fixes: #17892 Fixes: #17893 Fixes: #18992 PR-URL: #18993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Fixes: #17892 Fixes: #17893 Fixes: #18992 PR-URL: #18993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Fixes: #17892 Fixes: #17893 Fixes: #18992 PR-URL: #18993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Fixes: #17892 Fixes: #17893 Fixes: #18992 PR-URL: #18993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Fixes: #17892 Fixes: #17893 Fixes: #18992 PR-URL: #18993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Fixes: nodejs#17892 Fixes: nodejs#17893 Fixes: nodejs#18992 PR-URL: nodejs#18993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Fixes: nodejs#17892 Fixes: nodejs#17893 Fixes: nodejs#18992 PR-URL: nodejs#18993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Fixes: #17892 Fixes: #17893 Fixes: #18992 Backport-PR-URL: #22380 PR-URL: #18993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Fixes: #17892
Fixes: #17893
Fixes: #18992
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
perf_hooks