Skip to content
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

Closed
wants to merge 4 commits into from
Closed

Conversation

TimothyGu
Copy link
Member

Fixes: #17892
Fixes: #17893
Fixes: #18992

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

perf_hooks

@TimothyGu TimothyGu added the perf_hooks Issues and PRs related to the implementation of the Performance Timing API. label Feb 25, 2018
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Feb 25, 2018
@TimothyGu
Copy link
Member Author

@@ -145,6 +146,13 @@ function now() {
return hr[0] * 1000 + hr[1] / 1e6;
}

function getMilestoneTimestamp(milestoneIdx) {
const us = milestones[milestoneIdx];
Copy link
Member

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.)

Copy link
Member Author

@TimothyGu TimothyGu Feb 27, 2018

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
}
Copy link
Member

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…

Copy link
Member Author

@TimothyGu TimothyGu Feb 27, 2018

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

@BridgeAR
Copy link
Member

BridgeAR commented Mar 1, 2018

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018
Copy link
Member

@apapirovski apapirovski left a 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.;
Copy link
Member

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!

Copy link
Member Author

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.

Copy link
Member

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.

assert(Math.abs(performance.timeOrigin - Date.now()) < 300);

const inited = performance.now();
assert(inited < 300);
Copy link
Member

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...

Copy link
Member Author

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.

@TimothyGu
Copy link
Member Author

@TimothyGu
Copy link
Member Author

CI after fixing Windows (hopefully): https://ci.nodejs.org/job/node-test-pull-request/13522/

jasnell pushed a commit that referenced this pull request Mar 6, 2018
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]>
@jasnell
Copy link
Member

jasnell commented Mar 6, 2018

Landed in 9256dbb

@jasnell jasnell closed this Mar 6, 2018
@TimothyGu TimothyGu deleted the performance-bug-fixes branch March 6, 2018 18:13
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
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]>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
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]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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]>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
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]>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. perf_hooks Issues and PRs related to the implementation of the Performance Timing API.
Projects
None yet
7 participants