Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Maximize priority of nextTick #269

Closed
wants to merge 1 commit into from

4 participants

@kriskowal
Owner

Unroll ticks regardless of platform. Use the highest priority event
queue avaliable on each platform.

This should also address issues with Node's newly non-recursive
nextTick. #259.

Seems to have a problem with domains on NodeJS 0.8, but not 0.6 or 0.10.
https://travis-ci.org/kriskowal/q/jobs/6231967

Needs cross browser testing.

@kriskowal Maximize priority of nextTick
Unroll ticks regardless of platform.  Use the highest priority event
queue avaliable on each platform.

This should also address issues with Node's newly non-recursive
nextTick.  #259.
ee23289
@rkatic

Have you tested it with NodeJS 0.10 with cases of many uncaught exceptions? I am travelling and unable to test it for myself right now.

@ForbesLindesay

Looking at this I can't help but feel we should just use a separate next-tick library. There already is one, but it doesn't do any of this clever trampolining stuff. I just feel it's not at all specific or unique to Q that we have this requirement.

The downside would be that it wouldn't be dependency free by default, but we could use browserify to generate a dependency free version. It has a --standalone option that outputs a complete UMD wrapper that's actually based off the UMD wrapper in this repository (I used this repo as my starting point for building umd).

We could then just build the standalone one:

$ browserify q.js --standalone Q > q-standalone.js

and then minify

$ uglifyjs q-standalone.js > q.min.js

The browserify headers are actually a very small overhead in the latest version.

@rkatic

I still have no time for this (next week I will), but I am pretty sure that this will not work in all cases, unless #217 is applied.

@ForbesLindesay I would not recommend to use a separate next-tick library - it would bring even more issues.
Also, having an own solutions, enables us to have maximum control over it, making it more performant (for instance, I have new ideas how to avoid unnecessary "next-turns" that I would like to experiment).

@ForbesLindesay

I was suggesting a separate next-tick library controlled by @kriskowal or someone else similarly invested in q, not just a generic one. It can be a highly specialized one that pretty much states its aims as "implement a next-tick that meets the requirements of promise libraries". My point is that there are tonnes of different promise libraries out there and I'd happily change then to use Qs next-tick implementation, since it's faster than what we're using in lots of situations, but I don't want to just copy and paste it.

@rkatic

@ForbesLindesay In that case, I have no objections, but it should be primarily for Q, since there is probably no an always optimal solution for all promise libraries.

@ForbesLindesay

I agree, Q's needs should come first. My point is simply that as I see it Q's needs are:

  1. The highest priority next-tick possible
  2. Still breaks up the stack to avoid stack overflow
  3. Errors in one handler don't interact with other handlers (e.g. in the browser or when there's a global error handler present).

I imagine there are quite a few applications of such a function.

@rkatic

I ran locally unit tests with this change in NodeJs 0.10, and realized that even if there are not reported errors, those do not complete.
Running jasmine-node --captureExceptions spec, a "progress" related error message is shown. I think this confirms my strong suspicion that current queue implementation does not eliminate recursive nextTick calls.
If process.nextTick is really needed, then we probably have to adopt #217 first.

@kriskowal kriskowal referenced this pull request
Closed

Modularize Q #280

@kriskowal
Owner

@rkatic do you approve of this change? I do intend to modularize Q, but want to land this first.

@domenic
Collaborator

I am becoming increasingly convinced something like this is necessary. Here are some random thoughts:

  • when.js has an implementation that is very simple, from what I can see. Tagging in @briancavalier in case he has any comments.
  • RSVP.js makes use of Mutation Observers, which are very fast as they occur in "micro tasks" instead of "macro tasks". The number of browsers that support MessageChannel but not mutation observers is pretty low, and IE10 has setImmediate which does allow paints but is pretty fast anyway. We'd basically lose Safari 5.x.
  • Both RSVP and when seem to make the simplifying assumption that enqueued functions cannot throw. This seems reasonable if we carefully control who uses this function.
  • It might be worthwhile to rename per #298?
@kriskowal
Owner

@domenic Let’s spin off a next-turn or asap (hahahahahah) package ASAP. We can beat it to death ad nauseam.

(further maniacal laughter elided)

I do not believe that we catch exceptions before nextTick in all cases. Quick scan shows nodeify in particular.

@kriskowal
Owner

By @domenic, I mean @rkatic. We would be delighted to put this in your purview and will gladly help in any way we can, including package management if that will help.

@domenic
Collaborator

I do not believe that we catch exceptions before nextTick in all cases. Quick scan shows nodeify in particular.

Sure, but we could put the burden on nodeify instead of on asap (hahahahaha).

@rkatic
@rkatic

I am going to prepare a new PR in next hours, that will be based on #217 and that will maximize priority of nextTick (and console.error when existent, but not sure yet).

Let’s spin off a next-turn or asap...

I am not aware of a related asap package (not found in npm or github), so I am not able to comment it.
If you are pointing to my next-turn, it is still in experimental phase and it is not tested yet.

I do not believe that we catch exceptions before nextTick in all cases. Quick scan shows nodeify in particular.

I would add progress listeners and any other external Q.nextTick(fn) usage to the list.

Hope to have something well tested before @domenic starts his Q-issies-triage :)

@rkatic

Ok, after searching for a good and performant solution that would adopt #217 and that would not corrupt debugging, I only came with some significant compromises - nothing ideal.

At the end, I realized that the change with this PR is actually not that bad!
Even if in theory current queue is not immune to nextTick recursion, it should be quite safe in practice. Considering that process.maxTickDepth == 1000, thanks to the amortization algorithm, there can be almost 4^1000 uncaught exceptions per event loop with no problems.

Unfortunately, there is still a failure with a unit test. I thought it was related to progress listeners (damn to line wrapping on the line number), but it's actually about node domains ("should be caught by the domain" at line 2119). This problem also persisted after applying other solutions that involved the queue. Not sure if this is a bug in NodeJS. If it is not, to keep the support for node domains, we could be forced to avoid queuing tasks entirely in NodeJS and to adopt a solution with controlled recursions (something similar to the solution in next-turn).
Will investigate this further tomorrow.
Have to go sleep now.

@rkatic

Actually, since current amortization algorithm will almost never require more ticks then the number of pending tasks, it will produce recursion much faster then I stated in my last comment. However, it is probably irrelevant, since a NodeJS process should die (soon) after first uncaught exception (?).

Still not sure how to resolve the issue with node domains. Is it relevant at all? I have no experience on using them, perhaps because NodeJS is relatively new to me...

@rkatic rkatic referenced this pull request
Merged

Reimplement nextTick #316

@kriskowal
Owner

Superseded by #316

@kriskowal kriskowal closed this
@kriskowal kriskowal deleted the priority branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 10, 2013
  1. Maximize priority of nextTick

    authored
    Unroll ticks regardless of platform.  Use the highest priority event
    queue avaliable on each platform.
    
    This should also address issues with Node's newly non-recursive
    nextTick.  #259.
This page is out of date. Refresh to see the latest.
Showing with 107 additions and 94 deletions.
  1. +84 −72 q.js
  2. +1 −1  spec/lib/jasmine-promise.js
  3. +22 −21 spec/q-spec.js
View
156 q.js
@@ -75,86 +75,98 @@ var noop = function () {};
// use the fastest possible means to execute a task in a future turn
// of the event loop.
-var nextTick;
-if (typeof setImmediate === "function") {
- // In IE10, or use https://github.com/NobleJS/setImmediate
- if (typeof window !== "undefined") {
- nextTick = setImmediate.bind(window);
- } else {
- nextTick = setImmediate;
- }
-} else if (typeof process !== "undefined") {
- // node
- nextTick = process.nextTick;
-} else {
- (function () {
- // linked list of tasks (single, with head node)
- var head = {task: void 0, next: null};
- var tail = head;
- var maxPendingTicks = 2;
- var pendingTicks = 0;
- var queuedTasks = 0;
- var usedTicks = 0;
- var requestTick = void 0;
-
- function onTick() {
- // In case of multiple tasks ensure at least one subsequent tick
- // to handle remaining tasks in case one throws.
- --pendingTicks;
-
- if (++usedTicks >= maxPendingTicks) {
- // Amortize latency after thrown exceptions.
- usedTicks = 0;
- maxPendingTicks *= 4; // fast grow!
- var expectedTicks = queuedTasks && Math.min(
- queuedTasks - 1,
- maxPendingTicks
- );
- while (pendingTicks < expectedTicks) {
- ++pendingTicks;
- requestTick();
- }
- }
-
- while (queuedTasks) {
- --queuedTasks; // decrement here to ensure it's never negative
- head = head.next;
- var task = head.task;
- head.task = void 0;
- task();
- }
-
+var nextTick = (function () {
+ // linked list of tasks (single, with head node)
+ var head = {task: void 0, next: null};
+ var tail = head;
+ var maxPendingTicks = 2;
+ var pendingTicks = 0;
+ var queuedTasks = 0;
+ var usedTicks = 0;
+ var requestTick;
+
+ function onTick() {
+ // In case of multiple tasks ensure at least one subsequent tick
+ // to handle remaining tasks in case one throws.
+ --pendingTicks;
+
+ if (++usedTicks >= maxPendingTicks) {
+ // Amortize latency after thrown exceptions.
usedTicks = 0;
- }
-
- nextTick = function (task) {
- tail = tail.next = {task: task, next: null};
- if (
- pendingTicks < ++queuedTasks &&
- pendingTicks < maxPendingTicks
- ) {
+ maxPendingTicks *= 4; // fast grow!
+ var expectedTicks = queuedTasks && Math.min(
+ queuedTasks - 1,
+ maxPendingTicks
+ );
+ while (pendingTicks < expectedTicks) {
++pendingTicks;
requestTick();
}
- };
+ }
- if (typeof MessageChannel !== "undefined") {
- // modern browsers
- // http://www.nonblocking.io/2011/06/windownexttick.html
- var channel = new MessageChannel();
- channel.port1.onmessage = onTick;
- requestTick = function () {
- channel.port2.postMessage(0);
- };
+ while (queuedTasks) {
+ --queuedTasks; // decrement here to ensure it's never negative
+ head = head.next;
+ var task = head.task;
+ head.task = void 0;
+ task();
+ }
+ usedTicks = 0;
+ }
+
+ function nextTick(task) {
+ tail = tail.next = {task: task, next: null};
+ if (
+ pendingTicks < ++queuedTasks &&
+ pendingTicks < maxPendingTicks
+ ) {
+ ++pendingTicks;
+ requestTick();
+ }
+ }
+
+ // Use the highest priority queue that the platform provides
+ if (typeof process !== "undefined") {
+ // NodeJS
+ // process.nextTick is a high priority queue as of version 0.10.
+ // It is low priority on older versions, but we mitigate that problem
+ requestTick = function () {
+ process.nextTick(onTick);
+ };
+ } else if (typeof MessageChannel !== "undefined") {
+ // modern browsers
+ // http://www.nonblocking.io/2011/06/windownexttick.html
+ // Message channels are high priority, meaning drawing and IO are not
+ // interleaved.
+ var channel = new MessageChannel();
+ channel.port1.onmessage = onTick;
+ requestTick = function () {
+ channel.port2.postMessage(0);
+ };
+ } else if (typeof setImmediate === "function") {
+ // setImmediate is low priority in both browsers and NodeJS.
+ // In IE10, or use https://github.com/NobleJS/setImmediate
+ var requestImmediate;
+ if (typeof window !== "undefined") {
+ requestImmediate = setImmediate.bind(window);
} else {
- // old browsers
- requestTick = function () {
- setTimeout(onTick, 0);
- };
+ requestImmediate = setImmediate;
}
- })();
-}
+ requestTick = function () {
+ requestImmediate(onTick);
+ };
+ } else {
+ // old browsers
+ // setTimeout is not only low priority, but has a minimum delay, but is
+ // the only show in town on old browsers
+ requestTick = function () {
+ setTimeout(onTick, 0);
+ };
+ }
+
+ return nextTick;
+})();
// Attempt to make generics safe in the face of downstream
// modifications.
View
2  spec/lib/jasmine-promise.js
@@ -28,7 +28,7 @@ jasmine.Block.prototype.execute = function (onComplete) {
}, function (error) {
spec.fail(error);
onComplete();
- });
+ })
} else if (this.func.length === 0) {
onComplete();
}
View
43 spec/q-spec.js
@@ -19,27 +19,6 @@ afterEach(function () {
Q.onerror = null;
});
-describe("computing sum of integers using promises", function() {
- it("should compute correct result without blowing stack", function () {
- var array = [];
- var iters = 1000;
- for (var i = 1; i <= iters; i++) {
- array.push(i);
- }
-
- var pZero = Q.fulfill(0);
- var result = array.reduce(function (promise, nextVal) {
- return promise.then(function (currentVal) {
- return Q.fulfill(currentVal + nextVal);
- });
- }, pZero);
-
- return result.then(function (value) {
- expect(value).toEqual(iters * (iters + 1) / 2);
- });
- });
-});
-
describe("Q function", function () {
it("should result in a fulfilled promise when given a value", function () {
expect(Q(5).isFulfilled()).toBe(true);
@@ -2362,3 +2341,25 @@ describe("unhandled rejection reporting", function () {
expect(Q.unhandledReasons.length).toEqual(0);
});
});
+
+describe("computing sum of integers using promises", function() {
+ it("should compute correct result without blowing stack", function () {
+ var array = [];
+ var iters = 1000;
+ for (var i = 1; i <= iters; i++) {
+ array.push(i);
+ }
+
+ var pZero = Q.fulfill(0);
+ var result = array.reduce(function (promise, nextVal) {
+ return promise.then(function (currentVal) {
+ return Q.fulfill(currentVal + nextVal);
+ });
+ }, pZero);
+
+ return result.then(function (value) {
+ expect(value).toEqual(iters * (iters + 1) / 2);
+ });
+ });
+});
+
Something went wrong with that request. Please try again.