Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

use hrtime instead of Date.now for timeouts #5497

Closed
isaacs opened this issue May 17, 2013 · 7 comments
Closed

use hrtime instead of Date.now for timeouts #5497

isaacs opened this issue May 17, 2013 · 7 comments
Labels

Comments

@isaacs
Copy link

isaacs commented May 17, 2013

Semantically, we're doing something like "Do this in 1234ms", rather than "Do this at 10:45:53", so using a wall clock time for this makes hardly any sense.

At least, exposing uv_default_loop()->now to JS-land would be a big improvement, since that saves an extra gettimeofday syscall.

@egorse
Copy link

egorse commented May 15, 2014

@isaacs, @tjfontaine please fix me - if this would works the #7603 wont appears.

@indutny
Copy link
Member

indutny commented May 15, 2014

@egorse thanks for finding this issue, that commit ( timers: use uv_now instead of Date.now ) has landed in v0.11 branch. May I ask you to give it a try? It'll soon become a next stable release anywy.

@egorse
Copy link

egorse commented May 16, 2014

@indutny the 0.11.13 works just fine.

More over, the patch works for 0.10.25 too (except minor correction s/scope(node_isolate);/scope;/)

@indutny
Copy link
Member

indutny commented May 16, 2014

@tjfontaine what do you think about backporting it?

@tjfontaine
Copy link

The reason I didn't backport it initially is because some people are (erroneously) relying on _idleStart-- we can and probably should but leave _idleStart as Date.now() but use a separate private field for Timer.now() that we do our calculations on

@tjfontaine tjfontaine reopened this May 22, 2014
misterdjules added a commit to misterdjules/node that referenced this issue Jul 25, 2014
Original commit message:

 timers: use uv_now instead of Date.now

 This saves a few calls to gettimeofday which can be expensive, and
 potentially subject to clock drift. Instead use the loop time which
 uses hrtime internally.

 fixes nodejs#5497

In addition to the backport, this commit:
 - keeps _idleStart timers' property which is still set to
   Date.now() to avoid breaking existing code that uses it, even if
   its use is discouraged.
 - adds automated tests. These tests use a specific branch of
   libfaketime that hasn't been submitted upstream yet. libfaketime
   is git cloned if needed when running automated tests.
misterdjules added a commit to misterdjules/node that referenced this issue Jul 25, 2014
Original commit message:

 timers: use uv_now instead of Date.now

 This saves a few calls to gettimeofday which can be expensive, and
 potentially subject to clock drift. Instead use the loop time which
 uses hrtime internally.

 fixes nodejs#5497

In addition to the backport, this commit:
 - keeps _idleStart timers' property which is still set to
   Date.now() to avoid breaking existing code that uses it, even if
   its use is discouraged.
 - adds automated tests. These tests use a specific branch of
   libfaketime that hasn't been submitted upstream yet. libfaketime
   is git cloned if needed when running automated tests.
@jasnell
Copy link
Member

jasnell commented May 26, 2015

@misterdjules ... what is the status on this one?

gibfahn pushed a commit to ibmruntimes/node that referenced this issue Mar 31, 2016
Uses better troff formatting.
Removes v8 options from the man page.

Also edits `node -h` in node.cc slightly.

PR-URL: nodejs#5497
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@Fishrock123
Copy link
Member

This is fixed in any timers code I've interacted in (e.g. io.js 1.0.0+), everything uses Timer.now().

0.x is EOL, so I think there is nothing left to be done. Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants