Mvvmcross: Axml binding performance issue

[johnthiriet]

Hi everyone,

I stumbled upon some performance issue with navigation in a Android app. I managed to do some profiling and I realised most of the time was spend while applying binding. It's not surprising but still the numbers were pretty high (about half of a second).
Digging further let me see a window for potential performance improvements.

Let's take the exemple of the ReflectionExtensions file : https://github.com/MvvmCross/MvvmCross/blob/67db28fad46438d113377cfadc3a3df23240e3d9/MvvmCross/Platform/Platform/ReflectionExtensions.cs

There you can see a tremendous usage of Linq which doesn't help making the whole binding process quick.

I think it would be a great idea to completely remove Linq from the binding process since it adds an overhead above reflection which is already costly by itself.

It has already be done in some other libraries : JSON.NET
https://github.com/JamesNK/Newtonsoft.Json/commit/221beff235ced3ac3e74d056df02e3af052159a5

So what do you think about it ?

[roubachof]

related to #1322 ?

[johnthiriet]

1322 will help the view appearing faster it's true. Nonetheless it doesn't change the fact that too much Linq is done on the ReflectionExtensions. #1322 will just make another "thread" do the work not speeding the operation.

[Cheesebaron]

Kind of related to #1322

Removing LINQ would improve performance a bit, I think we should look into making the transition.

[kjeremy]

Makes sense to me as long as it improves things. Do you have any performance numbers (or a test) so we can track the performance?

[johnthiriet]

It's hard to extract from our current project but it should not be too hard to create a horribly long axml file full of bindings to serve as a basis.
Please note that for the moment Xamarin Profiler doesn't offer the ability to track time (it has been removed). So we have to ressort to the old way of doing that by using directly the mono profiler.
Or even more efficiently : Just outputting the time directly with MvxStopWatch.
I'll try to make a sample if I find some time this weekend and I'll post it right here.

[kjeremy]

Should putting a stopwatch around BindingInflate should give us a rough idea. You may be able to override a class in MvvmCross in your Setup.cs to inject the necessary stopwatch as well.

[johnthiriet]

I've set up a little test project where I test Linq speed, @Cheesebaron's fix and another way without Linq or Yield return.

https://github.com/johnthiriet/MvvmCrossBindingInflatePerformanceTest

In my set up Linq tends to be faster which is weird. I'm using the latest Google Emulators and didn't have time to try it on a device yet.

Please give it a try if you can find different numbers.

[Cheesebaron]

Using DateTime for measuring time is not really ideal. Stopwatch is more precise.

[kjeremy]

@johnthiriet I think your test repo is incomplete. I don't see a customized MvxActivity implementation.

[roubachof]

If there is no impact on performance, for readiness sake, I would prefer to keep the linq implementation.
For general axml binding performance, I think @softlion did some benchmarking with or wo his PR.

[kjeremy]

After modifying the test to use a Stopwatch as in:

public override void SetContentView(int layoutResId)
{
    var sw = Stopwatch.StartNew();
    var view = this.BindingInflate(layoutResId, null);

    Mvx.Trace(MvxTraceLevel.Diagnostic, "Binding Inflate Duration {0}", sw.ElapsedMilliseconds);
    this.SetContentView(view);
}

I get the following timings on a Tango:

| | Current | revert of d48a93eacdc86eb3aa57143c3db3360652810961 |
| --- | --- | --- |
| First Inflation | 711 | 772 |
| Rotation | 652 | 678 |
| Rotation | 553 | 653 |
| Rotation | 620 | 616 |

There doesn't appear to be too much of a difference.

[johnthiriet]

So basically it shows that without Linq it seems to be faster than with it.
It would be interesting to test the same thing with my modified version without Linq nor yield.
The difference doesn't seem so high but on complex layout with a lot of fragments such as the one I'm working on at work it would make a significant difference.

[kjeremy]

Yeah if we had a really complex view hierarchy we would probably see a
difference.

On Mon, Jun 6, 2016 at 11:41 AM, John Thiriet [email protected]
wrote:

So basically it shows that without Linq it seems to be faster than with it.
It would be interesting to test the same thing with my modified version
without Linq nor yield.
The difference doesn't seem so high but on complex layout with a lot of
fragments such as the one I'm working on at work it would make a
significant difference.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/MvvmCross/MvvmCross/issues/1342#issuecomment-223998399,
or mute the thread
https://github.com/notifications/unsubscribe/AEIBRP3CYb8Skgcde3DdbhuN0G63nmfGks5qJD-2gaJpZM4ItZZK
.

[Cheesebaron]

I can try remove the yield stuff from the code. Give me some minutes, will make a pull request shortly

[johnthiriet]

You can try to use the code I made if you want to test it fast : https://github.com/johnthiriet/MvvmCrossBindingInflatePerformanceTest

[slodge]

I suspect this might be looking in the wrong place... I would be surprised* if the Linq is the mahjor thing causing the issues in this case. It would feel to me like removing the Linq is more of a micro-optimisation.

My number one place to look would be to look at binding causing multiple individual value changes each of which causes a separate "container"-level layout recalculation (plus extra draw methods). I seem to remember the hierarchy viewer being able to give you some profiling information on drawing, layout, etc. If that's the case, then there might be something cunning that could be done at the init/create level - something that suppresses layout until after all the bindings have been set...

Beyond that, it might also be worth looking at whether the property lookup could be cached (per Type, string tuple...)...

....plus using a profiler might also tells you some interesting things...

but then I could be very wrong - * I'm often surprised :)

[johnthiriet]

@slodge I very much agree that there is probably other issues in the code we need to take into account. I've started removing layout nesting layout where I could to speed up everything by the way (it helps also).
I used a profiler and there I noticed that I spent a lot of time on the BindingInflate method and dug further to realise a lot was spent into these Linq operation.
I did not say Linq was the only cause of the issue but, even though It may be a micro optimisation, it's neither hard to maintain nor hard to read to remove it from this single file. If removing Linq from a single file helps the BindingInflate method do its work faster and that the modification is easy and without broad impact, I don't see a reason not to do it. Especially since MvvmCross is a framework and frameworks should be as lightweight as possible :)

[kjeremy]

Does removing LINQ actually improve the numbers? I don't think we have a proper benchmark yet to either confirm or deny that LINQ is a problem other than intuition.

[Cheesebaron]

@johnthiriet your sample app does not contain any of the Stopwatch code you mention.

@slodge LINQ vs. foreach in this case is indeed a micro-optimization. Caching the results where appropriate might help a lot. We might also want to look into using Dynamics/Expression instead of Reflection to get more performance.

There's some more info here:

http://theburningmonk.com/2010/09/performance-test-dynamic-method-invocation-in-csharp-4/
http://geekswithblogs.net/SunnyCoder/archive/2009/06/26/c-4.0-dynamics-vs.-reflection.aspx
https://codeblog.jonskeet.uk/2008/08/09/making-reflection-fly-and-exploring-delegates/
http://theburningmonk.com/2015/08/fasterflect-vs-hyperdescriptor-vs-fastmember-vs-reflection/
http://stackoverflow.com/questions/7189642/speeding-up-reflection-invoke-c-net

[johnthiriet]

I've modified the test to use MvxStopwatch and opening/closing the activity automatically 20 times.
On my real device (a Xiaomic 4c) there is a slight performance gain without using Linq nor Yield.

The axel layout used in the test is pretty simple, I'm almost pretty sure that by using a more complex layout with nested layouts and such the performance gain would be much more obvious.

| Test | No Linq Nor Yield | Using Yield | Using Linq |
| :-: | :-: | :-: | :-: |
| Test | 1330 | 1397 | 1300 |
| Test | 1266 | 1346 | 1234 |
| Test | 1320 | 1416 | 1306 |
| Test | 1323 | 1437 | 1331 |
| Test | 1376 | 1458 | 1363 |
| Test | 1462 | 1594 | 1404 |
| Test | 1450 | 1549 | 1463 |
| Test | 1537 | 1572 | 1522 |
| Test | 1544 | 1607 | 1639 |
| Test | 1595 | 1702 | 1610 |
| Test | 1612 | 1700 | 1678 |
| Test | 1664 | 1737 | 1683 |
| Test | 1718 | 1906 | 1712 |
| Test | 1880 | 1954 | 1742 |
| Test | 1895 | 1878 | 1794 |
| Test | 1908 | 2481 | 2460 |
| Test | 1938 | 1866 | 1917 |
| Test | 1871 | 2022 | 1971 |
| Test | 2031 | 1935 | 1853 |
| Test | 2123 | 2237 | 2045 |
| Test | 1946 | 2027 | 2103 |
| | | | |
| *Average | 1656,619048 | 1753,380952 | 1672,857143 |

[kjeremy]

I don't understand why the execution time trends upwards. Is this with a Release version of MvvmCross and latest Xamarin stable? Those are pretty terrible times.

[kjeremy]

Also please use var sw = Stopwatch.StartNew(); instead of MvxStopWatch. It is more precise.

[johnthiriet]

I've done the modification,

The application uses a MvvmCross compiled in release mode.
We still see this execution time trends going upward, I don't really understand why for the moment. Maybe it is due to some memory pressure increase overtime since the GC doesn't run as often as it may have to.

| Test | No Linq Nor Yield | Using Yield | Using Linq |
| :-: | :-: | :-: | :-: |
| Test | 1336 | 1321 | 1394 |
| Test | 1283 | 1237 | 1311 |
| Test | 1332 | 1301 | 1390 |
| Test | 1331 | 1350 | 1424 |
| Test | 1412 | 1378 | 1513 |
| Test | 1448 | 1445 | 1608 |
| Test | 1575 | 1515 | 1565 |
| Test | 1557 | 1507 | 1695 |
| Test | 1576 | 1547 | 1656 |
| Test | 1732 | 1730 | 1655 |
| Test | 1634 | 1823 | 1726 |
| Test | 1664 | 1652 | 1788 |
| Test | 1751 | 1705 | 1788 |
| Test | 1749 | 1762 | 1820 |
| Test | 1791 | 1772 | 1888 |
| Test | 2623 | 2383 | 2659 |
| Test | 1793 | 1935 | 2020 |
| Test | 1860 | 1900 | 2069 |
| Test | 2188 | 1886 | 1966 |
| Test | 1924 | 2092 | 2005 |
| Test | 1986 | 2162 | 2179 |
| | | | |
| _Average_ | 1692,619048 | 1685,857143 | 1767,571429 |

Here the winner seems to be with yield.

I've upgrade my git repo to reflect the changes.

[Cheesebaron]

You are seeing very negligible performance differences between yield and non-yield versions. So there is practically no difference between those two versions. However the picture does show that LINQ has some overhead here.

It would be interesting to see how much time is actually spent on inflating the view and how much time is spent actually binding the view. I did some small tests on doing reflection and invoking properties yesterday and it does not seem like that stuff is hitting us hard, but would still be interesting to see what the times are.

We could add more binding trace so we could see these things in debug builds.

[kjeremy]

@Cheesebaron I'm sure the time spent in view inflation is substantial. I basically re-implemented LayoutInflater/PhoneLayoutInflater with a pile of custom factories and some Java reflection. At the very least It's a lot of context switching and a pile of suck. It's been on my list to investigate alternate approaches.

[softlion]

The only good alternate approach is a build custom process or a t4 template
which creates the static binding classes. No reflection at all.

Le mardi 7 juin 2016, Jeremy Kolb [email protected] a écrit :

@Cheesebaron https://github.com/Cheesebaron I'm sure the time spent in
view inflation is substantial. I basically re-implemented
LayoutInflater/PhoneLayoutInflater with a pile of custom factories and some
Java reflection. At the very least It's a lot of context switching and a
pile of suck. It's been on my list to investigate alternate approaches.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/MvvmCross/MvvmCross/issues/1342#issuecomment-224263186,
or mute the thread
https://github.com/notifications/unsubscribe/AALpJEGlYKlAM16JzzpHDQyYKVDf03Lyks5qJWHjgaJpZM4ItZZK
.

[kjeremy]

I'll add some instrumentation for layout inflation.

[heytherewill]

@softlion Can you explain further on what classes this t4 template needs to create? I'm currently having performance problems with MvxBindingInflate and any performance boosts would be great, so I guess I can start working on this issue :)

[kjeremy]

@willsb Can you profile it with the Xamarin Profiler? I would be interested to know which part is slow.

[SlyNet]

Here is a sample
app where only 300 bindings are done. But when you scroll the view there is a noticeable lag if you scroll down to second 300 items to be shown. Could it be related to the way binding is working? I notice a lot of GC collections during scrolling, but no new view models are created.

[johnthiriet]

Hey @SlyNet.
Thanks for the sample, by looking at it I realised that there is a vertical RecyclerView within another vertical RecyclerView in your code. I really don't think that it is a good idea.
Did you try doing that using a full native approach ? I think that it would still be laggy in this kind of scenario.

[SlyNet]

@johnthiriet Will try to implement example using native android. But according to Device Monitor, its not hard for android to draw all the items http://i.imgur.com/163Q84K.png. Its about 100ms.

[SlyNet]

With native implementation there is still a lag when next 300 items needs to be shown, but it is much smaller LargeRecyclerNative.zip

[johnthiriet]

Thanks for the test. I guess the lag is to be expected as there's RecyclerView's within a RecyclerView. Less lag is also to be expected using a native approach since there's no binding inflation.
If, for any reason, you have such a performance issue on your main app, I suggested to get a rid of nested RecyclerViews by flattening the collection and using different cell templates (and therefore different RecyclerViewHolder).
From a performance point of view it should really be faster that way.