-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
#include "node.h" | ||
#include "v8.h" | ||
|
||
#include <algorithm> | ||
#include <map> | ||
#include <string> | ||
|
||
|
@@ -76,7 +77,10 @@ class performance_state { | |
isolate, | ||
offsetof(performance_state_internal, observers), | ||
NODE_PERFORMANCE_ENTRY_TYPE_INVALID, | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason this is set to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
} | ||
|
||
AliasedBuffer<uint8_t, v8::Uint8Array> root; | ||
AliasedBuffer<double, v8::Float64Array> milestones; | ||
|
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 forPERFORMANCE_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, whileuv_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 whichperformance.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