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.

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

src: don't lazy-load timer globals #1280

Merged
merged 1 commit into from
Mar 26, 2015

Conversation

bnoordhuis
Copy link
Member

Don't lazy-load setInterval(), setTimeout(), etc. Most applications are
going to need them and routing every call through NativeModule.require()
and Function#apply() is not exactly efficient.

R=@trevnorris?

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/382/

@Fishrock123
Copy link
Member

LGTM

Don't lazy-load setInterval(), setTimeout(), etc.  Most applications are
going to need them and routing every call through NativeModule.require()
and Function#apply() is not exactly efficient.

PR-URL: nodejs#1280
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@bnoordhuis bnoordhuis closed this Mar 26, 2015
@bnoordhuis bnoordhuis deleted the dont-lazy-load-timer-globals branch March 26, 2015 23:16
@bnoordhuis bnoordhuis merged commit 2903410 into nodejs:v1.x Mar 26, 2015
@petkaantonov
Copy link
Contributor

Function#apply() is not exactly efficient

just a side note: this was using .apply in the one and only way v8 recognizes and optimizes for, that is .apply(x, arguments) :p

@bnoordhuis
Copy link
Member Author

Sure, but it's still less efficient than calling the function directly.

@targos
Copy link
Member

targos commented Mar 27, 2015

What about lazy-loading the functions only on first call ?

global.setTimeout = function() {
  var t = NativeModule.require('timers');
  global.setTimeout = t.setTimeout;
  return t.setTimeout.apply(this, arguments);
};

@bnoordhuis
Copy link
Member Author

What good would that do? This PR is based off the observation that the timers module is almost certainly going to get loaded anyway (by core, if not the user) so you might as well do it eagerly.

@rvagg rvagg mentioned this pull request Mar 28, 2015
Qard added a commit to Qard/async-listener that referenced this pull request Apr 7, 2015
Timer functions are now mounted directly on global as of iojs 1.6.3
See: nodejs/node#1280
Qard added a commit to othiym23/async-listener that referenced this pull request Apr 8, 2015
Timer functions are now mounted directly on global as of iojs 1.6.3
See: nodejs/node#1280
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants