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

timers: improve setTimeout/Interval performance #8661

Merged
merged 1 commit into from
Oct 1, 2016
Merged

timers: improve setTimeout/Interval performance #8661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
206 changes: 106 additions & 100 deletions lib/timers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ const TIMEOUT_MAX = 2147483647; // 2^31-1
//
// - key = time in milliseconds
// - value = linked list
const refedLists = {};
const unrefedLists = {};
const refedLists = Object.create(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we had a long discussion about this, I think for http. Have we ever come to a conclusion whether this is a good idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The objects here are not exposed, so it's not an issue in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. And it does improve performance? Because - at least for me - it decreases readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't measure it specifically, but it does remove unnecessary hash table entries. It may also help if V8 sees only numeric keys being set on the object?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fhinkel Just curious. Do you mean using a Map instead of Object.create? #7581 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it increases readability, but please check performance - I recall there being a regression when using .create(null) at one point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Florian-R Thanks, that's the discussion I was thinking of. But in this case I meant {} vs .create(null).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjamingr I'm not sure what regression you're referring to. These objects are created at startup, not during runtime, so the time to create them doesn't matter as much.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this is not about performance, but just out of curiosity, I measured the performance difference.

console.time("{}"); for (var i = 100000; i--;) { var a = {}; } console.timeEnd("{}"); var create = Object.create; console.time("create"); for (var i = 100000; i--;) { var b = create(null); } console.timeEnd("create");

{}: 5.792ms
create: 15.221ms

Probably, calling a function is heavier than {}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Literals are always faster than instantiation and builder function calls, in JS and most languages.

const unrefedLists = Object.create(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any tangible benefit to doing this.

We only use Number value keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As was previously mentioned, I didn't measure the change specifically, but it does remove unnecessary hash table entries. It may also help if V8 sees only numeric keys being set on the object? It has nothing to do with key name collisions in this case.



// Schedule or re-schedule a timer.
Expand Down Expand Up @@ -128,23 +128,28 @@ function insert(item, unrefed) {
var list = lists[msecs];
if (!list) {
debug('no %d list was found in insert, creating a new one', msecs);
// Make a new linked list of timers, and create a TimerWrap to schedule
// processing for the list.
list = new TimersList(msecs, unrefed);
L.init(list);
list._timer._list = list;

if (unrefed === true) list._timer.unref();
list._timer.start(msecs);

lists[msecs] = list;
list._timer[kOnTimeout] = listOnTimeout;
lists[msecs] = list = createTimersList(msecs, unrefed);
}

L.append(list, item);
assert(!L.isEmpty(list)); // list is not empty
}

function createTimersList(msecs, unrefed) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure why we'd want this in an extra function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think extracting it into a function increases readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason is to keep the insert() size down, but it does also help a bit with readability I suppose.

// Make a new linked list of timers, and create a TimerWrap to schedule
// processing for the list.
const list = new TimersList(msecs, unrefed);
L.init(list);
list._timer._list = list;

if (unrefed === true) list._timer.unref();
list._timer.start(msecs);

list._timer[kOnTimeout] = listOnTimeout;

return list;
}

function TimersList(msecs, unrefed) {
this._idleNext = null; // Create the list with the linkedlist properties to
this._idlePrev = null; // prevent any unnecessary hidden class changes.
Expand Down Expand Up @@ -229,7 +234,7 @@ function tryOnTimeout(timer, list) {
timer._called = true;
var threw = true;
try {
timer._onTimeout();
ontimeout(timer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ontimeout() throws will it deo-opt that function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which function? tryOnTimeout()? ontimeout()? The user's callback?

I don't know that throwing causes a deopt or not, but adding a try-* block certainly does cause a deopt for the containing function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but adding a try-* block certainly does cause a deopt for the containing function.

Right, which is why it was constrained to within one separate function.

I'm curious if a throw fork a user callback de-opts ontimeout() along the way though, since that could be somewhat unpleasant.

Maybe we shouldn't worry about it? Idk the sate of benchmarking this stuff is already so bad 😐

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wrote a little test and as far as I can tell a function (or any calling functions in the call stack) shouldn't get deopted if that function throws.

Copy link

@leodutra leodutra Oct 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try deopt any container function. Node.js should, IMHO, always test for nullity and use flags/optionals and never try-catch in the inner functions... This pattern is not really a requirement in functional programming and there's huge performance benefits on native calls (basically, in any platform too. try-catch system is expensive).

threw = false;
} finally {
if (!threw) return;
Expand Down Expand Up @@ -317,51 +322,76 @@ exports.enroll = function(item, msecs) {
*/


exports.setTimeout = function(callback, after) {
exports.setTimeout = function(callback, after, arg1, arg2, arg3) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I like this much better than using arguments.

Copy link
Contributor

@Fishrock123 Fishrock123 Sep 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...args ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest args are still slow.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great move.

if (typeof callback !== 'function') {
throw new TypeError('"callback" argument must be a function');
}

after *= 1; // coalesce to number or NaN

if (!(after >= 1 && after <= TIMEOUT_MAX)) {
after = 1; // schedule on next tick, follows browser behaviour
var len = arguments.length;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check for typeof args1 == 'undefined' rather than using length? Or is undefined a valid value for args1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say it's a valid value.

var args;
if (len === 3) {
args = [arg1];
} else if (len === 4) {
args = [arg1, arg2];
} else if (len > 4) {
args = [arg1, arg2, arg3];
for (var i = 5; i < len; i++)
// extend array dynamically, makes .apply run much faster in v6.0.0
args[i - 2] = arguments[i];
}

var timer = new Timeout(after);
var length = arguments.length;
var ontimeout = callback;
switch (length) {
// fast cases
case 1:
case 2:
break;
case 3:
ontimeout = () => callback.call(timer, arguments[2]);
break;
case 4:
ontimeout = () => callback.call(timer, arguments[2], arguments[3]);
break;
case 5:
ontimeout =
() => callback.call(timer, arguments[2], arguments[3], arguments[4]);
break;
// slow case
default:
var args = new Array(length - 2);
for (var i = 2; i < length; i++)
args[i - 2] = arguments[i];
ontimeout = () => callback.apply(timer, args);
break;
}
timer._onTimeout = ontimeout;
return createSingleTimeout(callback, after, args);
};

if (process.domain) timer.domain = process.domain;
function createSingleTimeout(callback, after, args) {
after *= 1; // coalesce to number or NaN
if (!(after >= 1 && after <= TIMEOUT_MAX))
after = 1; // schedule on next tick, follows browser behaviour

var timer = new Timeout(after, callback, args);
if (process.domain)
timer.domain = process.domain;

active(timer);

return timer;
};
}


function ontimeout(timer) {
var args = timer._timerArgs;
var callback = timer._onTimeout;
if (!args)
callback.call(timer);
else {
switch (args.length) {
case 1:
callback.call(timer, args[0]);
break;
case 2:
callback.call(timer, args[0], args[1]);
break;
case 3:
callback.call(timer, args[0], args[1], args[2]);
break;
default:
callback.apply(timer, args);
}
}
if (timer._repeat)
rearm(timer);
}


function rearm(timer) {
// If timer is unref'd (or was - it's permanently removed from the list.)
if (timer._handle && timer instanceof Timeout) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should only be timer._handle I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not, as _http_outgoing and net have a ._handle and manually enroll() pre-made objects. Node will crash since those ._handle instances are not only not Timeout instances, but they also do not have a .start() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the reason this change is necessary now is that the rearming code no longer has access to the scope where the Timeout instance was created.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmmm ok

timer._handle.start(timer._repeat);
} else {
timer._idleTimeout = timer._repeat;
active(timer);
}
}


const clearTimeout = exports.clearTimeout = function(timer) {
Expand All @@ -376,66 +406,41 @@ const clearTimeout = exports.clearTimeout = function(timer) {
};


exports.setInterval = function(callback, repeat) {
exports.setInterval = function(callback, repeat, arg1, arg2, arg3) {
if (typeof callback !== 'function') {
throw new TypeError('"callback" argument must be a function');
}

repeat *= 1; // coalesce to number or NaN
var len = arguments.length;
var args;
if (len === 3) {
args = [arg1];
} else if (len === 4) {
args = [arg1, arg2];
} else if (len > 4) {
args = [arg1, arg2, arg3];
for (var i = 5; i < len; i++)
// extend array dynamically, makes .apply run much faster in v6.0.0
args[i - 2] = arguments[i];
}

return createRepeatTimeout(callback, repeat, args);
};

if (!(repeat >= 1 && repeat <= TIMEOUT_MAX)) {
function createRepeatTimeout(callback, repeat, args) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a new function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep function sizes down.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, there's no repetead call, you add a short flow path (and creates a mini and fake cyclomatic complexity), function is in the same script and right under the function (so you just added brackets and more names to read). Function call is slower than direct structured code (and even minimal performance gains, in this call stack queen, is important).

I would not do this separation.

repeat *= 1; // coalesce to number or NaN
if (!(repeat >= 1 && repeat <= TIMEOUT_MAX))
repeat = 1; // schedule on next tick, follows browser behaviour
}

var timer = new Timeout(repeat);
var length = arguments.length;
var ontimeout = callback;
switch (length) {
case 1:
case 2:
break;
case 3:
ontimeout = () => callback.call(timer, arguments[2]);
break;
case 4:
ontimeout = () => callback.call(timer, arguments[2], arguments[3]);
break;
case 5:
ontimeout =
() => callback.call(timer, arguments[2], arguments[3], arguments[4]);
break;
default:
var args = new Array(length - 2);
for (var i = 2; i < length; i += 1)
args[i - 2] = arguments[i];
ontimeout = () => callback.apply(timer, args);
break;
}
timer._onTimeout = wrapper;
timer._repeat = ontimeout;
var timer = new Timeout(repeat, callback, args);
timer._repeat = repeat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure if changing this won't have side-effects somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I grepped lib/ and ._repeat isn't used anywhere except timers.js.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more concerned about user code, I do understand that it is unlikely but it has been the way it was an awfully long time in the wild.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you rather I leave the variable set to null since it wouldn't be used anymore and add a new property that stores the interval value?

if (process.domain)
timer.domain = process.domain;

if (process.domain) timer.domain = process.domain;
active(timer);

return timer;

function wrapper() {
timer._repeat();

// Timer might be closed - no point in restarting it
if (!timer._repeat)
return;

// If timer is unref'd (or was - it's permanently removed from the list.)
if (this._handle) {
this._handle.start(repeat);
} else {
timer._idleTimeout = repeat;
active(timer);
}
}
};

}

exports.clearInterval = function(timer) {
if (timer && timer._repeat) {
Expand All @@ -445,19 +450,20 @@ exports.clearInterval = function(timer) {
};


function Timeout(after) {
function Timeout(after, callback, args) {
this._called = false;
this._idleTimeout = after;
this._idlePrev = this;
this._idleNext = this;
this._idleStart = null;
this._onTimeout = null;
this._onTimeout = callback;
this._timerArgs = args;
this._repeat = null;
}


function unrefdHandle() {
this.owner._onTimeout();
ontimeout(this.owner);
Copy link

@leodutra leodutra Oct 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always better for internal workings. ALWAYS, Node community.
Avoid this, globals, function calls, function separation in minimal pieces and inner functions... and we'll always have fine code with max JS performance.

if (!this.owner._repeat)
this.owner.close();
}
Expand Down
1 change: 1 addition & 0 deletions test/message/timeout_throw.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
^
ReferenceError: undefined_reference_error_maker is not defined
at Timeout._onTimeout (*test*message*timeout_throw.js:*:*)
at ontimeout (timers.js:*:*)
at tryOnTimeout (timers.js:*:*)
at Timer.listOnTimeout (timers.js:*:*)