-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Lazy subModelSeq (and subModel)? #143
Comments
Yep! I want this now.
Yep, you are correct. We noticed the performance of our application decrease significantly after I added the Just like how we have Quoting #157 (comment)
Again, I am indifferent to which version bump you go with. However, I recommend that we implement these lazy bindings while using |
I agree with everything. Thanks! 👍 I suggest we wait with merging #158 until this is ready, too. Alternatively, if it makes the job easier for you, we can create a dedicated branch in this repo for these changes, and merge toward that instead, and when it's ready, merge that into master. |
Yes, I like the idea of having a dedicated branch. I used my new write access to create a branch for that purpose. |
I think I have come up with a good way to implement this. Originally I thought that we would need new Therefore, in my mind, the simplest solution is to extend all the existing sub-model bindings with an additional, optional
What do you think? |
I agree with the For I had forgotten about the present issue when I created #157. Even if I had remembered this issue, I may still have created that one since My application at work has a
WIthout a To be fair, adding an optional Typing out the previous paragraph made me realize that a runs away to code As a proof of concept,
and
become
and
How about we make this hack a first-class binding? In this world, there would be no
would look something like
This makes me think about the functor/applicative/monadic-like behaviors of lazy, seq, and opt. Maybe we can express these "effects" better thereby making it easier to create the desired binding by making it possible to compose bindings: like binding combinators. Ah! I want to think more about this now, but I have to go :'( |
Fantastic! I tried a long time ago to come up with a single lazy abstraction, but couldn't do it. If what you suggest will work, and no XAML changes are needed (i.e., no binding to a separate If the binding API can be designed to be more combinatorial in this way, I think it will end up more ergonomic. Experiment away! |
I think #151 is blocking this work. The "conceptual call stack" for my hack above using
The reason my hack needs a separate
|
I don't think that any more. I think I can get the needed recursion by calling a function that takes the I expect to make progress on this later today. |
I was thinking that the lazy binding would be created in part by calling a method Would anyone like to suggest a different or better name? Maybe |
IMHO
Further suggestions welcome. |
I also like We have two types of "lazy":
I think the behavior of the second one is more like (but not the exactly the same as) the .NET/C# type I feel like we have ample room for naming improvements here. I will keep thinking about this. |
Do that, though the term |
Oh, I just realized something that I had wrong in my head.
Elmish.WPF/src/Elmish.WPF/ViewModel.fs Lines 17 to 22 in 60fdb89
I also think I am wrong as to why "Lazy" appears in the name of Elmish.WPF/src/Elmish.WPF/ViewModel.fs Lines 13 to 22 in 60fdb89
...I thought that OneWayLazy had "Lazy" in its name because of the addition of Equals and Map . However, I now have the feeling that this is because of the use of Lazy<> .
@cmeeren, can you clarify this for me? Is it for only one reason or the other that |
It's called "lazy" because of the corresponding optimization in Elm (and in Elmish). Note that there are of course some differences due to Elmish.WPF using static views, but the primary goal and the end result is similar: Skip doing UI work if the inputs haven't changed. |
Thanks for those links. They are very helpful :)
But what is UI work? Here are three possibilities phrased in terms of Elm.
I associate the phrase "UI work" work the first possibility. Elm avoids DOM computation by doing more virtual DOM computation. Elmish.WPF also strives for the related goal of minimizing change notifications. In the case of The second possibility is mostly about the virtual DOM diffing algorithm. Being able to compute the diff more efficiently is the purpose of Elm's I don't know if Elm has any mechanism for making the third possibility more efficient. In Elmish.WPF for the case of More specifically: This is related to #176 (comment)
...which was also quoted in #178 (comment). I view the third possibility as lacking an analog in Elm or Elmish-react and maybe not even necessary in Elmish.WPF. If WPF always requests a bound value, then removing the use of |
Quoting Elm's HTML.lazy:
Then why not always do such reference equality checks? Why make the user put This is the change discussed in #157 for I think it is a good idea use reference equality checks by default. They cost almost nothing and could save loads of computation. To me, successfully using |
I'm not sure exactly what you mean by point 3, but if I understand you correctly, that would just be some variant of plain old memoization implemented and controlled by the user.
You seem to assume that each binding is used exactly once. That's not the case. You can have several XAML bindings to a single VM property (i.e., Elmish.WPF binding). Using |
I'm unsure if you want an answer (and if so, what the question is), or if you're simply writing down your reflections. Let me know if you have concrete suggestions for a change to a binding. |
I agree that point three by itself is rather unclear. This is partially because I tried to phrase things in terms of Elm, but I don't know what is possible in Elm for this case as it exists in my mind. In terms of Elmish.WPF, I am specifically referring to the use of
Sure, but computing things more than once is not the issue. Both... let a = f ()
printf "%A" a
printf "%A" a ...and... let a = lazy { f () }
printf "%A" a.Value
printf "%A" a.Value ...execute Of course WPF won't request the bound value for a binding it doesn't have, so any binding defined in Elmish.WPF that doesn't appear in any XAML sort of fits this description, but this is a pathological case. I think this case is better described as a bug. Either the user should use this binding in some XAML or the binding should be deleted. Another consequence of using Oh, this gives me an idea though. It used to be that Elmish dispatch loop (and especially
It is an honest question, but it is also fine if it is taken as being rhetorical.
Will do. |
Actually, don't bother replying my previous comment. I (think I) have an idea that will make it moot. I will create a new issue soon. |
I have thought some more about this. It seems we are doing three optimizations:
I think memoization and caching are the most important ones, since laziness is only useful if the UI doesn't fetch the value, which I think should happen very rarely, if at all (but I may be wrong). In any case, instead of calling it a variant of the reserved keyword To be honest though, I would still prefer Elmish.React has something similar and called it |
I'm using Bindings.SubModelSeq with code based on the SubModelSeq sample. Our tree has over 6,000 nodes total (not top level) in the worst case. I'll get you a minimal working example as soon as I can. |
Oh, yes. Another point from your original comment that I didn't fully appreciate. I agree. This implementation is quadratic. To be clear, the main inefficiency is with the sample code, which I presume you copied into your actual project. I don't think this is a good example to motivate the present issue. I created a MWE in this branch. It contains 1000 top-level nodes and takes about four seconds to increment a counter. I will see if I can find a quick fix. |
Hum...I pushed a commit that drops the time from four seconds to one when there are 1000 counters. But it still takes longer than I am willing to wait for 6000. I will continue to investigate. |
I pushed a commit where I did a minimal amount of work implementing lazy behavior for @ScottHutchinson, can you try this change with your private project and let us know what the performance is like now? |
FYI: I just tested with the lazy optimization in Elmish.WPF I implemented second without the caching optimization I implemented first in the |
The changes are in this branch, which is in my fork of Elmish.WPF. Sorry for the confusion. You can add my repo as another remote. |
I'm looking at your changes to the subModelSeq sample. Is the new |
Sorry for the confusion (again!). I implemented two optimizations, which are independent of each other. I have now pushed them to separate branches to help emphasize that.
So in an attempt to be as specific as possible, can you test your private application on the branch |
No worries. It's just me being slow, and my git fu is not so good. OK. I think optimization 1 is changing the Then I will figure out how to point my app to your branch to test optimization 2, which I think requires the new itemEquals parameter for the subModelSeq binding. |
I recommended trying optimization 2 first because my testing showed it gave a significantly bigger improvement.
Oh, yes. You are correct. You need to supply that argument. I suggest using |
I referenced the Elmish.WPF package built from your branch in my app, and added the two |
Huh :( I suppose I could have introduced a bug to cause that, but I don't know what it would be off the top of my head. I could increase my priority of this present GitHub issue and then implement it (bug free!). In the meantime, we can make progress on your specific issue when you are able to create a MWE for us to investigate. |
My "MWE" is at https://github.com/ScottHutchinson/Elmish.WPF-1/tree/ShowDialog_subModelSeq This branch includes a new sample project I started to add code to measure the elapsed time for a checkbox binding to complete, but I couldn't figure out how to make it work. By the way, the window also loads slowly. But I can live with that, since this number of nodes is the worst case, while most of the cases have fewer than 1,000 nodes and load much faster. I'm much more concerned about the responsiveness of the checkboxes. I thought I did the rebase correctly, but after I pushed the new branch, GitHub says "This branch is 1 commit ahead, 91 commits behind elmish:master." So I probably did something wrong. |
I locally rebased your branch on I will continue to investigate. |
I figured out the problem. First, you definitely need the lazy behavior for which this GitHub issue exists. A special case of a tree is a list. If all your nodes where in a list (i.e. if every "real" node in your tree had the "dummy node" as its parent), then you would avoid the buggy behavior related to trees that I discuss below but still need the optimization to significantly decrease the runtime. Second, my sloppy commit paired with a value for Passing in The I will create a new issue for that part. |
I created an issue about how to handle the case of trees. It is issue #236. |
I thought more about the naming. For now, I am going to use |
This feature now exists in the branch composable/Lazy/implementation. Here is an example of it working using the branch composable/Lazy/example. In this example, I implemented the If I remove the laziness, then updating the step size also ensures the delay. As stated in the OP, a great place to add this effect is "on top of" a |
One change I want to make is to address the comment I left here (with typos fixed and expanded a bit below).
Another advantage of this change (as I plan on making it) is that the resulting code will do a better (but still not perfect) job of separating pure from impure code. I have to be careful to preserve behavior. There is a a lot of impurity there. I know of three sources (not "directly" in
|
There was a discussion a mile up this issue about a name for No strong opinions, just felt like writing it down before I forgot. |
Maybe. The actual pronunciation of What about |
Well, I wasn't thinking it was "proper" pronunciation; just that if you squint a little,
|
I will go with |
Unless I'm mistaken, currently
subModelSeq
will loop through and update the sub-bindings for every item on every iteration of the update loop. This may have performance implications for large sequences. If the input sequence has not changed, the bindings should ideally be skipped altogether. This calls for asubModelSeqLazy
binding.The same might be said for
subModel
, though there's only ever one model there, so it's not as critical. But given that the model might have further sub-models (or just a lot of bindings), it should be possible to skipsubModel
, too, if the input hasn't changed.Nobody has requested this, and I don't have any immediate plans on implementing it, but I'm leaving this issue here in case anyone else wants to look at it, or just as a future reminder for myself.
The text was updated successfully, but these errors were encountered: