Performance improvements - #240
Merged
Merged
Conversation
…tion during sorting
…very sort/filter/paginate call
…ow to avoid O(n²)
…has active filter text
… access is already serialized
…ortFilterAndPaginate
…e by using reverse index loop
…ath instead of single-level GetProperty
…GridHeaderRow OnParentSet
…ending sort on column 0
There was a problem hiding this comment.
Pull request overview
This PR introduces several caching and lookup optimizations in the core DataGrid to reduce repeated allocations and expensive linear searches, and adds a suite of MAUI/xUnit tests in the sample app to improve coverage.
Changes:
- Added caching for reflection-based property-path access and reused that for sort/filter operations.
- Introduced item index mapping and original-items snapshot caching to avoid repeated
IndexOf/enumerations during row updates and pagination. - Added/expanded unit tests in the sample app for converters, extensions, and collection helpers.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Maui.DataGrid/Extensions/ReflectionExtensions.cs | Adds cached property-path resolution for faster GetValueByPath. |
| Maui.DataGrid/DataGridRow.cs | Avoids duplicate event handlers and replaces IndexOf with a cached index lookup. |
| Maui.DataGrid/DataGridHeaderRow.cs | Avoids repeated Any() calls when building header cells and prevents duplicate handlers. |
| Maui.DataGrid/DataGridColumn.cs | Adds a reset method to force re-resolving column data types when the source changes. |
| Maui.DataGrid/DataGrid.xaml.cs | Adds original-items caching, item-index mapping, refactors selection cleanup, and adjusts filtering behavior to use GetValueByPath. |
| Maui.DataGrid/Converters/SortDataTypeConverter.cs | Extends parsing to support `"N ASC |
| Maui.DataGrid.Sample/Tests/SortDataTest.cs | Adds unit tests for SortData semantics and equality/hash behavior. |
| Maui.DataGrid.Sample/Tests/ReflectionExtensionsTest.cs | Adds unit tests covering GetValueByPath/GetPropertyTypeByPath behavior. |
| Maui.DataGrid.Sample/Tests/ObservableRangeCollectionTest.cs | Adds unit tests for range operations and notifications. |
| Maui.DataGrid.Sample/Tests/ListExtensionsTest.cs | Adds unit tests for list/grid extension helpers. |
| Maui.DataGrid.Sample/Tests/LayoutOptionsExtensionsTest.cs | Adds unit tests for layout-options alignment conversion. |
| Maui.DataGrid.Sample/Tests/DataGridColumnTest.cs | Adds unit tests for DataGridColumn defaults and sortability detection. |
| Maui.DataGrid.Sample/Tests/ConvertersTest.cs | Adds unit tests for converters, including new SortData parsing cases. |
| Maui.DataGrid.Sample/Tests/BindablePropertyExtensionsTest.cs | Adds unit tests for bindable property helper behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+90
to
+92
| properties[i] = property; | ||
| currentType = property.PropertyType; | ||
| } |
| { | ||
| private const char PropertyOfOp = '.'; | ||
|
|
||
| private static readonly ConcurrentDictionary<(Type Type, string Path), PropertyInfo[]?> PropertyPathCache = new(); |
Comment on lines
+1225
to
+1232
| if (_internalItemsIndexMap == null) | ||
| { | ||
| _internalItemsIndexMap = new(InternalItems.Count); | ||
| for (var i = 0; i < InternalItems.Count; i++) | ||
| { | ||
| _internalItemsIndexMap[InternalItems[i]] = i; | ||
| } | ||
| } |
Comment on lines
+1268
to
1286
| if (originalItems.Count == 0) | ||
| { | ||
| PageCount = 1; | ||
| _internalItemsIndexMap = null; | ||
| InternalItems.Clear(); | ||
| return; | ||
| } | ||
|
|
||
| var filteredItems = CanFilter() ? GetFilteredItems(originalItems) : originalItems; | ||
| var filteredItems = CanFilter() ? GetFilteredItems(originalItems) : originalItems; | ||
|
|
||
| var sortedItems = CanSort(sortData) ? GetSortedItems(filteredItems, sortData!) : filteredItems; | ||
| var sortedItems = CanSort(sortData) ? GetSortedItems(filteredItems, sortData!) : filteredItems; | ||
|
|
||
| var paginatedItems = PaginationEnabled ? GetPaginatedItems(sortedItems) : sortedItems; | ||
| var paginatedItems = PaginationEnabled ? GetPaginatedItems(sortedItems) : sortedItems; | ||
|
|
||
| PageCount = (int)Math.Ceiling(filteredItems.Count / (double)PageSize); | ||
| PageCount = (int)Math.Ceiling(filteredItems.Count / (double)PageSize); | ||
|
|
||
| InternalItems.ReplaceRange(paginatedItems); | ||
| } | ||
| _internalItemsIndexMap = null; | ||
| InternalItems.ReplaceRange(paginatedItems); | ||
| } |
Comment on lines
+1541
to
+1547
| if (string.IsNullOrEmpty(column.FilterText)) | ||
| { | ||
| Debug.WriteLine(ex); | ||
| return false; | ||
| return true; | ||
| } | ||
| #pragma warning restore CA1031 // Do not catch general exception types | ||
|
|
||
| var value = item.GetValueByPath(column.PropertyName)?.ToString(); | ||
| return value?.Contains(column.FilterText, StringComparison.OrdinalIgnoreCase) == true; |
Collaborator
|
Nice work. This looks good to me. I think I will do a follow-up PR to improve the thread-safety without reintroducing locks, although nothing in here is incorrect. The performance looks good, when I run it. Found some stuff with AI, but I will clean it all up. And there's stuff I should do to make the build and tests work nicely. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I've go through the project and introduced some caching for performance improvements. In addition to that I've also improved the test coverage.