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

Optimize nextTick #195

Merged
merged 18 commits into from
Feb 13, 2013
Merged

Optimize nextTick #195

merged 18 commits into from
Feb 13, 2013

Conversation

rkatic
Copy link
Collaborator

@rkatic rkatic commented Feb 2, 2013

  • Minimize the number of postMessage/setTimeout calls.
  • Remove structural changes on list nodes.

OK, this is perhaps an "over-optimization", but could still be relevant specially for old browsers, where setTimeout is used and promises are used during some animation.

Currently if you run Q.when(1).then().then().then().then(), nextTick will be called 13 times.
With this change, that code will produce only one postMessage/setTimeout (+1 to survive eventual error), but still respecting the spec.

Please, let me know if I am missing something.

NOTE: This pull would make pull-191 unnecessary.

- Minimize the number of postMessage/setTimeout calls.
- Remove structural changes on list nodes.
@domenic
Copy link
Collaborator

domenic commented Feb 2, 2013

The problem with this approach is that it breaks subsequent handlers if one throws.

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 2, 2013

It shouldn't. I use a preventive postMessage/setTimeout to handle remaining tasks even if one throws.

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 2, 2013

@domenic I added a comment that should clarify the approach. Feel free to suggest an better one.

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 3, 2013

Here a performance test: http://jsperf.com/wqgrecrereffrre/3

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 4, 2013

To put in a better perspective, in test http://jsperf.com/wqgrecrereffrre/4 I also added the list optimization alone (pull191).

@domenic
Copy link
Collaborator

domenic commented Feb 4, 2013

Maybe test them against this version as well? I believe we were planning on replacing the current one with that, but your idea is intriguing also.

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 4, 2013

@domenic That one will fail on IE 6 and 7. It does not support try finally without catch.

@domenic
Copy link
Collaborator

domenic commented Feb 4, 2013

sigh. In that case, @kriskowal, thoughts?

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 5, 2013

I would like to point out that while this approach do minimize latency of "ticks" in case of none thrown exceptions, it also can increase latency of tasks after a thrown exception. In case we wont to be less "optimistic", we can preemptively request n ticks, where n <= m and m is the number of tasks.
With my last updates, I adopt an "rather-optimistic" approach, where n <= 2.
This could be further adjusted.

- No need for loop
- Comment grammar
@rkatic
Copy link
Collaborator Author

rkatic commented Feb 7, 2013

With the last change, I think, I resolved the problem of the introduced latency on multiple thrown exceptions. 🎱 (is this the proper use of 8ball?)

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 7, 2013

Ignore my last comment. Unfortunately the only way to completely eliminate the increased latency after thrown exceptions, is to make an tick request per task ( n >= m ). I created a new branch with such "pessimistic" approach. Implementation is even simpler (https://github.com/rkatic/q/compare/more-nextTick), with no apparent drop in performances (http://jsperf.com/wqgrecrereffrre/7).
Now, if we can expect tasks to throw exceptions only occasionally (mainly during debugging), then the current "optimistic" approach should be probably fine, otherwise, I don't see why we shouldn't adopt the "pessimistic" one.

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 9, 2013

After some given thoughts, I come to the conclusion that the assumption, that uncaught exceptions will be rare in production, is too dangerous. It's not hard to imagine a progress listener start throwing exceptions with high frequency, considerably delaying resolutions of many promises.
However, the "pessimistic" approach, although with still good performances, would produce mostly wasted postMessage/setTimeout calls.
Therefore, I made a change that "amortizes" both, tick requests, and latency after thrown exceptions.

- pending -> queuedTasks
- ticking -> pendingTicks
- maxTicking -> maxPendingTicks
- cycle -> usedTicks
- n -> expectedTicks
@domenic
Copy link
Collaborator

domenic commented Feb 12, 2013

@rkatic a very nice benchmark has been put together over in #206 by @francoisfrisch. But, you've clearly added a lot of thought about the "reticking" process. Care to give us an explanation of where it currently stands, how much it optimizes for pessimism vs. optimism, etc.? Could it be made faster by being more optimistic, for example?

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 12, 2013

@domenic The problem that a naive "tick-reusage" solution has, is that in cases of thrown exceptions, subsequent tick is requested much later and sequentially. This is a problem, because tasks are queued, and all subsequent tasks after thrown exceptions will wait for the newly requested tick. My solution amortizes such costs.
Take for example this test, where on 100 tasks, every second throws. While the "naive" solution with the "catch re-tick" approach will sequentially request 50 ticks, producing an O(t n) delay, the amortized solution produces an O(t log n) delay.
I hope I was clear enough, beside my bad-ish English. Please, feel free to ask for more clarifications.

t: tick delay (~3ms on Firefox, 0-1ms on Chrome, ...)
n: number of uncaught exceptions

@kriskowal
Copy link
Owner

I think I’ll buy it. We can adjust as necessary, but it looks good to me.

kriskowal added a commit that referenced this pull request Feb 13, 2013
@kriskowal kriskowal merged commit 5f0471d into kriskowal:master Feb 13, 2013
@kriskowal
Copy link
Owner

Landed. Really wanted this for v0.9.

@francoisfrisch
Copy link

Thanks @rkatic !

@rkatic
Copy link
Collaborator Author

rkatic commented Feb 15, 2013

I'm glad I could help, however, I would like to point out that the amortization algorithm is mostly relevant when the tick delay is significant, and that in future, if the usage of setTimeout will be removed, the simpler "catch re-tick" solution could be probably adopted.

I am actually wondering if we could avoid the setTimeout fallback even now, and if we can not, could we at least use the image.onerror technique in not IE browsers.

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

Successfully merging this pull request may close these issues.

4 participants