src: fix negative values in process.hrtime() #4757

bnoordhuis · 2016-01-19T10:50:30Z

Fix a regression introduced in commit 89f056b ("node: improve
performance of hrtime()") where the nanosecond field sometimes
had a negative value when calculating the difference between two
timestamps.

Fixes: #4751

R=@trevnorris

[Can't run the CI - https://ci.nodejs.org/ times out.]

evanlucas · 2016-01-19T11:43:09Z

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

evanlucas · 2016-01-19T11:43:37Z

LGTM if CI and @trevnorris are happy

BridgeAR · 2016-01-19T12:03:03Z

test/parallel/test-process-hrtime.js

@@ -24,3 +24,9 @@ function validateTuple(tuple) {
    assert(isFinite(v));
  });
 }
+
+const base = process.hrtime();
+for (let i = 0; i < 1e5; i += 1) {


I think using assert(process.hrtime([0, 999999999])[1] >= 0); as @ChALkeR suggested would be better than my test.

assert(process.hrtime([0, 1e9 - 1])[1] >= 0); then, I think.

well if we really want to be sure then assert(process.hrtime([0, -1 >>> 0])[1] >= 0)

Why? I doubt that's valid input for process.hrtime.

Because it properly decremented the seconds for any uint32 ns passed previous to my change. Therefore it should probably continue to work?

trevnorris · 2016-01-19T18:13:29Z

@bnoordhuis since the code previously was doing FromUnsigned in native, the following would work:

> process.hrtime([-1 >>> 0, -1 >>> 0])
[ 1267632966, 506597361 ]

Whereas with this patch (and before the patch) it shows as something like [ -4294209242, -2846409641 ]. That still a regression, or since the array value is outside the expected range it's not?

EDIT: Seems the math used to not care how large a value in the uint32 range was passed. e.g.

> process.hrtime([0, -1 >>> 0])
[ 758412, 512694013 ]
> process.hrtime()
[ 758418, 266142011 ]

Fix a regression introduced in commit 89f056b ("node: improve performance of hrtime()") where the nanosecond field sometimes had a negative value when calculating the difference between two timestamps. Fixes: nodejs#4751

ChALkeR · 2016-01-19T19:35:02Z

@trevnorris Is that valid?
https://nodejs.org/api/process.html#process_process_hrtime states:

You may pass in the result of a previous call to process.hrtime() to get a diff reading, useful for benchmarks and measuring intervals

[0, -1 >>> 0] can not come as a result from process.hrtime() call, so it's a bit out of scope. What happens to it seems more like implementation detail to me, and I do not see a problem if that returns negative values in the nanosecond field.

trevnorris · 2016-01-19T20:46:29Z

@ChALkeR Is passing a larger seconds value supposed to return a negative value? If not then there's still a regression with this patch.

ChALkeR · 2016-01-19T22:58:55Z

@trevnorris Hm. As I read the doc, the passed value should be a result of some previous call to process.hrtime(), not some arbitrary constructed array.

As it looks to me, uv_hrtime (and therefore process.hrtime()) are not affected by any system time changes, so new values should not be smaller than old values.

I would say that the current behavior introduced by this patch is ok.

If it could be altered to return expected values for other cases (when the passed in value is not a result of some a previous call) without considerable drawbacks (speed, code readability, etc), perhaps that should be done to keep us on the safe side. But I don't see a problem in not doing that.

trevnorris · 2016-01-19T23:14:33Z

I see. Not trying to replicate all the old behavior, just the correct part. LGTM.

CI is happy.

ChALkeR · 2016-01-20T06:48:40Z

@trevnorris Btw, I have just checked the old behavior. It also does not support values that are larger that the current ones, negative values, and arbitrary large numbers in nanosecond field.

Examples (on 5.0.0):

> process.hrtime()
[ 741222, 372827150 ]
> process.hrtime([0,0])
[ 741226, 252473283 ]
> process.hrtime([740000,0])
[ 1237, 26668592 ]
> process.hrtime([750000,0])
[ 1266866129, 303942850 ]
> process.hrtime([-1,0])
[ 1267616149, 293166365 ]
> process.hrtime([0,999999999])
[ 741274, 29108495 ]
> process.hrtime([0,99999999999999])
[ 741311, 514787310 ]
> process.hrtime([0,0])
[ 741404, 826126044 ]
> process.hrtime([0,-1])
[ 741402, 130741800 ]

Everything that has negative values gives wrong results here, [0,99999999999999] gives wrong results, [750000,0] also gives wrong results. And that's ok per doc, and that's how it was in 5.0.0. I don't see a point in trying to support arbitrary input.

trevnorris · 2016-01-20T10:27:08Z

@ChALkeR If the ns field is a valid uint32 then the seconds used to decrement correctly because the values were combined in a int64 on the native side before doing the subtraction.

bnoordhuis · 2016-01-20T10:32:12Z

Updated the test to just do process.hrtime([0, 1e9-1]), PTAL. CI: https://ci.nodejs.org/job/node-test-pull-request/1317/

Apropos the discussion about "user-cooked" values, I agree with @ChALkeR that GIGO applies. If you throw in the wrong numbers, don't expect the right answers to come out.

evanlucas · 2016-01-20T17:30:06Z

Looks like the linter isn't happy.

bnoordhuis · 2016-01-20T18:29:15Z

Style fix-up pushed. Running the CI one more time, last time one of the windows buildbots crapped out: https://ci.nodejs.org/job/node-test-pull-request/1320/

trevnorris · 2016-01-20T18:56:36Z

Apropos the discussion about "user-cooked" values, I agree with @ChALkeR that GIGO applies. If you throw in the wrong numbers, don't expect the right answers to come out.

Cool. Just wanted to clear up the line between accidentally working and expected behavior.

Windows CI failures are not related.

ChALkeR · 2016-01-20T18:59:08Z

LGTM

Fix a regression introduced in commit 89f056b ("node: improve performance of hrtime()") where the nanosecond field sometimes had a negative value when calculating the difference between two timestamps. Fixes: nodejs#4751 PR-URL: nodejs#4757 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

bnoordhuis · 2016-01-20T21:04:51Z

Landed in 1e5a026.

Fix a regression introduced in commit 89f056b ("node: improve performance of hrtime()") where the nanosecond field sometimes had a negative value when calculating the difference between two timestamps. Fixes: #4751 PR-URL: #4757 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

MylesBorins · 2016-02-17T19:21:10Z

89f056b did not land on LTS. As such I am adding a dont-land flag on this pr for v4.

Please feel free to chime in you you have questions or concerns

Fix a regression introduced in commit 89f056b ("node: improve performance of hrtime()") where the nanosecond field sometimes had a negative value when calculating the difference between two timestamps. Fixes: nodejs#4751 PR-URL: nodejs#4757 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

bnoordhuis added the process Issues and PRs related to the process subsystem. label Jan 19, 2016

BridgeAR reviewed Jan 19, 2016
View reviewed changes

Fishrock123 mentioned this pull request Jan 19, 2016

v5.5.0 Release Proposal (Stable) #4742

Merged

src: fix negative values in process.hrtime()

dcafe71

Fix a regression introduced in commit 89f056b ("node: improve performance of hrtime()") where the nanosecond field sometimes had a negative value when calculating the difference between two timestamps. Fixes: nodejs#4751

bnoordhuis force-pushed the fix4751 branch from 422e146 to dcafe71 Compare January 20, 2016 10:25

fixup! shut up linter

142e0bc

bnoordhuis force-pushed the fix4751 branch from 142e0bc to 1e5a026 Compare January 20, 2016 21:02

bnoordhuis closed this Jan 20, 2016

jasnell added the lts-watch-v4.x label Jan 20, 2016

rvagg mentioned this pull request Feb 8, 2016

Release proposal: v5.5.1 (Stable) #5141

Closed

MylesBorins added the dont-land-on-v4.x label Feb 17, 2016

MylesBorins removed the lts-watch-v4.x label Feb 17, 2016

ChALkeR mentioned this pull request May 6, 2016

doc: update process.hrtime docs to include optional parameter #6585

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix negative values in process.hrtime() #4757

src: fix negative values in process.hrtime() #4757

bnoordhuis commented Jan 19, 2016

evanlucas commented Jan 19, 2016

evanlucas commented Jan 19, 2016

BridgeAR Jan 19, 2016

ChALkeR Jan 19, 2016

trevnorris Jan 19, 2016

ChALkeR Jan 19, 2016

trevnorris Jan 19, 2016

trevnorris commented Jan 19, 2016

ChALkeR commented Jan 19, 2016

trevnorris commented Jan 19, 2016

ChALkeR commented Jan 19, 2016

trevnorris commented Jan 19, 2016

ChALkeR commented Jan 20, 2016

trevnorris commented Jan 20, 2016

bnoordhuis commented Jan 20, 2016

evanlucas commented Jan 20, 2016

bnoordhuis commented Jan 20, 2016

trevnorris commented Jan 20, 2016

ChALkeR commented Jan 20, 2016

bnoordhuis commented Jan 20, 2016

MylesBorins commented Feb 17, 2016

src: fix negative values in process.hrtime() #4757

src: fix negative values in process.hrtime() #4757

Conversation

bnoordhuis commented Jan 19, 2016

evanlucas commented Jan 19, 2016

evanlucas commented Jan 19, 2016

BridgeAR Jan 19, 2016

Choose a reason for hiding this comment

ChALkeR Jan 19, 2016

Choose a reason for hiding this comment

trevnorris Jan 19, 2016

Choose a reason for hiding this comment

ChALkeR Jan 19, 2016

Choose a reason for hiding this comment

trevnorris Jan 19, 2016

Choose a reason for hiding this comment

trevnorris commented Jan 19, 2016

ChALkeR commented Jan 19, 2016

trevnorris commented Jan 19, 2016

ChALkeR commented Jan 19, 2016

trevnorris commented Jan 19, 2016

ChALkeR commented Jan 20, 2016

trevnorris commented Jan 20, 2016

bnoordhuis commented Jan 20, 2016

evanlucas commented Jan 20, 2016

bnoordhuis commented Jan 20, 2016

trevnorris commented Jan 20, 2016

ChALkeR commented Jan 20, 2016

bnoordhuis commented Jan 20, 2016

MylesBorins commented Feb 17, 2016