fix(schedulers): fix asap and animationFrame schedulers to execute across their respective async boundaries #1820

trxcllnt · 2016-07-11T02:46:26Z

Description:

The AsapScheduler and AnimationFrameScheduler were totally busted. My bad. Now they execute their scheduled actions in batches. If actions reschedule while executing a batch, a new frame is requested for the rescheduled action to execute in.

Other updates included in this PR:

simplifies the public Scheduler and Action APIs. Implementation details like the actions queue and active boolean are now on the concrete implementations, so it's easier for people to implement the Scheduler API.
renames FutureAction -> AsyncAction to conform to the same naming convention as the rest of the Action types.
Actions should be able to be recursively rescheduled with different delay times and switch between their delay = 0 and delay > 0 behaviors. For example, a delay=0 QueueAction should be able to reschedule with delay > 0 and fall back to its parent class's (AsyncAction) behavior.
If a scheduled action throws an error, the schedulers now dispose (and un-reference) the rest of the scheduled actions before re-throwing. I'm not sure how necessary this is, but it did seem like keeping the actions in the queue could was incorrect (they'd stick around until the Scheduler was flushed, at which time they'd be executed) or possibly leaky.

Related issue (if exists):

Fixes #1814

Performance results:

Re-ran a few of the performance tests to check regressions when using the queue scheduler. No discernible perf hits.

Current master:

	RxJS 4.1.0	RxJS 5.0.0-beta.10	factor	% improved
concat - immediate	50,291 (±1.21%)	357,051 (±2.00%)	7.10x	610.0%
merge - immediate	5,621 (±1.53%)	56,834 (±1.00%)	10.11x	911.1%
concat	21,527 (±1.74%)	153,550 (±4.04%)	7.13x	613.3%
merge	2,283 (±2.35%)	21,295 (±2.45%)	9.33x	832.8%
from-with-array - immediate	127,203 (±1.81%)	895,727 (±1.10%)	7.04x	604.2%
from-with-iterable - immediate	105,449 (±2.19%)	357,870 (±3.85%)	3.39x	239.4%
from-with-string - immediate	60,665 (±4.35%)	384,754 (±3.69%)	6.34x	534.2%
of - immediate	59,050 (±3.56%)	829,434 (±3.29%)	14.05x	1,304.6%
from-with-array	54,174 (±3.47%)	668,259 (±3.17%)	12.34x	1,133.6%
from-with-iterable	51,085 (±3.41%)	366,127 (±2.51%)	7.17x	616.7%
from-with-string	33,901 (±2.78%)	365,895 (±3.86%)	10.79x	979.3%
of	60,350 (±3.36%)	443,226 (±2.38%)	7.34x	634.4%

This PR:

	RxJS 4.1.0	RxJS 5.0.0-beta.10	factor	% improved
concat - immediate	54,293 (±1.61%)	386,044 (±1.16%)	7.11x	611.0%
merge - immediate	5,751 (±1.11%)	57,951 (±0.86%)	10.08x	907.6%
concat	22,460 (±1.07%)	169,922 (±1.51%)	7.57x	656.5%
merge	2,415 (±1.21%)	22,674 (±1.01%)	9.39x	838.7%
from-with-array - immediate	128,302 (±1.54%)	851,673 (±1.70%)	6.64x	563.8%
from-with-iterable - immediate	114,602 (±1.34%)	364,576 (±1.75%)	3.18x	218.1%
from-with-string - immediate	60,711 (±4.18%)	367,734 (±3.85%)	6.06x	505.7%
of - immediate	58,137 (±3.61%)	853,370 (±3.29%)	14.68x	1,367.9%
from-with-array	51,498 (±3.14%)	691,779 (±3.70%)	13.43x	1,243.3%
from-with-iterable	51,048 (±3.52%)	372,203 (±3.40%)	7.29x	629.1%
from-with-string	33,374 (±3.06%)	394,613 (±2.56%)	11.82x	1,082.4%
of	53,013 (±4.33%)	395,391 (±2.94%)	7.46x	645.8%

Side-by-side:

	RxJS 5.0.0-beta.10 (master)	RxJS 5.0.0-beta.10 (PR)
concat - immediate	357,051 (±2.00%)	386,044 (±1.16%)
merge - immediate	56,834 (±1.00%)	57,951 (±0.86%)
concat	153,550 (±4.04%)	169,922 (±1.51%)
merge	21,295 (±2.45%)	22,674 (±1.01%)
from-with-array - immediate	895,727 (±1.10%)	851,673 (±1.70%)
from-with-iterable - immediate	357,870 (±3.85%)	364,576 (±1.75%)
from-with-string - immediate	384,754 (±3.69%)	367,734 (±3.85%)
of - immediate	829,434 (±3.29%)	853,370 (±3.29%)
from-with-array	668,259 (±3.17%)	691,779 (±3.70%)
from-with-iterable	366,127 (±2.51%)	372,203 (±3.40%)
from-with-string	365,895 (±3.86%)	394,613 (±2.56%)
of	443,226 (±2.38%)	395,391 (±2.94%)

coveralls · 2016-07-11T02:53:41Z

Coverage decreased (-0.1%) to 96.342% when pulling 61a6e83 on trxcllnt:scheduler-action-fixes into 19f4362 on ReactiveX:master.

coveralls · 2016-07-11T04:11:43Z

Coverage increased (+0.2%) to 96.689% when pulling 62aa0d2 on trxcllnt:scheduler-action-fixes into 19f4362 on ReactiveX:master.

staltz · 2016-07-11T07:22:10Z

src/Scheduler.ts

+ * ```
+ *
+ * @class Scheduler
+ */


For anyone else reviewing this PR: JSDoc in this file looks correct and should render correctly on the site. 👍

benlesh · 2016-07-11T16:58:07Z

LGTM (as long as we correct the one docs issue @staltz pointed out)

trxcllnt · 2016-07-11T17:11:37Z

@Blesh what's the issue?

tetsuharuohzeki · 2016-07-12T13:08:56Z

src/Rx.ts

@@ -184,6 +184,8 @@ import observable from 'symbol-observable';
 * asynchronous conversions.
 * @property {Scheduler} async Schedules work with `setInterval`. Use this for
 * time-based operations.
+ * @property {Scheduler} animation Schedules work with `requestAnimationFrame`.


should this animation be animationFrame?

Oops, yup @trxcllnt this needs fixed.

@Blesh ok, I'll update. Though I was thinking Scheduler.animation vs. Scheduler.animationFrame would be a nice alias.

benlesh · 2016-07-12T19:22:12Z

Looks like this needs a rebase, @trxcllnt I'd like to get it merged.

trxcllnt · 2016-07-12T20:42:05Z

@Blesh rebased

coveralls · 2016-07-12T20:49:47Z

Coverage increased (+0.2%) to 96.689% when pulling 1bd2902 on trxcllnt:scheduler-action-fixes into 4f65b03 on ReactiveX:master.

tetsuharuohzeki · 2016-07-13T06:58:57Z

src/scheduler/Action.ts

+   * according to the `delay` parameter, if specified.
+   * @param {any} [state] Some contextual data that the `work` function uses when
+   * called by the Scheduler.


Is this {any} jsdoc correct? Should it be {T} with @template T?

@saneyuki I think you're right. I'll update the PR.

…ross async boundaries. The AsapScheduler and AnimationFrameSchedulers were totally busted. My bad. Now they execute their scheduled actions in batches. If actions reschedule while executing a batch, a new frame is requested for the rescheduled action to execute in. This PR also simplifies the public `Scheduler` and `Action` APIs. Implementation details like the `actions` queue and `active` boolean are now on the concrete implementations, so it's easier for people to implement the Scheduler API. This PR also renames `FutureAction` -> `AsyncAction` to conform to the same naming convention as the rest of the Action types. Fixes ReactiveX#1814

coveralls · 2016-07-13T19:49:19Z

Coverage decreased (-0.1%) to 96.342% when pulling 61a6e83 on trxcllnt:scheduler-action-fixes into 19f4362 on ReactiveX:master.

lock · 2018-06-06T21:42:21Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

trxcllnt force-pushed the scheduler-action-fixes branch from b934493 to 61a6e83 Compare July 11, 2016 02:47

trxcllnt force-pushed the scheduler-action-fixes branch from 61a6e83 to 62aa0d2 Compare July 11, 2016 04:05

staltz reviewed Jul 11, 2016
View reviewed changes

benlesh added the PR: needs another LGTM label Jul 11, 2016

tetsuharuohzeki reviewed Jul 12, 2016
View reviewed changes

benlesh added PR: needs rebase and removed PR: needs another LGTM labels Jul 12, 2016

trxcllnt force-pushed the scheduler-action-fixes branch from 62aa0d2 to 1bd2902 Compare July 12, 2016 20:41

tetsuharuohzeki reviewed Jul 13, 2016
View reviewed changes

trxcllnt force-pushed the scheduler-action-fixes branch from 1bd2902 to 8968293 Compare July 13, 2016 19:40

trxcllnt removed the PR: needs rebase label Jul 13, 2016

benlesh merged commit 548ec2a into ReactiveX:master Jul 16, 2016

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(schedulers): fix asap and animationFrame schedulers to execute across their respective async boundaries #1820

fix(schedulers): fix asap and animationFrame schedulers to execute across their respective async boundaries #1820

trxcllnt commented Jul 11, 2016

coveralls commented Jul 11, 2016

coveralls commented Jul 11, 2016

staltz Jul 11, 2016

benlesh commented Jul 11, 2016 •

edited

Loading

trxcllnt commented Jul 11, 2016

tetsuharuohzeki Jul 12, 2016

benlesh Jul 12, 2016

trxcllnt Jul 13, 2016

benlesh commented Jul 12, 2016

trxcllnt commented Jul 12, 2016

coveralls commented Jul 12, 2016

tetsuharuohzeki Jul 13, 2016

trxcllnt Jul 13, 2016

coveralls commented Jul 13, 2016 •

edited

Loading

lock bot commented Jun 6, 2018

fix(schedulers): fix asap and animationFrame schedulers to execute across their respective async boundaries #1820

fix(schedulers): fix asap and animationFrame schedulers to execute across their respective async boundaries #1820

Conversation

trxcllnt commented Jul 11, 2016

coveralls commented Jul 11, 2016

coveralls commented Jul 11, 2016

staltz Jul 11, 2016

Choose a reason for hiding this comment

benlesh commented Jul 11, 2016 • edited Loading

trxcllnt commented Jul 11, 2016

tetsuharuohzeki Jul 12, 2016

Choose a reason for hiding this comment

benlesh Jul 12, 2016

Choose a reason for hiding this comment

trxcllnt Jul 13, 2016

Choose a reason for hiding this comment

benlesh commented Jul 12, 2016

trxcllnt commented Jul 12, 2016

coveralls commented Jul 12, 2016

tetsuharuohzeki Jul 13, 2016

Choose a reason for hiding this comment

trxcllnt Jul 13, 2016

Choose a reason for hiding this comment

coveralls commented Jul 13, 2016 • edited Loading

lock bot commented Jun 6, 2018

benlesh commented Jul 11, 2016 •

edited

Loading

coveralls commented Jul 13, 2016 •

edited

Loading