-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
base: main
Are you sure you want to change the base?
Conversation
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. |
f128c8e
to
7ec5f5a
Compare
7ec5f5a
to
55fbfbc
Compare
a4fd87d
to
07f9c15
Compare
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 I already had this PR open to address this kind of issue involving 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. |
Hi @symbiogenesis |
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. |
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. |
bb679e4
to
6072686
Compare
87684a2
to
ac0d7dc
Compare
ac0d7dc
to
5a33a26
Compare
This all looks fine to me, thoughts @PureWeen? |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
src/Controls/src/Core/Platform/Windows/CollectionView/ObservableItemTemplateCollection.cs
Outdated
Show resolved
Hide resolved
54b0271
to
29fc617
Compare
Need to rerun tests |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Is this good to merge? |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have some kind of benchmark to show numbers of performance improvements ?
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.
maui/src/Controls/src/Core/ObservableList.cs
Lines 10 to 14 in c02195d