[layout] avoid IView[] creation in LayoutExtensions #8250
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonathanpeppers
commented
Jun 22, 2022
Comment on lines
-10
to
-12
| class ZIndexComparer : IComparer<ViewAndIndex> | ||
| { | ||
| public int Compare(ViewAndIndex? x, ViewAndIndex? y) |
There was a problem hiding this comment.
It appears that a regular OrderBy() is the same as this logic. Let me know if I'm missing something, thanks!
There was a problem hiding this comment.
Yeah, I only wrote it the hard way because otherwise I'd have to argue with people about the Linq :)
There was a problem hiding this comment.
I found other places where performance matters, that used OrderBy():
This is used when MSBuild evaluates a project, for every type of item group in a build. It would be called when VS loads a project.
hartez
suggested changes
Jun 22, 2022
hartez
reviewed
Jun 22, 2022
| return layout.OrderByZIndex().IndexOf(view); | ||
| switch (layout.Count) | ||
| { | ||
| case 0: |
There was a problem hiding this comment.
I bet this is a huge chunk of the performance gain.
Context: https://github.com/unoplatform/performance/tree/master/src/dopes/DopeTestMaui Reviewing `dotnet trace` output of the "dopes" test sample, 7% of CPU time is spent in `LayoutExtensions.OrderByZIndex()`: 3.42s (16%) microsoft.maui!Microsoft.Maui.Handlers.LayoutHandler.Add(Microsoft.Maui.IView) 1.52s (7.3%) microsoft.maui!Microsoft.Maui.Handlers.LayoutExtensions.GetLayoutHandlerIndex(Microsoft.Maui.ILayout,Microsoft.Maui.IVie... 1.50s (7.2%) microsoft.maui!Microsoft.Maui.Handlers.LayoutExtensions.OrderByZIndex(Microsoft.Maui.ILayout) Reviewing the code in `OrderByZIndex()`: * Makes a `record ViewAndIndex(IView View, int Index)` for each child * Makes a `ViewAndIndex[]` the size of the number of children * Call `Array.Sort()` * Makes a `IView[]` the size of the number of children * Iterating twice over the arrays in the process Then if you look at the `GetLayoutHandlerIndex()` method, it does all the same work to create an `IView[]`, get the index, then throws the array away. I removed the above code and used a System.Linq `OrderBy()` with faster paths for 0 or 1 children in `GetLayoutHandlerIndex()`. I also made `OrderByZIndex()` return `IOrderedEnumerable` for use by `foreach` loops. This avoids creating arrays in those cases. The `ZIndexTests` still pass for me, which proves out `OrderBy()`'s stable sort: https://docs.microsoft.com/dotnet/api/system.linq.enumerable.orderby After this change, the % time spent in these methods went down: 1.84s (13%) microsoft.maui!Microsoft.Maui.Handlers.LayoutHandler.Add(Microsoft.Maui.IView) 352.24ms (2.50%) microsoft.maui!Microsoft.Maui.Handlers.LayoutExtensions.GetLayoutHandlerIndex(Microsoft.Maui.ILayout,Microsoft.Maui.IVie... 181.27ms (1.30%) microsoft.maui!Microsoft.Maui.Handlers.LayoutExtensions.<>c.<EnumerateByZIndex>b__0_0(Microsoft.Maui.IView) 2.78ms (0.02%) microsoft.maui!Microsoft.Maui.Handlers.LayoutExtensions.EnumerateByZIndex(Microsoft.Maui.ILayout) This kind of goes against the guidance "avoid System.Linq". But `OrderBy()` generally seems to be fine? ~~ Results ~~ A `Release` build on a Pixel 5 device, I was getting: Before: 103.04 Dopes/s After: 230.87 Dopes/s I suspect that was a lot of `IView[]`'s before!
jonathanpeppers
force-pushed
the
OrderByZIndex
branch
from
1378ae2
to
24263d4
Compare
rmarinho
reviewed
Jun 23, 2022
| { | ||
| indexedViews[n] = new ViewAndIndex(layout[n], n); | ||
| case 0: | ||
| return -1; |
hartez
approved these changes
Jun 23, 2022
Eilon
added
the
t/perf
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: https://github.com/unoplatform/performance/tree/master/src/dopes/DopeTestMaui
Reviewing
dotnet traceoutput of the "dopes" test sample, 7% of CPUtime is spent in
LayoutExtensions.OrderByZIndex():Reviewing the code in
OrderByZIndex():record ViewAndIndex(IView View, int Index)for each child
ViewAndIndex[]the size of the number of childrenArray.Sort()IView[]the size of the number of childrenThen if you look at the
GetLayoutHandlerIndex()method, it does allthe same work to create an
IView[], get the index, then throws thearray away.
I removed the above code and used a System.Linq
OrderBy()withfaster paths for 0 or 1 children. I also made an
internalEnumerateByZIndex()method for use byforeachloops. This avoidscreating arrays in those cases. The
ZIndexTestsstill pass for me,which proves out
OrderBy()'s stable sort:https://docs.microsoft.com/dotnet/api/system.linq.enumerable.orderby
After this change, the % time spent in these methods went down:
This kind of goes against the guidance "avoid System.Linq".
But
OrderBy()generally seems to be fine?Results
A
Releasebuild on a Pixel 5 device, I was getting:I suspect that was a lot of
IView[]'s before!