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.

Sign up for GitHub

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

Jump to bottom

hrtime regression #4751

Closed
BridgeAR opened this issue Jan 19, 2016 · 7 comments
Closed

hrtime regression #4751

BridgeAR opened this issue Jan 19, 2016 · 7 comments
Labels
process Issues and PRs related to the process subsystem.

Comments

@BridgeAR
Copy link
Member

I have the feeling 89f056b introduced a regression. The nanoseconds might return negative values if you compare the actual value with the one before. This does not occur in 5.3 or before.

> process.hrtime(t)
[ 45, 373756947 ]
> process.hrtime(t)
[ 46, -1831860 ]

Easy to reproduce with:

var t = process.hrtime(); while(t[1] > 0) t = process.hrtime(t);
@jasnell
Copy link
Member

jasnell commented Jan 19, 2016

@Fishrock123 @trevnorris

@jasnell jasnell added the process Issues and PRs related to the process subsystem. label Jan 19, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Jan 19, 2016

I can reproduce this.

@ChALkeR
Copy link
Member

ChALkeR commented Jan 19, 2016

It does not actually mean negative values, it looks to me that the values are just not normalized.
I.e. it returns [1, -692093386] when it should return [0, 307906614].

@ChALkeR
Copy link
Member

ChALkeR commented Jan 19, 2016

89f056b#diff-6ff379484cbabad48301d485db111c08R198does:
ret[1] = hrValues[2] - ar[1]; does not take into an account that hrValues[2] could be less than ar[1].

@ChALkeR
Copy link
Member

ChALkeR commented Jan 19, 2016

The simplified fast test would be: process.hrtime([0, 999999999])[1] >= 0.

@mscdex
Copy link
Contributor

mscdex commented Jan 19, 2016

I also noticed this recently and was really confused.

@Fishrock123 Fishrock123 mentioned this issue Jan 19, 2016
Merged
@trevnorris
Copy link
Contributor

My bad. I'll write up a patch and additional test case for this ASAP.

@bnoordhuis bnoordhuis mentioned this issue Jan 19, 2016
Closed
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jan 20, 2016
@bnoordhuis
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
evanlucas pushed a commit that referenced this issue Jan 20, 2016
@bnoordhuis @evanlucas
src: fix negative values in process.hrtime()
a2c257a 
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>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
@bnoordhuis
src: fix negative values in process.hrtime()
18a851e 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@mscdex @trevnorris @ChALkeR @jasnell @BridgeAR