Skip to content
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

Reduce allocations #805

Merged
merged 19 commits into from
Oct 1, 2020

Conversation

TimLariviere
Copy link
Member

@TimLariviere TimLariviere commented Sep 21, 2020

It's a concurrent PR to #802 because I'm testing a different approach.
Ultimately only one PR will be merged, containing the best of the 2.

I have been profiling FabulousContacts (slightly modified version) on Windows, in an Android emulator.

Here's the list of changes I made

  • Use struct tuples instead of tuples in the ViewBuilders.Update... when comparing old/new values with match ... with
  • Use struct union for Collections.Operation
  • Remove printf in ViewUpdaters.updateNavigationPages (should use Program.withConsoleTrace instead in a future version)
  • Replaced Array.tryFind (fun x -> ...) by a simple recursive function to avoid lambda context capture

Here's the result as of today (left: Fabulous 0.57, right: this PR)

By allocations
image

By methods
image

By assemblies
image

Top allocations
image

@TimLariviere
Copy link
Member Author

/azp run full build

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vshapenko
Copy link

for updateIncremental would be worth take a look at #780 to reduce unneccessary allocations, i think

@TimLariviere
Copy link
Member Author

@vshapenko For now, I'm currently looking at reducing allocations in Collections.diff, as this is where most allocations come from.

@TimLariviere
Copy link
Member Author

TimLariviere commented Sep 24, 2020

Ok, I managed to reduce allocations in Collections.diff with a dirty trick.
The problem was essentially the fact that Collections.diff and Collections.adaptForObservableCollection were returning an F# list with Operation to apply to a list to go from State A to State B. Those functions are called a lot of times (~20.000 times just for adding 20 people to the contact list in FabulousContacts), so any allocation quickly become a lot of allocations...

So to avoid having those returned F# lists, I preallocated an array from the ArrayPool for the maximum size the algorithm will need and made diff/adaptForObservableCollection directly work with it.

Also made sure that all local functions were not implicitly capturing variables to avoid allocations.

I will need to clean up this algorithm before merging.

By top allocations
image

By methods
image

By assemblies
image

Top allocations
image

@vshapenko
Copy link

Looks very promising. Looking forward to try this merge in real action

@TimLariviere
Copy link
Member Author

@vshapenko Yes! I will surely publish a preview release.

@TimLariviere
Copy link
Member Author

Fabulous has to box struct to store them in ViewElement because it stores any kind of value as obj.
So, to reduce allocations on structs (Xamarin.Forms.Color, Xamarin.Forms.Thickness and Xamarin.Forms.LayoutOptions), I put memoization mechanism by storing the boxed values and reusing those instead of reboxing each time.

Also I've been bit by dotnet/fsharp#526 when trying to reduce struct boxing.
So to avoid implicit boxing of structs like ValueOption, I replaced the = operator with a custom function.

By top allocations
image

By methods
image

By assemblies
image

Top allocations
image

@TimLariviere
Copy link
Member Author

This time, I changed the way we update Attached Properties to avoid passing an overload dictionary each time we update a ViewElement.
Instead the overloaded updater is a function stored in ViewElement just like update.

This removed instantiations of the 5th and 7th most instantiated types, but with apparently little effect on the global memory allocation.

By top allocations
image

By methods
image

By assemblies
image

Top allocations
image

@vshapenko
Copy link

@TimLariviere , preview is ready to try?

@TimLariviere
Copy link
Member Author

It's almost ready. Just need to fix the few comments I made and then I'll merge this PR and push the preview package to NuGet.
Should be done by the end of day.

I'll continue working on allocations in a next PR.

@TimLariviere
Copy link
Member Author

/azp run full build

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TimLariviere
Copy link
Member Author

/azp run full build

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TimLariviere
Copy link
Member Author

/azp run full build

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TimLariviere
Copy link
Member Author

/azp run full build

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TimLariviere
Copy link
Member Author

/azp run full build

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TimLariviere TimLariviere changed the title WIP: Reduce allocations - other part Reduce allocations Oct 1, 2020
@TimLariviere TimLariviere merged commit 44b6cca into fabulous-dev:master Oct 1, 2020
@TimLariviere TimLariviere deleted the reduce-allocations-2 branch November 23, 2020 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants