Optimize resetting gesture recognizers #19987

symbiogenesis · 2024-01-18T21:08:13Z

This should improve the performance of resetting the gesture recognizers on any View by leveraging a HashSet and reducing casting

ghost · 2024-01-18T21:08:19Z

Hey there @symbiogenesis! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

mattleibow · 2024-01-19T07:48:18Z

/azp run

azure-pipelines · 2024-01-19T07:48:37Z

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow · 2024-01-19T07:48:50Z

Is it possible to add a benchmark for this?

src/Controls/src/Core/View/View.cs

jonathanpeppers

Can you add a benchmark? Thanks!

https://github.com/dotnet/maui/tree/main/src/Core/tests/Benchmarks/Benchmarks

src/Core/tests/Benchmarks/Benchmarks/GestureRecognizerBenchmarker.cs

src/Controls/src/Core/View/View.cs

symbiogenesis · 2024-01-21T07:45:04Z

Reworked it a bit.

Allocations are down, and it looks like clearing can be up to six times faster with this PR.

Before:

Method	Mean	Error	StdDev	Gen0	Gen1	Gen2	Allocated
AddLotsOfGestureRecognizers	11,889.09 us	261.119 us	723.560 us	1093.7500	125.0000	31.2500	9843.75 KB
ClearLotsOfGestureRecognizers	2,895.50 us	55.361 us	63.753 us	238.2813	234.3750	97.6563	2218.98 KB
RemoveLotsOfGestureRecognizers	55.17 us	1.037 us	1.018 us	0.3052	0.2441	-	2.93 KB
AddOneGestureRecognizer	3,932.09 us	49.127 us	43.550 us	437.5000	429.6875	15.6250	4047.62 KB
RemoveOneGestureRecognizer	2,373.05 us	45.308 us	50.360 us	292.9688	289.0625	-	2719.46 KB
ClearOneGestureRecognizer	3,896.27 us	77.675 us	155.126 us	562.5000	554.6875	-	5172.02 KB

After:

Method	Mean	Error	StdDev	Median	Gen0	Gen1	Gen2	Allocated
AddLotsOfGestureRecognizers	10,579.72 us	199.913 us	442.994 us	10,593.28 us	1000.0000	125.0000	31.2500	9154.29 KB
ClearLotsOfGestureRecognizers	542.30 us	10.537 us	13.326 us	540.67 us	149.4141	128.9063	52.7344	1478.13 KB
RemoveLotsOfGestureRecognizers	52.00 us	0.846 us	1.838 us	51.57 us	0.3052	0.2441	-	2.93 KB
AddOneGestureRecognizer	3,607.38 us	63.466 us	59.366 us	3,612.58 us	433.5938	429.6875	15.6250	3992.92 KB
RemoveOneGestureRecognizer	2,702.21 us	224.002 us	660.474 us	2,344.16 us	292.9688	289.0625	-	2719.46 KB
ClearOneGestureRecognizer	3,647.28 us	71.943 us	129.729 us	3,693.72 us	546.8750	542.9688	7.8125	5023.58 KB

symbiogenesis · 2024-01-21T10:11:41Z

Although these changes helped a bit, and it could be merged, I will need to rework this one based on my new PR #20036

If that other PR is merged, then I can make all the ObservableCollections of GestureRecognizers into ObservableRangeCollections and do very simple AddRange() RemoveRange() and ReplaceRange() operations that will dramatically reduce CollectionChanged events.

jonathanpeppers · 2024-01-22T17:03:05Z

@symbiogenesis I just wanted to ask how you decide which method(s) to focus on for some of your recent PRs.

Are you profiling a real-world MAUI app? (with either VS, PerfView, or dotnet-trace)
Then look at methods that show time spent (wasted?) in a real app

I worry you might be optimizing some methods that aren't as impactful as others. If you see it in a real app, when profiling, can you share a screenshot of how much time it was taking?

symbiogenesis · 2024-01-22T17:17:35Z

@symbiogenesis I just wanted to ask how you decide which method(s) to focus on for some of your recent PRs.

Are you profiling a real-world MAUI app? (with either VS, PerfView, or dotnet-trace)

Then look at methods that show time spent (wasted?) in a real app

I worry you might be optimizing some methods that aren't as impactful as others. If you see it in a real app, when profiling, can you share a screenshot of how much time it was taking?

To be honest, I'm mostly trying to optimize things that seem like they will have a low probability of breaking stuff, given that it would take awhile to familiarize myself with certain higher-level concerns.

I scroll a big collection view with memory profiling and see huge numbers of objects in there. GestureRecognizers are part of that. Clearing them may not be as important as the platform-specific code or other areas. But it is a well-defined issue for a novice.

But you inspired me to be more diligent and use PerfView and the trace tool, and I will give that a go prior to further optimizations.

jonathanpeppers · 2024-01-22T17:21:11Z

If you want a feel for using https://speedscope.app, maybe open the traces in matthew.zip here:

[Android] CollectionVIew scrolling performance on MAUI lags compared to XF #18505 (comment)

You might see if you can spot how we saw {AppThemeBinding} taking time.

I also saw MauiTextView on Android spending time in OnLayout() as another example. That might not be something you'd want to work on, but I thought some of the code there could be moved to Java to avoid interop.

PureWeen · 2024-06-20T09:25:54Z

/rebase

PureWeen · 2024-06-20T09:26:40Z

/azp run

azure-pipelines · 2024-06-20T09:26:59Z

Azure Pipelines successfully started running 3 pipeline(s).

* Size and SizeF should not throw on NaN * Fix Release Versioning * Upgrade from 1.5.1 to 1.5.4 * SwipeView Fix #22580 (#22741) Co-authored-by: Javier Suárez <[email protected]> * Update vscode extension recommendations * [XC] Fix SimplifyTypeExtensionVisitor (#23043) * Add test * Fix the visitor * Add SR6 to issue template (#23071) * Make sure the main branch is using .NET 8 SDK (#23077) * Make sure the main branch is using .NET 8 SDK The main branch needs to use the .NET 8 SDK because we are building the .NET 7 TFMs as well. The .NET 9 SDK does not support .NET 7 TFMs anymore. * no previews! * Setup preview versioning for SR6.1 * Remove compat appium tests Since we're moving in a different direction for moving these over in #22635 these projects (and the Issue11853 test, since it's in the other PR) can be deleted. * [main] Update arcade and xharness (#22981) * Update arcade * Update dotnet-tools.json * Update xharness # Conflicts: # .config/dotnet-tools.json # eng/Version.Details.xml # eng/Versions.props * Remove more references to removed projects * Make titlebar button foreground colors use app theme * Update dependencies from https://github.com/dotnet/xharness build 20240612.3 (#23088) Microsoft.DotNet.XHarness.CLI , Microsoft.DotNet.XHarness.TestRunners.Common , Microsoft.DotNet.XHarness.TestRunners.Xunit From Version 9.0.0-prerelease.24311.2 -> To Version 9.0.0-prerelease.24312.3 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * Add additional logging for PopLifeCycle * Remove more legacy pipeline bits * [iOS] Fixed NRE after calling ViewCell.ForceUpdateSize (#23094) * [iOS] Fixed NRE after calling ViewCell.ForceUpdateSize * - add test --------- Co-authored-by: Shane Neuville <[email protected]> * Add x:DataType to the carousel view UI tests (#23113) * Bump to Android 34.0.113 Context: https://github.com/dotnet/android/releases/34.0.113 Changes: dotnet/android@34.0.79...34.0.113 I noticed the Android workload version used in dotnet/maui/main was a couple service releases old. We should use the latest one, as it has a memory leak fix for `Post()`. * Squashed commit of the following: (#22874) commit db8a3bd Author: Matthew Leibowitz <[email protected]> Date: Mon Jun 10 13:46:16 2024 +0800 Add android commit 0bd2d65 Author: Matthew Leibowitz <[email protected]> Date: Mon Jun 10 12:16:34 2024 +0800 oops! commit 647e5f5 Author: Matthew Leibowitz <[email protected]> Date: Sat Jun 8 18:15:40 2024 +0800 Almost commit c3ea650 Author: Matthew Leibowitz <[email protected]> Date: Sat Jun 8 16:17:58 2024 +0800 sadfasdf commit d9a398f Author: Matthew Leibowitz <[email protected]> Date: Sat Jun 8 13:56:34 2024 +0800 screenshots commit 4b3c01f Merge: 595ff03 e94b364 Author: Matthew Leibowitz <[email protected]> Date: Sat Jun 8 13:54:52 2024 +0800 Merge branch 'main' into dev/gif-in-release commit 595ff03 Author: Matthew Leibowitz <[email protected]> Date: Thu Jun 6 21:00:52 2024 +0800 Update ImageUITests.cs commit 2c20fe0 Author: Matthew Leibowitz <[email protected]> Date: Thu Jun 6 19:05:23 2024 +0800 Make some tests commit 1e82e8b Author: Matthew Leibowitz <[email protected]> Date: Thu Jun 6 18:44:18 2024 +0800 Take 2? commit d576c82 Author: Matthew Leibowitz <[email protected]> Date: Thu Jun 6 07:11:17 2024 +0800 Fix animated gifs in Release builds * Fix loaded so it fires on second subscription (#23095) * Fix loaded so it fires on second subscription * Update VisualElement.cs * Make sure the view is still alive after posting (#23114) I saw in some of my local testing the shape view may actually be disposed if you write code or when you run the device tests. When we come back after the UI loop, make sure the platform view is still alive. This should not happen if the view is attached to the UI since it is still being used, but if you happen to navigate or close some popup it may occur. * [iOS] TapGestureRecognizer should not fire when view is not enabled (#23049) * Don't receive touch when view is disabled * Add UITest for Single Tap enabled/disabled This removes the first pass unit test version which isn't really testing the scenario properly. * Fix element automation id * Don't fire tap for windows if control disabled * Make failure case text less confusing * [Tests] Update to Appium 5.0.0 (#23118) * Update to appium 5.0.0 Their driver has some removed methods (LaunchApp/CloseApp) which seem to be now implemented for windows. Also removed an extra explicit ref to system.drawing.common since it comes in transitively from appium.webdriver package (and the version was causing issues since the newer appium.webdriver uses a newer version). * Fix windows launchapp/closeapp for newer appium In Appium's dotnet driver in v5.0.0 they removed `LaunchApp` from the driver: appium/dotnet-client#766 The deprecation suggests using `ActivateApp` as an alternative, but that throws an error saying it's not implemented on the windows driver. Curiously, `CloseApp` which was also marked as deprecated was _moved_ from the base appium driver to the `WindowsDriver` here: appium/dotnet-client#773 I think the same treatment should have been done to the `LaunchApp` method since there's no alternative in windows. For now the workaround is to invoke the command manually: `windowsDriver.ExecuteScript("windows: launchApp", [_app.GetAppId()]);` * Use correct interface type in FrameRenderer (#23124) * Use correct interface type in FrameRenderer (#23124) (#23146) * Squashed commit of the following: (#19629) commit f6c3bfa Merge: d0b3f84 3143629 Author: Matthew Leibowitz <[email protected]> Date: Mon Jun 17 19:58:40 2024 +0800 Merge remote-tracking branch 'origin/main' into dev/macos-actionsheet commit d0b3f84 Author: Matthew Leibowitz <[email protected]> Date: Tue Jun 11 05:00:38 2024 +0800 Fix the older macOS testing commit 3dc6b4d Author: Matthew Leibowitz <[email protected]> Date: Mon Jun 10 23:40:40 2024 +0800 macOS 13 things commit e3e48e4 Author: Matthew Leibowitz <[email protected]> Date: Sat Jun 8 02:15:55 2024 +0800 docs commit 693d79c Merge: 3dbce49 a3c872d Author: Matthew Leibowitz <[email protected]> Date: Sat Jun 8 02:08:31 2024 +0800 Merge remote-tracking branch 'origin/main' into dev/macos-actionsheet commit 3dbce49 Merge: 17ea9c6 93a1bc4 Author: Matthew Leibowitz <[email protected]> Date: Sat Jun 8 02:06:16 2024 +0800 Merge remote-tracking branch 'origin/main' into dev/macos-actionsheet commit 17ea9c6 Author: Matthew Leibowitz <[email protected]> Date: Sat Jun 8 02:05:42 2024 +0800 Fix the tests commit 026da41 Author: Matthew Leibowitz <[email protected]> Date: Fri Jun 7 03:56:26 2024 +0800 fixes commit d2b85d5 Author: Matthew Leibowitz <[email protected]> Date: Thu Jun 6 23:58:33 2024 +0800 namespaces commit 9fa77cd Merge: 3f9596b 9d71d32 Author: Matthew Leibowitz <[email protected]> Date: Thu Jun 6 23:57:00 2024 +0800 Merge branch 'main' into dev/macos-actionsheet # Conflicts: # src/Controls/samples/Controls.Sample.UITests/Test.cs # src/Controls/src/Core/Platform/AlertManager/AlertManager.iOS.cs # src/Controls/tests/TestCases.Shared.Tests/Tests/Concepts/AlertsGalleryTests.cs # src/Controls/tests/TestCases/Concepts/AlertsGalleryPage.cs # src/TestUtils/src/UITest.Appium/AppiumCatalystApp.cs # src/TestUtils/src/UITest.Appium/HelperExtensions.cs commit 3f9596b Author: Matthew Leibowitz <[email protected]> Date: Sat Dec 30 09:29:31 2023 +0200 Add some UI tests Done: iOS/macOS/Android. TODO: Windows commit 68c930f Author: Matthew Leibowitz <[email protected]> Date: Tue Dec 19 18:15:51 2023 +0200 macOS does not use PopoverPresentationController Fixes #18156 * [ios/catalyst] fix more cycles in `NavigationPage` Fixes: #21453 Context: #22810 In #22810, a leak in `NavigationPage` was fixed for the case: Application.Current.MainPage = new NavigationPage(new Page1()); Application.Current.MainPage = new Page2(); However, it does *not* work for the case: await Navigation.PushModalAsync(new NavigationPage(new Page1())); await Navigation.PopModalAsync(); I could reproduce this problem in `MemoryTests.cs`. There were still a few cycles in `NavigationRenderer`: * `NavigationRenderer` -> `VisualElement _element` -> `NavigationRenderer` * `NavigationRenderer` -> `Page Current` -> `NavigationRenderer` * `NavigationRenderer` -> `ViewHandlerDelegator<NavigationPage> _viewHandlerWrapper` -> `TElement _element` -> `NavigationRenderer` After fixing these cycles, the test passes. `ViewHandlerDelegator` was slightly tricky, as I had to make a `_tempElement` variable and *unset* it immediately after use. * Only change ViewHandlerDelegator for iOS/Catalyst * [ios] fix leak in ListView *Cells (#23143) * [ios] fix leak in ListView *Cells Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2064274 After fixing #22867 for `CollectionView`, there was still a problem with `ListView`. The following types were leaking in the sample app: * `UIKit.UITableView` * `Microsoft.Maui.Controls.Handlers.Compatibility.ViewCellRenderer.ViewTableCell` * `Microsoft.Maui.Platform.MauiLabel` * `Microsoft.Maui.Platform.MauiImageView` * `Microsoft.Maui.Platform.WrapperView` After a lot of debugging, we found that `Cell` was holding a reference to the `UITableView`. This pointed *up* in the hierarchy, creating a cycle. I updated an existing device test to ensure the problem is solved. * Fix missing handlers in tests * Remove assertion * Fix UITableView reference * Update a different test * Can't enumerate while adding * Missed a null check * [Housekeeping] Added UI Test to validate project template (#18567) * Added template UI Test * Added pending snapshot * Updated snapshot * Generate snapshots for all the platforms * Added pending snapshots * Updated snapshot * More changes * Updated droid snapshot * More changes * Updated snapshots * Added sample path * Update Issue19509.cs * Update Issue19509.cs --------- Co-authored-by: Shane Neuville <[email protected]> * Optimize resetting gesture recognizers (#19987) * reduce LINQ usage * Remove useless check that will always be false * use HashSet * reduce enumerations and casting * reduce LINQ usage * only cast once * add benchmarks * fix build --------- Co-authored-by: Edward Miller <[email protected]> * [Windows] Fix ListView insert not working properly (#22746) * Add test * Remove force layout update during collection changed event * Adjust await * Update ListViewTests.Windows.cs --------- Co-authored-by: Mike Corsaro <[email protected]> * Fix <ApplicationTitle> encoding in maui templates (#22084) * Makes <ApplicationTitle> use a custom symbol instead of the default name parameter to ensure the name is XML encoded where necessaru * Adds new tests for various encodings and special characters --------- Co-authored-by: Eilon Lipton <[email protected]> * Fixed a null check #15102 (#23170) * [ios/catalyst] fix memory leak in TabbedPage (#23166) * [ios/catalyst] fix memory leak in TabbedPage Context: #23164 Just the same way as `NavigationPage` in #23164, `TabbedPage` also has a memory leak caused by the cycle: * `TabbedPage` -> `TabbedRenderer` -> `VisualElement _element;` -> `TabbedPage I could add a new `[Theory]` in `MemoryTests.cs` to see the issue. This PR fixes the memory leak by breaking the cycle in `TabbedRenderer`. * Ignore test on Windows I also cleaned up the Task.Delay() * [iOS] Set PlatformGraphicsView to transparent input (#23208) * Avoid JavaCast and exceptions and ask Java (#23215) * Wire RefreshView up to our xplat layout workflow (#23169) * Use better layout/measure path with refreshview * - fix naming * - set RefreshView content to maui compatible container * - add test * - fix null operator * Update Issue23029.xaml.cs * - fix content panel so it removes previous content * - add additional check * Fix Merge --------- Co-authored-by: Matthew Leibowitz <[email protected]> Co-authored-by: MartyIX <[email protected]> Co-authored-by: Jakub Florkowski <[email protected]> Co-authored-by: Javier Suárez <[email protected]> Co-authored-by: redth <[email protected]> Co-authored-by: Šimon Rozsíval <[email protected]> Co-authored-by: Rui Marinho <[email protected]> Co-authored-by: Mike Corsaro <[email protected]> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Vitaly Knyazev <[email protected]> Co-authored-by: Jonathan Peppers <[email protected]> Co-authored-by: Jonathan Dick <[email protected]> Co-authored-by: Edward Miller <[email protected]> Co-authored-by: Edward Miller <[email protected]> Co-authored-by: Mike Corsaro <[email protected]> Co-authored-by: Michael Yanni <[email protected]> Co-authored-by: Eilon Lipton <[email protected]> Co-authored-by: Tim Miller <[email protected]>

symbiogenesis requested a review from a team as a code owner January 18, 2024 21:08

symbiogenesis requested review from jfversluis and tj-devel709 January 18, 2024 21:08

ghost added the community ✨ Community Contribution label Jan 18, 2024

symbiogenesis force-pushed the hashsets branch from af55548 to 7f6c5af Compare January 18, 2024 21:47

symbiogenesis changed the title ~~use HashSet where it makes sense~~ Optimize resetting gesture recognizers Jan 18, 2024

mattleibow requested a review from jonathanpeppers January 19, 2024 07:49

jonathanpeppers reviewed Jan 19, 2024

View reviewed changes

src/Controls/src/Core/View/View.cs Outdated Show resolved Hide resolved

jonathanpeppers requested changes Jan 19, 2024

View reviewed changes

symbiogenesis force-pushed the hashsets branch 3 times, most recently from 5e0c9db to 2cf2df1 Compare January 19, 2024 19:03

jonathanpeppers reviewed Jan 19, 2024

View reviewed changes

src/Core/tests/Benchmarks/Benchmarks/GestureRecognizerBenchmarker.cs Outdated Show resolved Hide resolved

Eilon added the area-gestures Gesture types label Jan 20, 2024

symbiogenesis force-pushed the hashsets branch 2 times, most recently from 5a171b1 to 93a82eb Compare January 20, 2024 16:38

MartyIX reviewed Jan 20, 2024

View reviewed changes

src/Controls/src/Core/View/View.cs Show resolved Hide resolved

symbiogenesis force-pushed the hashsets branch from 93a82eb to 5faa49f Compare January 21, 2024 07:47

samhouts added the stale Indicates a stale issue/pr and will be closed soon label Feb 6, 2024

jonathanpeppers approved these changes Jun 17, 2024

View reviewed changes

Edward Miller added 8 commits June 20, 2024 09:26

reduce LINQ usage

cea6f70

Remove useless check that will always be false

0a0fc53

use HashSet

cc9c544

reduce enumerations and casting

2381637

reduce LINQ usage

b0f2c4f

only cast once

eb4eac8

add benchmarks

7cd4832

fix build

8012720

github-actions bot force-pushed the hashsets branch from 5faa49f to 8012720 Compare June 20, 2024 09:26

PureWeen enabled auto-merge (squash) June 21, 2024 16:39

PureWeen approved these changes Jun 21, 2024

View reviewed changes

PureWeen merged commit b7ef1e6 into dotnet:main Jun 21, 2024
47 checks passed

symbiogenesis deleted the hashsets branch June 21, 2024 22:44

PureWeen added this to the .NET 8 SR7 milestone Jul 3, 2024

samhouts added fixed-in-8.0.70 fixed-in-9.0.0-preview.6.24327.7 labels Aug 2, 2024

github-actions bot locked and limited conversation to collaborators Sep 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize resetting gesture recognizers #19987

Optimize resetting gesture recognizers #19987

symbiogenesis commented Jan 18, 2024 •

edited

Loading

ghost commented Jan 18, 2024

mattleibow commented Jan 19, 2024

azure-pipelines bot commented Jan 19, 2024

mattleibow commented Jan 19, 2024

jonathanpeppers left a comment

symbiogenesis commented Jan 21, 2024

symbiogenesis commented Jan 21, 2024 •

edited

Loading

jonathanpeppers commented Jan 22, 2024

symbiogenesis commented Jan 22, 2024 •

edited

Loading

jonathanpeppers commented Jan 22, 2024

PureWeen commented Jun 20, 2024

PureWeen commented Jun 20, 2024

azure-pipelines bot commented Jun 20, 2024

Optimize resetting gesture recognizers #19987

Optimize resetting gesture recognizers #19987

Conversation

symbiogenesis commented Jan 18, 2024 • edited Loading

ghost commented Jan 18, 2024

mattleibow commented Jan 19, 2024

azure-pipelines bot commented Jan 19, 2024

mattleibow commented Jan 19, 2024

jonathanpeppers left a comment

Choose a reason for hiding this comment

symbiogenesis commented Jan 21, 2024

symbiogenesis commented Jan 21, 2024 • edited Loading

jonathanpeppers commented Jan 22, 2024

symbiogenesis commented Jan 22, 2024 • edited Loading

jonathanpeppers commented Jan 22, 2024

PureWeen commented Jun 20, 2024

PureWeen commented Jun 20, 2024

azure-pipelines bot commented Jun 20, 2024

symbiogenesis commented Jan 18, 2024 •

edited

Loading

symbiogenesis commented Jan 21, 2024 •

edited

Loading

symbiogenesis commented Jan 22, 2024 •

edited

Loading