This repository has been archived by the owner. It is now read-only.

5-50% speed up setTimeout() #4193

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
6 participants
@AlexeyKupershtokh

Main points are:

  1. Do not set Timeout._when property at all -> save a relatively slow Date.now() call & smaller memory footprint.
  2. Set Timeout._idleStart and Timeout._onTimeout in the constructor due to these recommendations: http://s3.mrale.ph/nodecamp.eu/#55. Many runs of benchmark/settimeout.js verify this gives about 1-2% gain too.

Benchmark results are the following:
node 0.8.12 + timers.js from 0.9.2:

wait...
smaller is better
startup: 11257
done: 999

node 0.8.12 + optimized timers.js from 0.9.2:

wait...
smaller is better
startup: 7610
done: 999

node 0.9.2 reproduces these results as well.

@AlexeyKupershtokh

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Oct 25, 2012

This time the changes are minimalistic and come from a topic branch instead of master.

This time the changes are minimalistic and come from a topic branch instead of master.

@AlexeyKupershtokh

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Oct 29, 2012

This issue is related only to some systems though.

Node's Date.now() performance gained ~15 times on my laptop after upgrading from Ubuntu 12.04 32 bit + PAE to Ubuntu 12.10 64 bit.
There are more details in this thread:
https://groups.google.com/d/msg/nodejs/A4CQx8t5MgQ/AV2Z1VyFB0UJ

So this patch will usually give only 5-6% performance to the setTimeout, and only in some cases it will be 1.5x gain.

This issue is related only to some systems though.

Node's Date.now() performance gained ~15 times on my laptop after upgrading from Ubuntu 12.04 32 bit + PAE to Ubuntu 12.10 64 bit.
There are more details in this thread:
https://groups.google.com/d/msg/nodejs/A4CQx8t5MgQ/AV2Z1VyFB0UJ

So this patch will usually give only 5-6% performance to the setTimeout, and only in some cases it will be 1.5x gain.

lib/timers.js
};
Timeout.prototype.unref = function() {
if (!this._handle) {
- var delay = this._when - Date.now();
+ if (!this._idleStart) this._idleStart = Date.now();
+ var delay = this._idleStart + this._idleTimeout - Date.now();

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Oct 29, 2012

Member

Cache the result of Date.now() in a variable?

@bnoordhuis

bnoordhuis Oct 29, 2012

Member

Cache the result of Date.now() in a variable?

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Oct 29, 2012

Unref is going to be used rarely.
Also it should use actual Date.now in the "var delay = ..." statement, but it should be not set only once in the _idleStart.
So caching in this case is not needed and will be likely harmful.

@AlexeyKupershtokh

AlexeyKupershtokh Oct 29, 2012

Unref is going to be used rarely.
Also it should use actual Date.now in the "var delay = ..." statement, but it should be not set only once in the _idleStart.
So caching in this case is not needed and will be likely harmful.

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Oct 29, 2012

Member

I don't understand what you mean. I'm suggesting this:

var now = Date.now();
if (!this._idleStart) this._idleStart = now;
var delay = this._idleStart + this._idleTimeout - now;

Which could be simplified further.

@bnoordhuis

bnoordhuis Oct 29, 2012

Member

I don't understand what you mean. I'm suggesting this:

var now = Date.now();
if (!this._idleStart) this._idleStart = now;
var delay = this._idleStart + this._idleTimeout - now;

Which could be simplified further.

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Oct 29, 2012

Ah, I completely agree with you.

@AlexeyKupershtokh

AlexeyKupershtokh Oct 29, 2012

Ah, I completely agree with you.

@akzhan

This comment has been minimized.

Show comment Hide comment
@akzhan

akzhan Oct 29, 2012

@AlexeyKupershtokh Simply push additional commit to your topic branch to apply @bnoordhuis' update.

akzhan commented Oct 29, 2012

@AlexeyKupershtokh Simply push additional commit to your topic branch to apply @bnoordhuis' update.

@AlexeyKupershtokh

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Oct 30, 2012

@bnoordhuis I've committed your proposed change with --author=bnoordhuis

@bnoordhuis I've committed your proposed change with --author=bnoordhuis

@AlexeyKupershtokh

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Nov 1, 2012

more details: joyent#4225 (comment)
clearTimeout seems have HUGE performance gain with this patch.

more details: joyent#4225 (comment)
clearTimeout seems have HUGE performance gain with this patch.

@AlexeyKupershtokh

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Nov 1, 2012

@bnoordhuis is this pull request good enough for merging? what else should I do?

@bnoordhuis is this pull request good enough for merging? what else should I do?

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Mar 9, 2013

Yeah, this does make a pretty big impact on benchmark/misc/timers.js.

# with the patch
misc/timers.js thousands=500 type=depth: 562.05
misc/timers.js thousands=500 type=breadth: 1459.8
# v0.10
misc/timers.js thousands=500 type=depth: 318.45
misc/timers.js thousands=500 type=breadth: 1030.8

Running tests and other benchmarks now. Will land if there aren't any other problems.

isaacs commented Mar 9, 2013

Yeah, this does make a pretty big impact on benchmark/misc/timers.js.

# with the patch
misc/timers.js thousands=500 type=depth: 562.05
misc/timers.js thousands=500 type=breadth: 1459.8
# v0.10
misc/timers.js thousands=500 type=depth: 318.45
misc/timers.js thousands=500 type=breadth: 1030.8

Running tests and other benchmarks now. Will land if there aren't any other problems.

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Mar 9, 2013

No discernible effect on http or net benchmarks. I was sort of hoping for some improvement there, since we do spend a lot of our time in gettimeofday due to calling timers.active(socket) whenever we send or receive. (If anyone is feeling ambitious, that might be a good spot to try to shave off some ms.)

But anyway, this helps a little and appears to do no harm. @AlexeyKupershtokh if you would be so kind as to sign the CLA, I will land this with pleasure. http://nodejs.org/cla.html

isaacs commented Mar 9, 2013

No discernible effect on http or net benchmarks. I was sort of hoping for some improvement there, since we do spend a lot of our time in gettimeofday due to calling timers.active(socket) whenever we send or receive. (If anyone is feeling ambitious, that might be a good spot to try to shave off some ms.)

But anyway, this helps a little and appears to do no harm. @AlexeyKupershtokh if you would be so kind as to sign the CLA, I will land this with pleasure. http://nodejs.org/cla.html

@Nodejs-Jenkins

This comment has been minimized.

Show comment Hide comment
@Nodejs-Jenkins

Nodejs-Jenkins Mar 13, 2013

Can one of the admins verify this patch?

Can one of the admins verify this patch?

@AlexeyKupershtokh

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Mar 14, 2013

@isaacs done.

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Mar 28, 2013

@AlexeyKupershtokh You should probably set up your git config to use your github name and email address. It's a bit confusing in the commit history to see wicked <wicked@alawar.com> as the author.

Landed on v0.10. Thanks!

isaacs commented Mar 28, 2013

@AlexeyKupershtokh You should probably set up your git config to use your github name and email address. It's a bit confusing in the commit history to see wicked <wicked@alawar.com> as the author.

Landed on v0.10. Thanks!

@isaacs isaacs closed this Mar 28, 2013

@AlexeyKupershtokh

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Mar 29, 2013

@isaacs yeah. It was one of my first PRs so I hadn't configured git by the time. Sorry for this.

@isaacs yeah. It was one of my first PRs so I hadn't configured git by the time. Sorry for this.

@olecom

This comment has been minimized.

Show comment Hide comment
@olecom

olecom Apr 3, 2013

  • var delay = Date.now();
  • if (!this._idleStart) this._idleStart = delay;
  • delay = this._idleStart + this._idleTimeout - delay;

Why not?

olecom commented on d2cae24 Apr 3, 2013

  • var delay = Date.now();
  • if (!this._idleStart) this._idleStart = delay;
  • delay = this._idleStart + this._idleTimeout - delay;

Why not?

This comment has been minimized.

Show comment Hide comment
@AlexeyKupershtokh

AlexeyKupershtokh Apr 3, 2013

Owner

maybe because this is semantically wrong? :)

maybe because this is semantically wrong? :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.