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

Throttling individual bindings #114

Closed
TysonMN opened this issue Sep 12, 2019 · 26 comments
Closed

Throttling individual bindings #114

TysonMN opened this issue Sep 12, 2019 · 26 comments

Comments

@TysonMN
Copy link
Member

TysonMN commented Sep 12, 2019

I want to throttle an individual binding. I started the discussion in elmish/elmish#189 (comment). I will briefly summarize the relevant parts.

My definition of throttling comes from this SO answer. That answer contrasts throttling with debouncing. Both of those concepts involve swallowing/dropping some amount of data. A related idea could be called limiting (as in rate limiting), which limits the rate at which data is allowed to flow by but doesn't drop any data. So although I am primarily motivated by throttling, debouncing and limiting are so similar that we can probably make all of these possible in one go.

My specific use case is a Slider with discrete interval of length 8000. As the user drags the thumb (as it is called) of the slider, each integral value encountered dispatches a message. My update function is trivial: it just saves the new value of the slider in the model. It is not computationally expensive. The part that is computationally expensive is update being called ~4000 in a single flick of the user's wrist.

@cmeeren (as stated in elmish/elmish#189 (comment)) and I like this idea. I am currenting having fun struggling with the details to make it happen :)

@TysonMN
Copy link
Member Author

TysonMN commented Sep 12, 2019

Quoting @cmeeren from elmish/elmish#189 (comment):

From the top of my head, that would require extending two-way and Cmd bindings with a new optional modifyDispatch parameter with signature Dispatch<'msg> -> Dispatch<'msg>.

Here is some very not-off-the-top-of-my-head analysis ;)

I have looked at all the ways that dispatch is called in ViewModel.fs. Here is what I found.

  • Of course OneWay, OneWayLazy, and OneWaySeqLazy never reference dispatch
  • TwoWay, TwoWayValidate, and SubModelSelectedItem call dispatch within TrySetMember
  • Cmd and CmdParam may or may not call dispatch when executed
  • SubModel and SubModelSeq never call dispatch but they pass it to sub-view models
  • SubModelWin passes dispatch into the view model it stores in its mutable window state and may or may not call dispatch when the window is closed

@cmeeren
Copy link
Member

cmeeren commented Sep 13, 2019

What I imagine, partly from the top of my head, is this:

Bindings.fs:

and internal TwoWayData<'model, 'msg> = {
  Get: 'model -> obj
  Set: obj -> 'model -> 'msg
+ ModifyDispatch: Dispatch<'msg> -> Dispatch<'msg>
}
  static member twoWay
      (get: 'model -> 'a,
-      set: 'a -> 'model -> 'msg)
+      set: 'a -> 'model -> 'msg,
+      ?modifyDispatch: Dispatch<'msg> -> Dispatch<'msg>)
      : string -> Binding<'model, 'msg> =
    TwoWayData {
      Get = get >> box
      Set = unbox<'a> >> set
+     ModifyDispatch = defaultArg modifyDispatch id
    } |> createBinding

And corresponding changes in ViewModel: Adding dispatch: Dispatch<'msg> to the relevant VmBinding cases, setting it to ModifyDispatch dispatch when initializing the bindings, and calling the binding-specific dispatch instead of the VM-global dispatch for those bindings.

Note that there may be better names than modifyDispatch. I'm open to suggestions.

Also note that this is not related to throttling or debouncing per se; it's a general abstraction that allows any modification of dispatch.

It would also be nice to have a sample demonstrating this new feature (such as a slider with 8000 steps, as you say).

@TysonMN
Copy link
Member Author

TysonMN commented Sep 13, 2019

What I imagine, partly from the top of my head, is this:

Yep, I think that works too.

Note that there may be better names than modifyDispatch. I'm open to suggestions.

Your name is reasonable. My working name has been dispatchDecorator since this "pattern" is the functional equivalent of the OOP decorator pattern. I think my name is worse because it is longer and is an OOP-inspired name. I think the type signature suggests the name replaceDispatch, but I think this precise syntactic name doesn't convey the intended semantic meaning. A name that I think does convey the intended semantic meaning is wrapDispatch, which is similar to your name but shorter. I also think "wrap" is a more precise description of the situation than "modify".

Also note that this is not related to throttling or debouncing per se; it's a general abstraction that allows any modification of dispatch.

Indeed. And I think this is the right place to start. I like API designs that both provide simple interfaces for the common cases (throttling, debouncing, etc.) while also not limiting what can be done. Right now, we are working on ensuring that anything is possible. Sometime later, we can add the simple interfaces for the common cases.

It would also be nice to have a sample demonstrating this new feature (such as a slider with 8000 steps, as you say).

Definitely. I can add that.

and internal TwoWayData<'model, 'msg> = {

To what cases do you think we should expose the possibility to wrap dispatch? In elmish/elmish#189 (comment), you suggested (off the top of your head)

  • TwoWay,
  • TwoWayValidate,
  • Cmd, and
  • CmdParam.

Do you still want to extend only those four even after reading my summary of my analysis? (I am fine with that...just want to double check.)

@cmeeren
Copy link
Member

cmeeren commented Sep 13, 2019

No, we should definitely extend all bindings that can dispatch.

(Note that this does not include any sub-model bindings. The wrappers for those will be in the sub-model bindings themselves.)

@cmeeren
Copy link
Member

cmeeren commented Sep 13, 2019

Also, wrapDispatch is nice. 👍

@TysonMN
Copy link
Member Author

TysonMN commented Sep 15, 2019

Note that this does not include any sub-model bindings. The wrappers for those will be in the sub-model bindings themselves.

Clearly that applies to SubModel and SubModelSeq. What about SubModelWinData? It may or may not dispatch a message when the window is closed. Should we allow wrapping just for that case?

@cmeeren
Copy link
Member

cmeeren commented Sep 15, 2019

What about SubModelWinData? It may or may not dispatch a message when the window is closed. Should we allow wrapping just for that case?

I can't think of any immediate use-case for this. I suggest we wait until it's actually requested by users, if ever.

cmeeren added a commit that referenced this issue Sep 15, 2019
@TysonMN
Copy link
Member Author

TysonMN commented Sep 15, 2019

Quoting part of #118 (comment).

let's discuss the "future PR" on making it easy for users in #114 or another issue first to get design decisions down first. For example, since RX combinators like throttling/debouncing etc. are inherently composable (e.g. throttle 250 >> distinctUntilChanged), I'm thinking it makes more sense to define such combinators than to add e.g. optional throttleMs and debounceMs parameters to all relevant overloads. (Based on your work so far, I'm sure you agree - or disagree because I am missing something significant.) And depending on how Elmish.WPF-specific these combinators end up, it may be better to implement them at the Elmish level or in a separate library (or is that already done?).

We are definitely on the same page. I don't want to add any more optional parameters related to wrapping dispatch. To aid in helping users discover what is possible, I was thinking we could add something to the documentation of the argument wrapDispatch in each overload.

cmeeren added a commit that referenced this issue Sep 15, 2019
@TysonMN
Copy link
Member Author

TysonMN commented Sep 17, 2019

I tried to use Rx to throttle the slider binding, but I ran into a deadlock. Before I get into that, I would like to consider a simpler case that also had unexpected behavior.

For a slider, I expect the following to be equivalent:

  1. OneWay binding and
  2. TwoWay binding with dispatch replaced by ignore.

However, the case with TwoWay binding doesn't allow the slider to move. I made the commit TysonMN@639996b to help see and test the difference.

Why can't the slider move in a TwoWay binding when dispatch is replaced by ignore?

@cmeeren
Copy link
Member

cmeeren commented Sep 17, 2019

I don't know for sure, but I guess it's because WPF reads the value of a two-way binding after setting it, thus resetting it to the current value, whereas in a one-way binding it doesn't. (I don't know for sure that it works that way, but it's my best guess at the moment, and there was a bug in the past that I think perhaps touched on the same, #10.)

@TysonMN
Copy link
Member Author

TysonMN commented Sep 17, 2019

Excellent guess. Yes, I think it is related to that issue.

When using a TwoWay binding and replacing dispatch with ignore, TrySetMember returns true, which causes WPF to call TryGetMember for the same member.

When using a OneWay binding (without any changes in the XAML), TrySetMember returns false because OneWay is read-only, which causes WPF to not call TryGetMember for the same member and to log the following to Diagnostics.Trace:

System.Windows.Data Error: 8 : Cannot save value from target back to source. BindingExpression:Path=StepSize; DataItem='ViewModel`2' (HashCode=65445301); target element is 'Slider' (Name=''); target property is 'Value' (type 'Double') InvalidOperationException:'System.InvalidOperationException: Property path is not valid. 'System.Dynamic.DynamicObject+MetaDynamic' does not have a public property named 'Items'.
   at CallSite.Target(Closure , CallSite , Object , Object )
   at MS.Internal.Data.PropertyPathWorker.SetValue(Object item, Object value)
   at System.Windows.Data.BindingExpression.UpdateSource(Object value)'

This situation is simpler now than it was back when #10 was created because the Elmish dispatch loop now runs on the UI thread #40.

@TysonMN
Copy link
Member Author

TysonMN commented Sep 17, 2019

Ok, now for the real question.

I tried to throttle the messages using Rx in 1562bcb. In particular, the Rx throttling code I wrote is

let throttle (dueTime: TimeSpan) (dispatch: Dispatch<Msg>) : Dispatch<Msg> =
  let subject = new Subject<Msg>()
  let observable = subject :> IObservable<Msg>
  observable.Sample(dueTime).Subscribe(dispatch) |> ignore
  subject.OnNext

The main issue with this is that it deadlocks inside subject.OnNext (rather easily). Just try moving the slider's thumb around quickly for a few seconds.

The second issue is that Rx throttle is actually debounce (which is easier to test using a longer TimeSpan) as "originally" pointed out by our very own @cmeeren! dotnet/reactive#395

Oh, but since dotnet/reactive#395 (comment)

RX.NET calls Debounce Throttle and Throttle Sample

we can just call Sample instead. I forced pushed a commit that calls Sample.

@TysonMN
Copy link
Member Author

TysonMN commented Sep 18, 2019

There is a lock in the Rx code that was causing a deadlock. (It does seem necessary for there to be a lock somewhere.)

Both of the following actions require this lock:

  1. Calling subject.OnNext
  2. Calling the synchronized dispatch after holding onto a particular message for the given length of time

The second action requires the UI thread, so if both are done on the UI thread, then there could be a deadlock (edited to add) if the second actions starts on a different thread. Thus, don't call subject.OnNext on the UI thread. The following code doesn't deadlock.

module Async =
  let ofFunc f a =
    async { return f a }
  
let throttle (dueTime: TimeSpan) (dispatch: Dispatch<Msg>) : Dispatch<Msg> =
  let subject = new Subject<Msg>()
  let observable = subject :> IObservable<Msg>
  observable.Sample(dueTime).Subscribe(dispatch) |> ignore
  subject.OnNext |> Async.ofFunc >> Async.Start

@cmeeren
Copy link
Member

cmeeren commented Sep 18, 2019

Glad you solved it! I don't know about the Rx internals, but if it uses a normal lock, shouldn't several locks from the same thread be fine? Wouldn't the deadlock occur if they are done on different threads and indefinitely have to wait on each other?

@TysonMN
Copy link
Member Author

TysonMN commented Sep 18, 2019

if it uses a normal lock, shouldn't several locks from the same thread be fine?

Correct. That is something that I only learned within the last few months, which is that locks in .NET are reentrant.

Wouldn't the deadlock occur if they are done on different threads and indefinitely have to wait on each other?

Also correct. They do use (at least one) additional thread. I can't find where though because the code seems a bit complicated to me. That thread calls the synchronized dispatch, which forces the UI thread to do the work. So while holding the lock, this additional thread is waiting for

let uiDispatch (innerDispatch: Dispatch<'msg>) : Dispatch<'msg> =
fun msg -> element.Dispatcher.Invoke(fun () -> innerDispatch msg)

while the UI thread is waiting for the lock just after calling subject.OnNext.

@cmeeren
Copy link
Member

cmeeren commented Sep 18, 2019

I see. So it's not trivial to support Rx stuff.

Perhaps we can have overloads taking wrapDispatch: IObservable<'msg> -> IObservable<'msg>? Then we can use an internal helper function like the below to convert it to a dispatch wrapper (adapted off the top of my head from your previous example):

let asDispatchWrapper
    (configure: IObservable<'msg> -> IObservable<'msg>)
    (dispatch: Dispatch<'msg>)
    : Dispatch<'msg> =
  let subject = new Subject<'msg>()
  subject :> IObservable<'msg> |> configure |> Observable.add dispatch
  subject.OnNext |> Async.ofFunc >> Async.Start

This of course requires that the above is the right way to do it every time, and just generally useful to include in Elmish.WPF (I think it might be; there's no external dependencies, and it allows users to easily use Rx combinators. But let's not rush.)

We have to make sure this doesn't leak anything, though (which might be a concern since we never unsubscribe).

@TysonMN
Copy link
Member Author

TysonMN commented Sep 18, 2019

This of course requires that the above is the right way to do it every time, and just generally useful to include in Elmish.WPF (I think it might be; there's no external dependencies...

Your example depends on System.Reactive; the full name of Subject in new Subject<'msg>() is System.Reactive.Subjects.Subject. It seems reasonable to me to expose this kinds of behavior in a NuGet package called "Elmish.Reactive".

Perhaps we can have overloads taking wrapDispatch: IObservable<'msg> -> IObservable<'msg>?

To what method overloads are you referring?

As I envision it, Elmish.Reactive would contain functions that return Dispatch<'msg> -> Dispatch<'msg>. For both simplicity as well as getting the names correct, it could contain simple functions like throttle (which uses Rx's Sample) and debounce (which uses Rx's throttle). It would also contain a more general function like you suggested taking

configureObservable: IObservable<'msg> -> IObservable<'msg>

@cmeeren
Copy link
Member

cmeeren commented Sep 18, 2019

To what method overloads are you referring?

The ones where you recently added wrapDispatch. But it's a moot point if we have to take a dependency on System.Reactive.

In any case, we should consider prior art and the rest of the Elmish ecosystem, too. Elmish.Reactive as you describe it seems specific to Elmish.WPF, since dynamic (i.e. "proper", i.e. mostly everything else) Elmish front-ends won't be able to use it as-is (as discussed in elmish/elmish#189, you'd need global state in the Rx combinators to stabilize the modified dispatch between calls to view, and that's not required in Elmish.WPF due to the peculiarities of how bindings is called differently than view on normal Elmish architectures).

How would you feel about opening a high-level discussion in elmish/elmish so that all Elmish parties can discuss whether it's sensible to have a common implementation? If the requirements turn out too different, we could just make an Elmish.WPF.Reactive package specific to Elmish.WPF, but we should hear what other Elmish folks say first. A joint effort could be a good idea if it's at all feasible.

@cmeeren
Copy link
Member

cmeeren commented Oct 25, 2019

Did we ever solve the issue with the "stuck" slider? I am trying to write a tutorial (#161) section on the wrapDispatch, but using the slider as an example in a custom debounce, the slider doesn't move until the debounce completes. (Furthermore, moving a slider with 10 steps dispatches many, many more messages than there are steps. Edit: Nope, that was just TryGetMember being called, not a message being dispatched. Misread the log.)

You mentioned something about this in #114 (comment).

@TysonMN
Copy link
Member Author

TysonMN commented Oct 26, 2019

I am trying to write a tutorial (#161) section on the wrapDispatch, but using the slider as an example in a custom debounce, the slider doesn't move until the debounce completes.

That is the correct behavior for debounce. You probably meant to throttle. Given in #114 (comment), here is a SO answer that does a great job defining the difference. If you are using Rx, recall the quote in #114 (comment) that quotes dotnet/reactive#395 (comment), which says

RX.NET calls Debounce Throttle and Throttle Sample.


You mentioned something about this in #114 (comment).

You can ignore that comment. It was about a small but related issue.

Prior to that comment, I used wrapDispatch to replace dispatch with ignore. I thought this would make things simpler, but I was surprised by some unexpected behavior. You perfectly explained what was happening in the previous comment, and then I restated things in my own words in that comment.


Did we ever solve the issue with the "stuck" slider?

Yes, the essential work was completed in PR #118. I think we left this issue open as a reminder that we could make it simpler to wrap dispatch is common ways, such as debouncing and throttling.

@cmeeren
Copy link
Member

cmeeren commented Oct 26, 2019

the slider doesn't move until the debounce completes.

That is the correct behavior for debounce. You probably meant to throttle.

The slider is also stuck for throttle (i.e. Sample) until a message is dispatched.

But I realize now that it's expected, since the slider is, after all, locked to the model's value.

@cmeeren
Copy link
Member

cmeeren commented Oct 26, 2019

After experimenting a bit: Is the slider really a problem? I set up a slider with 10,000 steps, and when I quickly dragged it from 1 to about halfway, it only dispatched for the following values, without needing any wrapDispatch: 113, 4158, 4270, 4383, 4495.

@TysonMN
Copy link
Member Author

TysonMN commented Oct 26, 2019

Yes, that is correct. My original description of the problem was that every tick in the interval gets put into a message and dispatched. I was wrong about that.

Eventually I figured out that the bad behavior is caused by too many messages and one iteration of the dispatch loop executing too slowly. There are two numbers then: how long am iteration takes to execute and the throttling interval. As I recall, the best behavior is achieved when the throttling interval is slightly longer than the execution time, like maybe by 30 - 50 ms. This makes sense because if something can be denounced/throttled, then the interval time might as well be at least the execution time. Of course in practice, the execution time would not be known and it would not be a constant. To come to this conclusion, I added a call to sleep in update and played with both parameters.

So the slider is just one way to dispatch many somewhat redundant messages.

I understand Elmish.WPF much better now than I did when I first raised this issue. I am still happy that we added dispatch wrapping, but I think the more urgent issue is the performance of the seq bindings.

@TysonMN
Copy link
Member Author

TysonMN commented Oct 26, 2019

But I realize now that it's expected, since the slider is, after all, locked to the model's value.

That is what confused me at first. I thought WPF would keep its own value for the slider that would be become inconsistent with our model if debouncing/throttling decided to hold onto a message (or in the extreme case, dispatch is replaced with ignore). That is what I was talking about in #114 (comment).

@cmeeren
Copy link
Member

cmeeren commented Apr 17, 2020

Is this issue still relevant?

@TysonMN
Copy link
Member Author

TysonMN commented Apr 18, 2020

We can close this issue. The main thing we achieved (in PR #118) was adding the optional wrapDispatch arguments. As a convenience, we could also define special cases for passing in as wrapDispatch that implement common behavior, such as throttling and deboucing. (I vaguely recall coming across a third common case, but now I don't see mentioned anywhere. Oh well.)

If anyone wants something like that, then they can comment here or open a new issue.

Also, these functions would be independent of Elmish.WPF (and WPF), so we implement them here first for convenience and then maybe one day they could be promoted to Elmish.

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

No branches or pull requests

2 participants