locked
Configurable retention of ContentPage renderers RRS feed

  • Question

  • User352 posted

    Summary

    Currently when a visual element is removed from the hierarchy, it's renderers are immediately disposed and discarded. This effect is not always desired and can cause performance issues with some usage patterns. Adding an attached property to a ContentPage allows the user to specify that the page is likely to come back onto the visual tree soon, so retaining the renderers is worth it.

    API Changes

    using Xamarin.Forms;
    
    var contentPage = new ContentPage ();
    contentPage.SetRetainsRenderer(true);
    

    Intended Use Case

    The most common use case for this API will be in conjunction with MasterDetailPages. When navigation is provided bia a List in the master, the Detail is swapped out, sometimes rapidly. It may be preferable to the developer to retain some of the more commonly accessed pages in memory without their renderers being cleared.

    Implementor

    @huangjinshe

    Tuesday, December 13, 2016 8:54 PM

All replies

  • User162453 posted

    How does this work with NavigationPage? Do we still retain renderers for pages popped off the stack? For how long? Or is the developer responsible for turning this off for pages when the renderer no longer needs to be retained?

    For that matter, if I call contentPage.SetRetainsRenderer(false), what happens? Is the renderer disposed immediately (assuming the page isn't currently on the navigation stack) or does it wait for the next call to Dispose()?

    Tuesday, December 13, 2016 9:06 PM
  • User352 posted

    I think this has to work as a hard override. If the value is true, the renderer is not available for being destroyed until the value is set to false. Once the value is set to false, it will either be destroyed or stay alive until it would normally be destroyed.

    Tuesday, December 13, 2016 9:09 PM
  • User1004 posted

    You have to think about what happens when the last reference to contentPage is lost. At this point, the renderer should go away. I'm worried that some would turn this on for all views and consider this as "late disposing" and that will hurt the performance, esp on Android...

    Tuesday, December 13, 2016 9:36 PM
  • User352 posted

    Excessive abuse of the API is definitely possible, setting the value to true should effectively keep a hard reference to contentPage alive? There could be an API to access all retained views... Just spitballing...

    Tuesday, December 13, 2016 9:38 PM
  • User181025 posted

    Why contentPage.SetRetainsRenderer(true); instead of contentPage.RetainsRenderer = true;? And why ContentPage only? Would it not be possible for people to want to hold on to renderers for potentially any element? This also makes me wonder what RendererPool was designed to do.

    Tuesday, December 13, 2016 10:46 PM
  • User352 posted

    Limiting to ContentPage resolves any questions about how the hierarchy behaves if a parent is explicitly marked true and a child is marked false explicitly. Things could get very hairy, very edge case-y, very fast. Since you can't nest a ContentPage into a ContentPage, this is not an issue.

    As for why its not a direct property, mostly because C# doesn't support extension properties, it only support extension methods. This would be implemented as an attached property. The idea here mostly being to keep the "magic dust" part of the API separate from the core API.

    Tuesday, December 13, 2016 10:49 PM
  • User225119 posted

    My idea is that we should always recommend developers use an exist page: [UWP] Fix Navigate/Switch to exist page performance and lost state problem in real phone. I'm sure how you guys doing on other platform, but in Lumia 640XL, open an exist page will take 5 seconds it will really let you crazy.

    The main reason was 2:

    1.Clean() previous page, that will take half time for it. 2.ContentPage.Content = renderer.ContainerElement That will take another mainly half time.

    I changed 2 place for this: 1. remove all Clean(). 2.change Content to Canvas, it's a UIElement before. and basically add all renderer.ContainerElement to the Canvas, when you decide display one, you just set it Visibility = true, not display, just don't let it visible.

    It worked pretty fine, But @TheRealJasonSmith closed my pull request. Looks like I should discuss with you guys.

    Actually I'm doing this for fix memory leak, not for increase memory leak(Don't you guys feel there are too much memory leak will came because you switch pages? at least in UWP I feel that way). But guys, who will crazy enough to create a lot of new pages? I mean, maybe someone want to create a page for display website, then he should only use 1 exist page, and it will let performance more easy right. So that's why I just choice the performance, and not clean the previous page every time. Another thing is: Did you guys think an App will run on your phone every moment? I really don't think so, Android has a memory leak by the OS(maybe now better), Also iOS will close your App, if the other App need more memory. Why we should worry for memory and let the App slow as hell? (We all know the App can't run in your phone forever, maybe it will close, not because of you App, because of the OS, or maybe because of the other App).

    My suggestion is that we let the performance better, If you see my pull request you should know, actually in current design, every time go/back navigate/switch, it will slow and also will lose the previous page state. that's too bad. We should improve the performance first, at least let user experience better, then we consider about memory, How about that?(Of course we need to recommend developers use exist page, but I think no one will create a new page if he know that will slow and take performance)

    Wednesday, December 14, 2016 7:55 AM
  • User225119 posted

    Of course, the page not clean, but in the controls from the page, it need to be clean when removed from page.

    Wednesday, December 14, 2016 8:07 AM
  • User352 posted

    What happens currently is as follows. Note that each "comment" bellow really represents a break in time where the user taps another button or something to trigger the next line of code.

    mdp.Detail = page1;
    
    // page1 renderers are created
    
    mdp.Detail = page2;
    
    // page1 renderers are destroyed, page2 renderers are created
    
    mdp.Detail = page1;
    
    // page2 renderers are destroyed, page1 renderers are created
    
    mdp.Detail = page2;
    
    // page1 renderers are destroyed, page2 renderers are created
    

    In the PR you have proposed essentially page1 renderers and page2 renderers are created exactly once, and never destroyed. This is fine so long as you intend to keep them around forever. However this breaks down if you have an app that does.

    mdp.Detail = new Page1();

    If it does that instead of assign the same instance every time (recreates a new page every time), since nothing is destroying the old renderers in your PR, the app will OOM and die fairly quickly.

    So the fix we are proposing here is to do the following:

    page1.SetRetainsRenderer(true);
    page2.SetRetainsRenderer(true);
    
    mdp.Detail = page1;
    
    // page1 renderers are created
    
    mdp.Detail = page2;
    
    // page2 renderers are created
    
    mdp.Detail = page1;
    
    // no renderers are created or destroyed, page1 renderers are just made active again
    
    mdp.Detail = page2;
    
    // no renderers are created or destroyed, page2 renderers are just made active again
    
    page1.SetRetainsRenderer(false);
    
    // page1 renderers are destroyed since it is not visible and the retain flag is now off
    
    page2.SetRetainsRenderers(false);
    
    // page2 renderers are NOT destroyed since it IS visible, however now that the retain flag is off...
    
    mdp.Detail = page3;
    
    // page2 renderers are now destroyed
    

    It is my belief that this system fixes your needs without causing any issues for other users. Performance can be dramatically boosted while retaining full backwards compatibility. The reason I closed your PR is because this fixes your issue more efficiently and handles use cases your PR doesn't (Tab addition/removal, MainPage assignment, etc...), as well as working on all platforms instead of just UWP.

    Wednesday, December 14, 2016 8:07 AM
  • User1004 posted

    My naïve thought is that looks quite simple to implement, and the capability is almost there already. And I checked the feasibility in iOS (only, for now).

    All the renderer affectation to pages goes through [Get|Set|Create]Renderer, and renderers are ultimately disposed after we do SetRenderer(page, null) (it should also catch the Dispose() calls, that's probably were it start getting complicated...). If all the [Get|Set|Create]Renderer calls are rewritten into GetOrCreateRenderer(), UnSetRenderer() and IsRendererSet() we will have a single point in which we should check for the RetainsRendererProperty value.

    Wednesday, December 14, 2016 8:52 AM
  • User225119 posted

    @TheRealJasonSmith said: What happens currently is as follows. Note that each "comment" bellow really represents a break in time where the user taps another button or something to trigger the next line of code.

    mdp.Detail = page1;
    
    // page1 renderers are created
    
    mdp.Detail = page2;
    
    // page1 renderers are destroyed, page2 renderers are created
    
    mdp.Detail = page1;
    
    // page2 renderers are destroyed, page1 renderers are created
    
    mdp.Detail = page2;
    
    // page1 renderers are destroyed, page2 renderers are created
    

    In the PR you have proposed essentially page1 renderers and page2 renderers are created exactly once, and never destroyed. This is fine so long as you intend to keep them around forever. However this breaks down if you have an app that does.

    mdp.Detail = new Page1();

    If it does that instead of assign the same instance every time (recreates a new page every time), since nothing is destroying the old renderers in your PR, the app will OOM and die fairly quickly.

    So the fix we are proposing here is to do the following:

    page1.SetRetainsRenderer(true);
    page2.SetRetainsRenderer(true);
    
    mdp.Detail = page1;
    
    // page1 renderers are created
    
    mdp.Detail = page2;
    
    // page2 renderers are created
    
    mdp.Detail = page1;
    
    // no renderers are created or destroyed, page1 renderers are just made active again
    
    mdp.Detail = page2;
    
    // no renderers are created or destroyed, page2 renderers are just made active again
    
    page1.SetRetainsRenderer(false);
    
    // page1 renderers are destroyed since it is not visible and the retain flag is now off
    
    page2.SetRetainsRenderers(false);
    
    // page2 renderers are NOT destroyed since it IS visible, however now that the retain flag is off...
    
    mdp.Detail = page3;
    
    // page2 renderers are now destroyed
    

    It is my belief that this system fixes your needs without causing any issues for other users. Performance can be dramatically boosted while retaining full backwards compatibility. The reason I closed your PR is because this fixes your issue more efficiently and handles use cases your PR doesn't (Tab addition/removal, MainPage assignment, etc...), as well as working on all platforms instead of just UWP.

    Believe me I totally got what you want to do. But, as I said before, it would only fix the half problem, the dispose old page not the only reason cause the App freeze or slow when page appearing.

    Actually you know what, I totally agree with your idea, I just hope we could have some complement and when you guys add that property on Xamarin.Forms, I hope I could upload the changed version of my code and discuss it. How about that?

    Or maybe we should start with UWP of Xamarin.Forms.(I mean add that property but only working on UWP), because UWP performance killing people now.

    My code actually already test on change MainPage. I didn't change TabbedPage, because it's no any problem for now, at least not slow as others.

    Wednesday, December 14, 2016 11:49 AM
  • User352 posted

    Based on discussion here, we have decided to accept this proposal.

    Wednesday, December 14, 2016 5:29 PM
  • User225119 posted

    @TheRealJasonSmith , Sure, please leave a message in here after you merge the new property on Github, I'll be change the old code. Thanks.

    Wednesday, December 14, 2016 5:55 PM
  • User352 posted

    No developer is currently assigned to work on this, if its something you want to do we are happy to help you get up and running on it. We have a pretty solid idea of how it could be implemented, but have not even assigned someone to work on it.

    Wednesday, December 14, 2016 6:16 PM
  • User225119 posted

    @TheRealJasonSmith said: No developer is currently assigned to work on this, if its something you want to do we are happy to help you get up and running on it. We have a pretty solid idea of how it could be implemented, but have not even assigned someone to work on it.

    Ok then, I add it in a few days by myself, and implement it on UWP for now, and you guys reopen my pull request, then we discuss again. Do you agree?

    Wednesday, December 14, 2016 6:23 PM
  • User352 posted

    Yes that is absolutely fine :)

    We cant accept the PR without it working on all platforms of course, but it will at the least allow someone else to jump in and finish it if you get it working and proved out on UWP.

    Wednesday, December 14, 2016 6:30 PM
  • User225119 posted

    @TheRealJasonSmith said: Yes that is absolutely fine :)

    We cant accept the PR without it working on all platforms of course, but it will at the least allow someone else to jump in and finish it if you get it working and proved out on UWP.

    Sure, but last question, How about we just add it to Page, not ContentPage. Because maybe some one will do this(maybe switch back very frequently):

    App.MainPage = exist NavigationPage;
    App.MainPage = exist MasterDetailPage;
    

    In this case, if we always need to clean the previous navigate page, it will cause some problems. I think we should check the type when SetRetainsRenderer like:

    public void SetRetainsRenderer(bool value)
    {
        if (currentPage is not MasterDetailPage or ContentPage or NavigationPage)
            {
                return new NotImplementException();
            }else{
                   _isRetains = value;
             }
    }
    

    Then we could improve the performance one by one when we found some page types need to use it.

    Wednesday, December 14, 2016 8:59 PM
  • User352 posted

    I am fine with it being on Page, but then we need to define in advance how the property behaves in the case where a Parent is set to retain and a child isn't.

    I personally think the best solution is you get retained if you are marked retain OR you have an ancestor which is retained.

    Wednesday, December 14, 2016 9:03 PM
  • User225119 posted

    @TheRealJasonSmith said: I am fine with it being on Page, but then we need to define in advance how the property behaves in the case where a Parent is set to retain and a child isn't.

    I personally think the best solution is you get retained if you are marked retain OR you have an ancestor which is retained.

    Ok then, based your advice, I think we better use property, not method. I put a new property in Page class:

    private bool _retainsRenderer = false;
    public bool RetainsRenderer
    {
           get { return _retainsRenderer; }
           set
           {
                if (Xamarin.Forms.Device.OS == TargetPlatform.Windows || Xamarin.Forms.Device.OS == TargetPlatform.WinPhone)
                {
                    _retainsRenderer = value;
                 }
                 else
                  {
                     throw new NotImplementedException("Only worked for Windows platform for now.");
                  }
    
             }
    }
    

    I said before check the pages type, but I realize App.MainPage = any page will put here So I don't check for page type here, Also In this way the developers would get the value, we should check the Page.RetainsRenderer, then no matter someone extend a class override will not affecting this.

    After that in Platform class(for App.MainPage), I add the new property check:

    // the original
     if (popping)
             previousPage.Cleanup();
    
        // new changing
        if (popping && !previousPage.RetainsRenderer)
        {
              // the logic for another performance problem
               _container.Children.Remove(previousRenderer.ContainerElement);
               previousPage.Cleanup();
        }else
        {
             // the logic for another performance problem
             previousRenderer.ContainerElement.Visibility = Visibility.Collapsed;
        }
    

    For navigation page:

    // the original
    if (isPopping)
      _currentPage.Cleanup();
    
    // new changing
    // Don't clean if RetainsRenderer is true
    if (isPopping && !_currentPage.RetainsRenderer)
    {
           IVisualElementRenderer previousRenderer = _currentPage.GetOrCreateRenderer();
           _container.RemoveContent(previousRenderer.ContainerElement);
            _currentPage.Cleanup();
    }
    

    For MasterDetail page, in ClearDetail I also checked only execute the if (!_detail.RetainsRenderer).

    A few things you need to know(If you prepare to add it for Android and iOS, I did these only for UWP): 1.If the Page is using RetainsRenderer, you need to make sure, manually fire Disappearing or Appearing events if your page evens are based on load and unload. Because after this changing, your page will should only load once. 2.You should make sure your page resize again when you navigate to RetainsRenderer page(a exist page), because your old page not new created, maybe your phone changed the orientation in the other pages.(In my case I need to do it , because I put all page render to a Canvas, and use Visibility control to display or not when use RetainsRenderer, that will more high performance than display a page , even the page already exist) 3.Make sure the navigation bar or title will display correctly if your change App.MainPage with use RetainsRenderer.

    Of course these things based on your platform old design, I just said something what I meant.

    I test these on my Lumia640XL, and worked fine. So I commit it now. @TheRealJasonSmith , please reopen my pull request: [UWP] Fix Navigate/Switch to exist page performance and lost state problem in real phone Let someone continue add it for other platform or some page types which they think have performance problems.

    Thursday, December 15, 2016 8:31 AM
  • User352 posted

    We dont want to use a property or a method, its an extension method, defined somewhere, who cares right now we can figure that out in the PR, which sets an attached bindable property.

    Why?

    Well the idea is fairly simple, no API on the core objects should leak the abstraction the renderers provide. By making a core API about them, you effectively leak that abstraction. Another way to think of it is this API is a bandaide for the fact that we just haven't figured out how to smart enough to do this automatically and correctly 100% of the time (and we never will). In theory if we ever did figure that out we could obsolete the API.

    In short, it's really important from a framework design standpoint that the API be separate and implemented as an extension method. Also it needs to be an Attached BindableProperty so it can be bound from XAML, which means you can't do things like put the logic in the getter/setter anyway.

    Thursday, December 15, 2016 8:36 AM
  • User225119 posted

    @TheRealJasonSmith said: We dont want to use a property or a method, its an extension method, defined somewhere, who cares right now we can figure that out in the PR, which sets an attached bindable property.

    Why?

    Well the idea is fairly simple, no API on the core objects should leak the abstraction the renderers provide. By making a core API about them, you effectively leak that abstraction. Another way to think of it is this API is a bandaide for the fact that we just haven't figured out how to smart enough to do this automatically and correctly 100% of the time (and we never will). In theory if we ever did figure that out we could obsolete the API.

    In short, it's really important from a framework design standpoint that the API be separate and implemented as an extension method. Also it needs to be an Attached BindableProperty so it can be bound from XAML, which means you can't do things like put the logic in the getter/setter anyway.

    I just pull a new request, looks like need to change the code again.... it needs to be an Attached BindableProperty, I could understand and it can be change. But use extension method, defined somewhere, also could get when need to check, that's....really let me little bit worry, You know I'm the guy from outside, I don't want to put it somewhere let your team API turn to dirty, Maybe I'm still not very get your point. Do you have a good sample let me understand(Did you guys did any similar implement before which I can see how is it looks like in the source code?)....

    Thursday, December 15, 2016 9:43 AM
  • User225119 posted

    I could add a extension method in somewhere for UWP:

    #if WINDOWS_UWP
    namespace Xamarin.Forms.Platform.UWP
    #else
    namespace Xamarin.Forms.Platform.WinRT
    #endif
    {
        public static class RetainsRendererExtentions
        {
            public static Xamarin.Forms.Page SetRetainsRenderer(this Xamarin.Forms.Page page, bool value)
            {
                page.RetainsRenderer = value;
                return page;
            }
        }
    }
    

    But if we need to it's a Attached BindableProperty so it can be bound from XAML, we still need to put it to the Page:

     public bool RetainsRenderer
            {
                get { return (bool)GetValue(RetainsRendererProperty); }
                internal set { SetValue(RetainsRendererProperty, value); }
            }
            public static readonly BindableProperty RetainsRendererProperty = BindableProperty.Create("RetainsRenderer", typeof(bool), typeof(Page), false);
    

    But apparently this way will conflict your idea that: don't put something in core....

    Thursday, December 15, 2016 10:27 AM
  • User225119 posted

    @TheRealJasonSmith , How do you think now ? I remove it from Page, create a class in Xamarin.Forms.Core and put all what you mention:

    public static class RetainsRendererExtentions
        {
            public static Xamarin.Forms.Page SetRetainsRendererValue(this Xamarin.Forms.Page page, bool value)
            {
                SetRetainsRenderer(page, value);
                return page;
            }
    
            public static bool GetRetainsRendererValue(this Xamarin.Forms.Page page)
            {
                return GetRetainsRenderer(page);
            }
    
    
            public static readonly BindableProperty RetainsRendererProperty = BindableProperty.CreateAttached("RetainsRenderer", typeof(bool), typeof(RetainsRendererExtentions), false);
    
            public static bool GetRetainsRenderer(BindableObject view)
            {
                return (bool)view.GetValue(RetainsRendererExtentions.RetainsRendererProperty);
            }
    
            public static void SetRetainsRenderer(BindableObject view, bool value)
            {
                view.SetValue(RetainsRendererExtentions.RetainsRendererProperty, value);
            }
        }
    

    Now, 1.Can use extension method for Set or Get, 2.It's a Attached BindableProperty, so you could access it from XAML: <AnyPage xmlns:forms="clr-namespace:Xamarin.Forms;assembly=Xamarin.Forms.Core" forms:RetainsRendererExtentions.RetainsRenderer="True" /> 3.Page class not changed, no API on the core objects should leak the abstraction the renderers provide.

    Thursday, December 15, 2016 5:47 PM
  • User352 posted

    @huangjinshe perfection. You have the API right there. Please feel free to continue implementing support. I strongly advise you read StephaneDelcroix's post above with provides a cleaner mechanism of implementation which wont require significant changes to any renderer. Instead the implementation can take place mostly outside of the renderers, which will minimize the patch size and also keep the renderer complexities down a bit.

    Thursday, December 15, 2016 6:55 PM
  • User225119 posted

    @TheRealJasonSmith said: @huangjinshe perfection. You have the API right there. Please feel free to continue implementing support. I strongly advise you read StephaneDelcroix's post above with provides a cleaner mechanism of implementation which wont require significant changes to any renderer. Instead the implementation can take place mostly outside of the renderers, which will minimize the patch size and also keep the renderer complexities down a bit.

    No problem, I've commit the code already. I know what you mention, but the renderer can't avoid some changes because after this changing need to resize or fire some event(as I mention before), also for UWP, there are still has some change required for fix the half performance issue. OF course I would like to discuss about it. and I will be try my best to minimize the path and also keep the renderer complexities down.

    Also I need some guys come and help to improve other platform and other pages for it. I will continue try to find some pages on UWP has performance problems. I more hope this will merge faster, then we could get some feedback, Also I want to try fix more problems if the time available, Like ListView memory leak it's a second thing I really like to check out.

    Friday, December 16, 2016 3:55 AM
  • User225119 posted

    @TheRealJasonSmith , Can't we merge it for UWP first? My code turning to old, it takes too much time, every day much more new pull request on Github, it will turn this thing no point if not merge, and keep changing the class from other pull request.

    Wednesday, December 21, 2016 2:13 PM
  • User140202 posted

    @huangjinshe We would not want to merge core APIs that only apply to one platform (at least for some indeterminate period of time) because it goes against the design principles of Forms and would subsequently confuse people. Because Forms is cross-platform, developers would expect that those APIs in core apply to all platforms, and partial implementation is not something we would seek to make a trend moving forward.

    Wednesday, December 21, 2016 4:00 PM
  • User225119 posted

    @PaulDiPietro said: @huangjinshe We would not want to merge core APIs that only apply to one platform (at least for some indeterminate period of time) because it goes against the design principles of Forms and would subsequently confuse people. Because Forms is cross-platform, developers would expect that those APIs in core apply to all platforms, and partial implementation is not something we would seek to make a trend moving forward.

    So what do you suggest? I just want to fix some problems buddy, I can't force someone go to fork and add something for iOS or Android. Also I can't just wait until there are too much changed and go to get new code, modify all again. Also the performance problems killing me right now.(It also cause a serious memory leak, My customer too much unhappy). How do I supposed to do now? I'm wait for half year, and really can't wait, that's why I add it myself.

    Why don't you team do it? I mean add for Android and iOS. and merge?

    Wednesday, December 21, 2016 4:10 PM
  • User352 posted

    If you don't add support for the other platforms we will eventually, it just might be longer than you desire.

    Wednesday, December 21, 2016 5:49 PM
  • User225119 posted

    @TheRealJasonSmith said: If you don't add support for the other platforms we will eventually, it just might be longer than you desire.

    I don't have some device for test for other platform(I'll try if I have), Sure, I'll wait. And I'll continue check other problems or BUG. Thank you.

    Saturday, December 24, 2016 4:07 PM