feat(async): adds AsyncScheduler and updates appropriate operators.

[trxcllnt]

Adds Scheduler.async which will always schedule an action to happen in the future. Fixes #1339. Updates FutureAction to use setInterval instead of repeated setTimeout calls as discussed with @mattpodwysocki in #1391.

[mattpodwysocki]

@trxcllnt why do we not have a separate schedule method like schedulePeriodic for when setInterval is necessary? Let's chat about this before merging because I want to bring in long running scheduling which avoids recursive scheduling which is what I'm retrofitting into v4...

[trxcllnt]

@mattpodwysocki well, mostly because early on we knew we wanted a smaller scheduling API and that recursive scheduling could accomplish all scheduling variants. replacing the state and reusing the scheduled action is effectively tail-call optimization.

[trxcllnt]

@mattpodwysocki I also ran some rudimentary tests in jsperf that show using setInterval is faster than setTimeout (in my Chrome), even if we only use it to run something once and dispose. So, as far as I can tell, there's no downside to using setInterval all the time.

[benlesh]

@trxcllnt why do we not have a separate schedule method like schedulePeriodic for when setInterval is necessary?

I suggested the same thing in Slack.

there's no downside to using setInterval all the time.

... except it's weird and confusing to look at in the code.

[benlesh]

I'm really against breaking this API, @trxcllnt. It would be a breaking change.

Before Scheduler.schedule(foo, 10) would have caused foo to execute 1 time at 10 milliseconds (or virtual frames, or whatever you're scheduling by). With this PR it will repeat every 10ms.

It seems like what we really want is schedulePeriodic or scheduleInterval which would be more idiomatic for JS folks.

The other thing is the need to schedule things on a repeating basis is really an edge case for schedulers. Most of the time schedulers are scheduling one thing and they're done.

It is interesting that setInterval is faster in Chrome than setTimeout. But given that this is something that's happening on a delay anyhow, I'm not sure the perf win is going to be noticeable enough in any meaningful way to justify the breaking change.

[benlesh]

even a Scheduler.schedule(fn: Function, delay: number = 0, state: any = null, repeat: boolean = false) API would be better IMO. Just as long as it doesn't break the current behavior.

... but I like scheduleInterval a little better.

[trxcllnt]

@Blesh Before Scheduler.schedule(foo, 10) would have caused foo to execute 1 time at 10 milliseconds (or virtual frames, or whatever you're scheduling by). With this PR it will repeat every 10ms.

That's not true. The behavior of schedule is exactly the same in the non-repeating case, and more idiomatic in the repeating case (see here and here).

[trxcllnt]

@Blesh

@trxcllnt why do we not have a separate schedule method like schedulePeriodic for when setInterval is necessary?

I suggested the same thing in Slack.

In the beginning, you and I set a goal of simplifying and optimizing the schedulers. We acknowledged that recursive scheduling was the most general case that would enable all the existing scheduling strategies, while being simple enough to fully optimize. What's the point of making the API more complex than it has to be?

there's no downside to using setInterval all the time.

... except it's weird and confusing to look at in the code.

Does anybody really look at the scheduler internals much? Optimization ain't always pretty.

[Guardiannw]

@trxcllnt Thank you for your PR, but I do think that the reason it is breaking in Angular is due to the recursive scheduling. I am not asking any of you to fix it on your end, but if you could that would be great.
I think that setInterval should definitely be utilized, but I agree with @Blesh in that it isn't really the best design choice to hack it in order to produce the same effect as setTimeout. What would be the feasibility of doing a scheduleInterval?

[trxcllnt]

@Guardiannw recursive in this context doesn't refer to literal recursion, but is a metaphor for the API that allows scheduled actions to reschedule themselves. Past versions of Rx did use recursion, which wasn't TCO, so we've optimized in the latest version. This is not the source of your memory leak.

[Guardiannw]

@trxcllnt I think I knew that already, but I am pretty sure that zone.js keeps track of that. That zone.js actually monitors a stack trace through what set the timeout and then where it went and then what set it and then where it went and then...... so on and so forth. Thereby creating an infinitely long stack trace full of leaked memory as the interval goes on and on.

[trxcllnt]

@Guardiannw then it sounds like moving from repeated setTimeout calls to a single setInterval call would solve your problem, no?

[Guardiannw]

@trxcllnt Yes I think it would. I just think that it would be best not to use setInterval on occasions where setTimeout is desired. But in the case of an IntervalObservable, I think it would be the most appropriate solution.

[Guardiannw]

@trxcllnt Yes, I very much believe that that is what is causing the problem.

[trxcllnt]

@Guardiannw I just think that it would be best not to use setInterval on occasions where setTimeout is desired

If there were any meaningful difference between canceling setInterval after it executes once and just calling setTimeout, including loss of performance, then I'd agree. We actually see the opposite here. And in fact, given the runtime scheduling differences between setTimeout and setInterval, I think a case can be made that we should always use setInterval, especially for single-run actions, because the runtime attempts to execute actions scheduled via setInterval more precisely.

This PR fixes multiple outstanding issues (including yours, it sounds like), is faster, doesn't contain any breaking changes, and doesn't increase API surface area. I don't know what more we could ask for in a PR.

[Guardiannw]

Hey I'm open to hearing from guys much more intelligent than me.

[trxcllnt]

@Guardiannw feel free to clone this branch and see whether the memory leak is still there or not.

[Guardiannw]

@trxcllnt I cloned and ran tests on your branch and your PR does indeed fix my other memory leak issue. I had to add the fix #1394 as well in order to be sure no memory leaks exist anymore. After I did both your branch and that fix, things are working like a dream. No memory leaks... @Blesh @mattpodwysocki Please move forward where you all want to, but please somehow incorporate this fix in some way so that we have this bug fixed.

Thank you all,

[trxcllnt]

@Blesh @mattpodwysocki as far as I can tell, we have two options here:

1. Modify the current scheduler API (either by adding a new schedulePeriodic method, or adding a flag to the current schedule method) to distinguish between actions scheduled once vs. actions scheduled periodically
   • Pros:
     • more explicit public API
     • distinguishes between scheduling actions with setTimeout and setInterval, which may be necessary if there are differences between the time at which a runtime invokes identically scheduled setTimeout and setInterval callbacks under timer pressure
   • Cons:
     • breaking change (major semver rev)
     • find new solution for optimizing periodic scheduling with state
     • adds complexity to an API new users already have a difficult time understanding
     • changes everywhere we're currently scheduling an action that runs more than once
     • could reintroduce @Guardiannw's memory leak if a schedule-one action is scheduled repeatedly
     • introduces an unnecessary code path if a runtime doesn't distinguish between identically scheduled setTimeout and setInterval callbacks under timer pressure
2. Maintain the current scheduling API (what this PR does), but apply internal optimization that uses setInterval instead of setTimeout to achieve more idiomatic periodic scheduling, essentially assuming an action will re-schedule itself with the same due time, and canceling if it doesn't (or the due time changes)
   • Pros:
     • no breaking change
     • the scheduled action determines whether it's re-scheduled or not
     • one optimized code path for scheduling whether executes once vs. periodic
   • Cons:
     • using setInterval and canceling after the first execution may not be equivalent to using setTimeout under timer pressure
     • difficult to tell that setInterval will be canceled after each execution if the scheduled action doesn't call this.schedule without carefully reading the source of FutureAction

I don't care which one we choose, but we've gotta choose one. I chose option 2 on the basis that it was less work, but if someone (not me) wants to take on the work for option 1, be my guest.

[benlesh]

breaking change (major semver rev)

It's not a breaking change. It's adding a new method to the type, and not at all changing the other method.

Unless I'm mistaken, what you're proposing is a breaking change.

Currently:

Scheduler.schedule(() => console.log('hi'), 1000) logs "hi" after 1000ms.

After the proposed change:

Scheduler.schedule(() => console.log('hi'), 1000) logs "hi" on an interval every 1000ms until something disposes of the returned subscription. That would be a "breaking change", for sure.

... either way, in a -beta. revision, we're not bumping up semver a major revision for a change like this.

[benlesh]

Also, this PR is doing way too much.

1. Adds a new scheduler
2. Sets it as the default to multiple operators
3. Changes existing schedulers to set up intervals rather than timeout.

... all in one commit, and without much discussion by the community.

I'm not trying to be adversarial, @trxcllnt, but I don't think I agree with the PR, and it's even harder for me to agree with it when it's doing so much.

[trxcllnt]

@Blesh I'm sorry, this PR does not automatically re-execute the action unless it's disposed (added an inline comment showing where the interval is canceled if the action isn't rescheduled).

The API change would be a breaking change because it would break actions that reschedule themselves recursively (i.e. remove the ability for a scheduled action to call this.schedule() inside the action callback itself) since a user would now have to call schedulePeriodic when the action is first scheduled, instead of the current way of calling schedule and rescheduling inside the action if it needs to continue.

[benlesh]

Okay... after reviewing this PR and discussing with Paul in Slack, I think what we've got here is fine.

I misunderstood the setInterval bit. It's a perf optimization for rescheduling, but doesn't change the behavior of the schedule method. It's just that if the action re-schedules, it will continue the interval. I'd like to keep the current API that allows the action to schedule itself again, because that opens the door for more dynamic scheduling down the road (for things like exponential stepback etc), where a schedulePeriodic would limit the API to only being able to schedule steady intervals.

The AsyncScheduler makes total sense, and I think we needed that. I'm dubious on the name, but it's fine for now I think, and "sounds" like it comes after asap, IMO.

The other changes to default some operators to use the async scheduler seem totally fine.

[benlesh]

Also, I LOVE the fact that @trxcllnt commented the hell out of FutureAction to make sure the implementation is clear to those hopping into it. The setInterval bit is confusing at first blush without the comments.

[benlesh]

LGTM

[luisgabriel]

Kudos for the comments! 👏

We should do it more. ;)

[benlesh]

@trxcllnt this looks good, but I think I'm going to block it while #1364 is merged. Then the tests will need to be updated to TypeScript. Cool?

[trxcllnt]

@Blesh sure. do you need me to rebase and resolve merge conflicts?

[benlesh]

@trxcllnt: If you'd be so kind, yes.

[trxcllnt]

@Blesh done

[Guardiannw]

@Blesh could we merge this PR now?

[benlesh]

LGTM

[lock]

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.