reactiveui / ReactiveUI

An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms that is inspired by functional reactive programming. ReactiveUI allows you to abstract mutable state away from your user interfaces, express the idea around a feature in one readable place and improve the testability of your application.
https://www.reactiveui.net
MIT License
8.1k stars 1.12k forks source link

WPF RoutedViewHost Navigation Performance and Application Lock-Ups #930

Open jcomtois opened 9 years ago

jcomtois commented 9 years ago

I have created a demo repository here: https://github.com/jcomtois/TestReactiveUINavigation

The sample solution above demonstrates the problem I have run into in my real-world application. The real app is for a touch-screen monitor where an operator will have a lot of buttons to choose from. Each "button" is backed one of many various viewmodels via ViewModel view hosts, which are then laid out in another ViewModelViewHost containing a grid, which might then be in another layout control backed by another viewmodel, etc. all the way up to the top level View which has the RoutedViewHost. All the viewmodels are doing their thang in the ReactiveUI way, or at least my interpretation/understanding of it. In my demo I just simulate various properties changing and binding commands.

So everything works fine until I want to navigate to another IRoutableViewModel. Routing to the other model (which is much simpler, just 1-2 non-composite sub controls) takes a long time -- more than one second. Navigating back to the original IRoutableViewModel that has a zillion controls and sub-viewmodels is much snappier.

I tried running it through a profiler, but I must admit that I'm not very adept with that tool. From what I could gather it is telling me all the time is down in the bowels of System.Reactive.SubscribeSafe() or thereabouts.

So this problem is not showing up creating the view and binding it to a viewmodel - it seems like it is during the disposal of the view and unhooking all of the subscriptions. So the user clicks a button to navigate, the button stays "depressed" and there are no UI updates for about a second, then it skips the transition animation or does a poor rendering of it, then navigates to the simpler view. Navigating back to the complex view is 2-3 times faster.

When I created the demo project, I really tried to put it through its paces with a lot of subscriptions and sub-controls. But what surprised me is that if I click the navigate forward and backward buttons many times in as rapid succession as it will let me, it will eventually lock up the program. This has happened in the debugger and detached from the debugger.

I don't know if I am doing something incorrectly, or somehow getting a race condition, or just overwhelming the dispatcher or what.

Since the navigation is so slow, I'm having to not use it all and just showing/hiding controls all on one View, which I feel may be sub-optimal.

Please let me know if you need more information regarding this.

gluck commented 9 years ago

Didn't fully grasp the issue, but if it helps I notice that when you navigate backwards, your ComplexView gets Unloaded/Loaded/Unloaded instead of a single Unload.

It results in a lot of processing (e.g. WhenActivated/Bindings) coz this view is complex.

Didn't get what caused this though.

jcomtois commented 9 years ago

This StackOverflow question seems to mirror my situation: http://stackoverflow.com/q/30946278/213169

After reading through this, I replaced the DataTemplate in my ItemsControl on the ComplexView with a manual binding to bypass ViewModelView host. The transition is now very fast...I agree with the poster though, it would be nice if this wasn't necessary. Perhaps the reflection in ViewLocator is what is bogging everything down?

gluck commented 9 years ago

After further tracking this down, it's caused by the TransitioningContentControl, there's some reference of ppl having the same issue on Silverlight.

They suggest to reset the Content to null, and indeed if you add Content = null to RoutedViewHost (right before Content = view), it fixes the issues (at the cost of messing up the fade out part of the animation).

Manual binding maybe doesn't trigger when the view is unloaded/reloaded, hence it's fixing the issue as well ? not sure what side effects it may have.

gluck commented 9 years ago

this page also describes the issue (silverlight TransitioningContentControl is very close to RxUI one).

Important: do not rely on WPF Loaded and Unloaded events for content initialization and clean up. The Loaded and Unloaded events are raised multiple times when the active content changes. This is due to the use of multiple ContentPresenters in the TransitioningContentControl required for fluent animations.

I believe we'd have to either change the IActivationForViewFetcher impl for views hosted in TransitioningContentControl, or fix TransitioningContentControl for properly raise Loaded/Unloaded events.

jcomtois commented 9 years ago

@gluck Thanks for checking this out. You might be onto something. My first attempt to fix this was to do something along the lines of below, (your first suggestion). However this approach fails because the this.WhenActivated is typically called in the constructor before the view is assigned as content so there is never a parent, so it doesn't know it is in a TransitioningContentControl.

Your second suggestion may be the way to go leveraging the TransitionBegin/TransistionCompleted events but maybe having to handle the case where it is the first content and there is no transition.

I'm hoping someone with a little more expertise in these details can chime in.

public class ActivationForViewFetcher : IActivationForViewFetcher
    {
        public int GetAffinityForView(Type view)
        {
            return (typeof(FrameworkElement).GetTypeInfo().IsAssignableFrom(view.GetTypeInfo())) ? 10 : 0;
        }

        public IObservable<bool> GetActivationForView(IActivatable view)
        {
            var fe = view as FrameworkElement;

            if (fe == null)
                return Observable.Empty<bool>();

            IObservable<bool> viewLoaded;
            IObservable<bool> viewUnloaded;

            var rvh = fe.Parent as RoutedViewHost;

            if (rvh != null)
            {
                viewLoaded = Observable.FromEventPattern<RoutedEventHandler, RoutedEventArgs>(x => rvh.TransitionStarted += x,
                x => rvh.TransitionStarted -= x).Select(_ => true);

                viewUnloaded = Observable.FromEventPattern<RoutedEventHandler, RoutedEventArgs>(x => rvh.TransitionCompleted += x,
                    x => rvh.TransitionCompleted -= x).Select(_ => false);
            }
            else
            {
                viewLoaded = Observable.FromEventPattern<RoutedEventHandler, RoutedEventArgs>(x => fe.Loaded += x,
                x => fe.Loaded -= x).Select(_ => true);

                viewUnloaded = Observable.FromEventPattern<RoutedEventHandler, RoutedEventArgs>(x => fe.Unloaded += x,
                    x => fe.Unloaded -= x).Select(_ => false);
            }

            return viewLoaded
                .Merge(viewUnloaded)
                .Select(b => b ? fe.WhenAnyValue(x => x.IsHitTestVisible).SkipWhile(x => !x) : Observable.Return(false))
                .Switch()
                .DistinctUntilChanged();
        }
    }
gluck commented 9 years ago

As I wrote in the SO answer, TransitioningContentControl can be more efficiently written in a way that don't move the current control into a different parent, I gave a try of such an implementation and it works fine/fast (albeit a minor issue irt IsHitTestVisible, but I think RxUI is to blame here, and it's fixed by adding Take(1) after the above SkipWhile).

gluck commented 9 years ago

(this control would maybe deserve a project of its own, it's been copy-pasted-modified in hundreds of projects already ...)

johnernaut commented 7 years ago

I'm having the same issue as OP, but I'm not sure how to go about getting the default RX RoutedViewHost to use the TransitionContentControl class with the performance fixes as posted above. Any ideas?

stale[bot] commented 7 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this because it helps reduce the workload of our extremely busy maintainers. If you want this issue progressed faster please start talks with a maintainer with how you can help out. There are so many ways to contribute to open-source and the most valued are often not code related. We are always looking for more help and look after those who stick around. Say howdy to one of the maintainers or browse https://reactiveui.net/contribute/ for ideas on where to start your journey.

mxvlshn commented 5 years ago

Having the same issue, but for now have no solution on how it can be fixed

andrejpanjan commented 4 years ago

I dispose an activation subscription at the end of WhenActivated block as explained here. I'm not sure if there are any side effects because of it, since I use this in a couple of views that activate view models with long running actions. So performance gain is significant.

soci1248 commented 4 years ago

We have the same performance problem discussed above, which is so serious for larger pages, that the response speed become unacceptable. Actually, I don't understand how others are not complaining about this. 0.5-2 sec to show a page with 1000-1500 visual children in RoutedViewHost or ViewModelViewHost is not acceptable. As others already researched, the source of the problem is the TransitioningContentControl. When you assign a new view to the Control property, first it removes the old visual tree, then rebuild a new one. The removal is the slowest, and it can be avoided by coding the transition in other way. You can see the cost here: image To avoid this overhead, I've attempted to create a faster ViewModelViewHost and RoutedViewHost. At first I'm not interested in animating the transition, I just want bloody fast view switching. We can add animation later. As also pointed out, the solution would be not to remove the switched out view, but only hide it. So, all of the instantiated view would be on the top of the others, but only 1 would be visible. This way the visual tree removal cost factored out. Here is the essence of this naive implementation:

class RoutedViewHost : Grid, IActivatable, IEnableLogger { private FrameworkElement _currentView; this.WhenActivated(d => { d(vmAndContract.DistinctUntilChanged().Subscribe( x => { if (x.Item1 == null) { //Content = DefaultContent; return; }

                    var viewLocator = ViewLocator ?? ReactiveUI.ViewLocator.Current;
                    var view = viewLocator.ResolveView(x.Item1, x.Item2) ?? viewLocator.ResolveView(x.Item1, null);

                    view.ViewModel = x.Item1;

                    var newView = (FrameworkElement)view;

                    if (_currentView != null) { AnimateOut(_currentView); }

                    _currentView = newView;

                    if (!Children.Contains(newView))
                    {
                        Children.Add(newView);
                    }

                    AnimateIn(newView);

                    //Content = view;
                }, ex => RxApp.DefaultExceptionHandler.OnNext(ex)));
        });

    private void AnimateIn(FrameworkElement u)
    {
        u.Visibility = Visibility.Visible;
    }

    private void AnimateOut(FrameworkElement u)
    {
        u.Visibility = Visibility.Hidden;
    }

}

AnimateIn and Out actually not animate yet, just change the visibility accordingly.

Ok, this solution works ... for a while. After some seconds or minutes, the views become disconnected from the viewmodels. It turns out, this is the same problem as described here: https://github.com/reactiveui/ReactiveUI/issues/661 The WeakEventManager.PrivateDeliverEvent method creates a WeakEvent to the WPF component which is responsible to forward NotifyPropertyChanged events to the visual components. This connection breaks when I use the rewritten RoutedViewHost. I will investigate the exact reason for this, but if anyone already has idea, please share us.

neyugnluan commented 4 years ago

I'm facing the same issue for around two years already.

<rxui:RoutedViewHost
                Grid.Row="1"
                HorizontalContentAlignment="Stretch"
                VerticalContentAlignment="Stretch"
                Panel.ZIndex="1"
                Router="{Binding Router}" />

If without animation, the view customer shows a bit slow but no effect to UX.

when I adding Transition="SlideLeft" make my view show blank first, then animation then shows my view. at code behind of view prevent activate view twice by code

IDisposable whenActivatedSubscription = null;
            whenActivatedSubscription = this.WhenActivated(d =>
            {
                Binding(d);
                d(whenActivatedSubscription); // <- Dispose of the activation subscription here
            });

The problem is when adding Transition to RootViewHost makes a big concern on performance during the transition the view.

For now, I don't know how to improve performance during routing or make complex ViewModel