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

Batch CollectionChanged events #20036

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

symbiogenesis
Copy link
Contributor

@symbiogenesis symbiogenesis commented Jan 21, 2024

There are a bunch of places where there was some kind of bulk update on an ObservableCollection

By pulling in the ObservableRangeCollection from the XamarinCommunityToolkit and MvvmHelpers that is maintained by @jamesmontemagno we can see that there is some significant performance improvement to be had.

This could be done in many places across the codebase, in the future.

EDIT:

Just realized that there already was an ObservableList object that supports some range operations. It may be more suitable.

internal class ObservableList<T> : ObservableCollection<T>
{
// There's lots of special-casing optimizations that could be done here
// but right now this is only being used for tests.

@symbiogenesis symbiogenesis requested a review from a team as a code owner January 21, 2024 09:42
@ghost ghost added the community ✨ Community Contribution label Jan 21, 2024
@ghost
Copy link

ghost commented Jan 21, 2024

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.

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Jan 25, 2024

Updated this PR to add this optimization to another file.

I followed your advice, @jonathanpeppers, and did some profiling.

While running the sample app on Windows in this branch of my open source DataGrid, just scroll a lot. Or use the settings to try to toggle whether pagination is enabled, multiple times.

My speedscope results looked garbled and I couldn't get much knowledge out of PerfView except to see that there were tons and tons of threads. But Visual Studio memory profiling was intuitive. Still learning the tools.

That sample code is just using simple MAUI controls with nothing platform-specific, and shouldn't be super slow. The memory profiling showed tons of ItemTemplateContext objects and calls from ObservableItemTemplateCollection.

I already had this PR open to address this kind of issue involving ObservableCollections. So I knew that batching the CollectionChanged events was the right approach.

I could write another benchmark to try to prove that this is a better approach by orders of magnitude on large collections, but my latest update involving the ObservableItemTemplateCollection to this PR is Windows-specific and writing a Windows platform benchmark requires creating another project.

@symbiogenesis symbiogenesis marked this pull request as ready for review January 25, 2024 17:32
@AlleSchonWeg
Copy link

Hi @symbiogenesis
could you make the ObservableRangeCollection public? I missed this type of collection in MAUI. In XF we had such type in the xct.

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Jan 26, 2024

Hi @symbiogenesis could you make the ObservableRangeCollection public? I missed this type of collection in MAUI. In XF we had such type in the xct.

That would be an API change that would need to be approved by the MAUI team. If they like this PR then the code will be there for them to expose as public whenever they like.

But I need to figure out why the tests are failing. So I will try to fix that now.

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Jan 26, 2024

In reality, ObservableCollection itself should be updated to expose ranged operations, and this is one of the most-requested .NET features for years. A dev is claiming it is under consideration for .NET 9, but provided no promises.

dotnet/runtime#18087

@symbiogenesis symbiogenesis force-pushed the layout-perf branch 2 times, most recently from bb679e4 to 6072686 Compare January 26, 2024 19:24
@symbiogenesis symbiogenesis marked this pull request as draft January 26, 2024 19:29
@symbiogenesis symbiogenesis force-pushed the layout-perf branch 2 times, most recently from 87684a2 to ac0d7dc Compare January 26, 2024 20:52
@symbiogenesis symbiogenesis changed the title Batch CollectionChanged events via ObservableRangeCollection Batch CollectionChanged events Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/perf 🏎️ Startup / Runtime performance community ✨ Community Contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants