Skip to content

Conversation

@akarnokd
Copy link
Collaborator

While looking at the Scrabble benchmark for hints of likely unnecessary allocations, I've come across Return which by default uses ImmediateScheduler to emit its single item. This scheduler simply executes the action it gets immediately but due to the standard IScheduler API, it has to provide an "inner" recursive scheduler to the Func. With Return, there won't be any further calls to the IScheduler API so this AsyncLockScheduler allocated is wasted.

I'm proposing a shortcut in the form of the IOneTimeScheduler interface that doesn't allow recursive scheduling and this PR shows a way to implement it inside ImmediateScheduler and the use of it in Return. The API is a simple as ScheduleDirect(TState, Action<TState>); it uses Action because returning an IDisposable makes not much sense - there is no task to follow up and keep referencing after the one-time action has finished.

Of course, Return could be simply have a special case when its scheduler is the ImmediateScheduler and emit directly.

@danielcweber
Copy link
Collaborator

So what we're essentially looking for is to bake some fire-and-forget-and-never-schedule-recursively mechanism into the schedulers, like it is now mimicked by the ScheduleAction-extension?

@akarnokd
Copy link
Collaborator Author

akarnokd commented Jun 18, 2018

Yes, but not only for the client, but for the service provider so it doesn't have to waste resources on a unneeded feature.

@danielcweber
Copy link
Collaborator

I'm thinking about how to bring this together with the already existing extension methods for non-recursive scheduling. Ideally, if default interface implementations were a thing in C# today, I guess we would use them here, executing what the extension does today.

I'm also thinking about somehow smarting up the existing extension so that it may detect the ability of one-time-scheduling.

And then, would renaming the one-time-methods to simple Schedule introduce ambiguities ?

@akarnokd
Copy link
Collaborator Author

This PR is about providing a different API when the operator is known to issue exactly one schedule on a standard scheduler and not conveniencing a call over the regular schedule call that has an IScheduler still provided but suppressed in those helper methods you contributed.

@danielcweber
Copy link
Collaborator

So just to understand it right, the operators that would take advantage of a one time scheduler would, in their API, still take an IScheduler and then type check it?

@akarnokd
Copy link
Collaborator Author

The operators would still take their usual IScheduler, but they would feature detect the direct schedule ability. That requires support and specialized implementation on the schedulers that want to support it.

@danielcweber
Copy link
Collaborator

So it could be centralized into the existing extension which could do the feature detection?

@danielcweber
Copy link
Collaborator

Like e.g. the Count() extension of Linq checks the enumerable if it implements ICollection and then just returns the Count property.

@akarnokd
Copy link
Collaborator Author

That's a matter of convenience, like having this part of ScheduleAction itself, but otherwise, the fundamental structural changes remain the same.

@danielcweber
Copy link
Collaborator

danielcweber commented Jun 27, 2018

Convenient is good, so the improvement can be leveraged by anybody who maybe doesn't know about IOneTimeSchedulers.

Just one more thing, the name ScheduleDirect misleads me since it suggests something temporal that happens directly when there is also a timed ScheduleDirect. ScheduleNonRecursively, ScheduleAction, Schedule ?

@akarnokd
Copy link
Collaborator Author

Convenient is good, so the improvement can be leveraged by anybody who maybe doesn't know about IOneTimeSchedulers.

Assuming the ScheduleAction is semantically equivalent of this ScheduleDirect call for all previously existing call sites of ScheduleAction. Also there is the overhead of the is check in general and on those IScheduler instances, that are actually part of a scheduler's recursive part (i.e., those (scheduler, state) => call(scheduler, state) which themselves recurse via ScheduleAction.

@danielcweber
Copy link
Collaborator

At least for the ImmediateScheduler, the semantic equivalency is obvious. I leave it up to you whether to push it into the extension, IMO the overhead of the is check is small compared to the simplicity we get for future contributors.

Maybe we can still align the namings, there are now 3 terms around that essentially mean the same (ScheduleAction, ...OneTime, ...Direct). I can live with either.

@akarnokd
Copy link
Collaborator Author

If cast to IOneTimeScheduler, ScheduleAction should be unambiguous, assuming that IOneTimeScheduler won't implement IScheduler so that the ScheduleAction extension method usage does not conflict with it. I'll update the PR and rename the interface methods to ScheduleAction

I'd like to keep it scoped to Return for the moment as each other operator requires individual evaluation. Also the interface is internal to Rx thus I don't expect external conflicts with pre-existing 3rd party schedulers.

@danielcweber
Copy link
Collaborator

Ambiguity should be no problem, interface methods are closer than extension methods and will always be preferred.

@danielcweber
Copy link
Collaborator

So...does the 'proposal' in the PR-text suggest this is not ready to merge, or is this good to go?

@akarnokd
Copy link
Collaborator Author

The PR is operational, it just needs a decision about going into this direction.

@danielcweber
Copy link
Collaborator

I'm all for it, I only feel it should be the 'pit of success', that is, the improvement should be there for anybody who doesn't want to deal with the different concepts of schedulers. Thus my proposal to put it in the extension.

We could, for a first step, try to make all non-recursive schedulings in the library explicit, by using ScheduleAction to get an overview of what's to be gained.

@danielcweber
Copy link
Collaborator

How should this advance?

@akarnokd
Copy link
Collaborator Author

akarnokd commented Jul 4, 2018

I don't know. I certainly don't want to affect any operator that happens to use ScheduleAction but its default scheduler does not actually support this one-shot behavior. Maybe it's better to special case Return by simply having an immediate-executing, compile-time decided variant.

@danielcweber
Copy link
Collaborator

What could happen if the changes would go into ScheduleAction ? I guess the semantics of a non-recursive schedule are very clear, behaviour should not be affected. I can't see how frequently it would be the ImmediateScheduler that would be used for ScheduleAction, so it's unclear how much is to gain.

@akarnokd
Copy link
Collaborator Author

akarnokd commented Jul 4, 2018

Currently, only a few would benefit:

  • Return
  • Throw
  • Prepend (one value)
  • Append (one value)

other uses either recurse in a different fashion or work with a scheduler that presents itself as the recursive scheduler.

@akarnokd
Copy link
Collaborator Author

akarnokd commented Jul 4, 2018

I'll post a replacement PR where I only modify the operators above and measure the effects via benchmarks.

@akarnokd akarnokd closed this Jul 4, 2018
@akarnokd akarnokd deleted the OneShotScheduler branch July 4, 2018 10:54
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.

2 participants