perf_hooks: fix timing #18993

TimothyGu · 2018-02-25T23:55:44Z

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 · 2018-02-25T23:56:27Z

CI: https://ci.nodejs.org/job/node-test-commit/16492/

addaleax · 2018-02-26T15:14:36Z

lib/perf_hooks.js

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

+function getMilestoneTimestamp(milestoneIdx) {
+  const us = milestones[milestoneIdx];


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

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

addaleax · 2018-02-26T15:21:25Z

src/node_perf.cc

+  gettimeofday(&tp, nullptr);
+  return MICROS_PER_SEC * tp.tv_sec + tp.tv_usec;
+#endif
+}


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…

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 · 2018-03-01T23:41:23Z

CI https://ci.nodejs.org/job/node-test-pull-request/13429/

apapirovski

Just a couple of very small things / questions, other than that LGTM.

apapirovski · 2018-03-02T12:53:55Z

src/node_perf_common.h

-      root) {}
+      root) {
+    for (size_t i = 0; i < milestones.Length(); i++)
+      milestones[i] = -1.;


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!

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.

I figured that might've been the intention but wanted to make sure. Thanks.

apapirovski · 2018-03-02T12:58:42Z

test/parallel/test-performance.js

+assert(Math.abs(performance.timeOrigin - Date.now()) < 300);
+
+const inited = performance.now();
+assert(inited < 300);


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

Not much, though this indeed failed on some Windows CIs. More relaxed bounds incoming.

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

TimothyGu · 2018-03-04T22:11:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13494/

TimothyGu · 2018-03-06T06:37:55Z

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

jasnell · 2018-03-06T16:42:11Z

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: 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]>

TimothyGu added the perf_hooks Issues and PRs related to the implementation of the Performance Timing API. label Feb 25, 2018

TimothyGu requested a review from jasnell February 25, 2018 23:55

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

jasnell approved these changes Feb 26, 2018

View reviewed changes

addaleax approved these changes Feb 26, 2018

View reviewed changes

cjihrig approved these changes Feb 28, 2018

View reviewed changes

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

apapirovski approved these changes Mar 2, 2018

View reviewed changes

TimothyGu added 3 commits March 4, 2018 13:58

perf_hooks: fix timing

484a3d8

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

fixup! fix units in variable name

d8a0744

fixup! Reduce expected precision

497b54d

TimothyGu force-pushed the performance-bug-fixes branch from 8e8e09a to 497b54d Compare March 4, 2018 22:03

fixup! fix FILETIME interpretation

ccc0ed2

apapirovski approved these changes Mar 6, 2018

View reviewed changes

jasnell closed this Mar 6, 2018

TimothyGu deleted the performance-bug-fixes branch March 6, 2018 18:13

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

MylesBorins mentioned this pull request Sep 6, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf_hooks: fix timing #18993

perf_hooks: fix timing #18993

TimothyGu commented Feb 25, 2018

TimothyGu commented Feb 25, 2018

addaleax Feb 26, 2018

TimothyGu Feb 27, 2018 •

edited

Loading

addaleax Feb 26, 2018

TimothyGu Feb 27, 2018 •

edited

Loading

BridgeAR commented Mar 1, 2018

apapirovski left a comment

apapirovski Mar 2, 2018

TimothyGu Mar 4, 2018

apapirovski Mar 4, 2018

apapirovski Mar 2, 2018

TimothyGu Mar 4, 2018

TimothyGu commented Mar 4, 2018

TimothyGu commented Mar 6, 2018

jasnell commented Mar 6, 2018

perf_hooks: fix timing #18993

perf_hooks: fix timing #18993

Conversation

TimothyGu commented Feb 25, 2018

Checklist

Affected core subsystem(s)

TimothyGu commented Feb 25, 2018

addaleax Feb 26, 2018

Choose a reason for hiding this comment

TimothyGu Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

addaleax Feb 26, 2018

Choose a reason for hiding this comment

TimothyGu Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Mar 1, 2018

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Mar 2, 2018

Choose a reason for hiding this comment

TimothyGu Mar 4, 2018

Choose a reason for hiding this comment

apapirovski Mar 4, 2018

Choose a reason for hiding this comment

apapirovski Mar 2, 2018

Choose a reason for hiding this comment

TimothyGu Mar 4, 2018

Choose a reason for hiding this comment

TimothyGu commented Mar 4, 2018

TimothyGu commented Mar 6, 2018

jasnell commented Mar 6, 2018

TimothyGu Feb 27, 2018 •

edited

Loading

TimothyGu Feb 27, 2018 •

edited

Loading