[net11.0] Improve TypedBinding performance#32382
[net11.0] Improve TypedBinding performance#32382simonrozsival wants to merge 2 commits intonet11.0from
Conversation
|
Just awesome. This will have a huge impact on all apps! |
simonrozsival
changed the title
simonrozsival
force-pushed
the
dev/srozsival/binding-improvements
branch
from
eb36fe4 to
9046e23
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the TypedBinding<TSource, TProperty> class to introduce a new, more efficient constructor and internal architecture for handling property change notifications. The main changes include:
- Introduces a new constructor that accepts a handlers function instead of a pre-built array, improving performance by deferring handler evaluation
- Adds internal
IPropertyChangeHandlerinterface with two implementations:LegacyPropertyChangeHandler(maintains backward compatibility) andPropertyChangeHandler(new efficient implementation) - Updates benchmark code to test both old and new approaches
- Adds comprehensive unit tests for the new constructor in
TypedBinding2UnitTests.cs - Removes the
TypedBinding.ForSingleNestingLevelfactory method and updates its single usage
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TypedBinding.cs | Core refactoring: removes static factory, adds new constructor with handler function parameter, introduces handler abstraction with two implementations |
| TypedBindingBenchmarker.cs | Expands benchmarks to compare old vs new binding approaches across multiple scenarios (simple, nested, indexed, two-way) |
| TypedBinding2UnitTests.cs | New comprehensive test file covering all binding scenarios with the new constructor |
| TypedBindingUnitTests.cs | Updates reflection code to access handlers through new _propertyChangeHandler field |
| TemplatedItemsList.cs | Updates single usage of removed factory method to use new constructor directly |
| BindingExpressionHelper.cs | Minor nullable annotation fix |
Comments suppressed due to low confidence (1)
src/Controls/tests/Core.UnitTests/TypedBinding2UnitTests.cs:1
- This reflection code accesses fields on what appears to be the new
PropertyChangeHandlerclass, but that class doesn't have a_handlersfield - it has_listeners. This code will fail at runtime if the new constructor path is taken. The test should be updated to work with both handler implementations or check which implementation is in use.
using System;
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@copilot there as a UI test failing and I am not sure if it can be related to these changes or not: Analyze this scenario and correlate it to the changes in this PR. Suggest improvements. |
|
@simonrozsival I've opened a new pull request, #32642, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@StephaneDelcroix I think it would be better to introduce this only in .NET 11 and not in .NET 10 SR. We might need to wait until there's a net11 branch. I also noticed I have not added the new public constructor to the Unshipped Public API documents... if this goes into .NET 11, I will do that in a separate PR. |
|
/rebase |
github-actions
bot
force-pushed
the
dev/srozsival/binding-improvements
branch
from
9046e23 to
179d216
Compare
you can wrap this code in #if NET_11_0_OR_GREATER |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
simonrozsival
force-pushed
the
dev/srozsival/binding-improvements
branch
from
179d216 to
a22c6a0
Compare
simonrozsival
changed the title
|
/azp run |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
/rebase |
simonrozsival
force-pushed
the
dev/srozsival/binding-improvements
branch
2 times, most recently
from
89166be to
6cf43dc
Compare
|
|
|
Hi Simon, Thanks for the update! I wanted to highlight a regression introduced by this change that affects TypedBinding with nested property paths. In previous MAUI versions, TypedBinding would re-establish INotifyPropertyChanged subscriptions whenever any object along the binding path changed, even if the binding had already been initialized once. This is essential for scenarios where the parent object starts as null, later becomes non-null, and then one of its nested objects changes. With this PR, when the binding engine detects that a subscription has already been created during binding initialization, it skips re-subscribing to PropertyChanged on updated source objects. As a result:
Changes: Thanks! |
StephaneDelcroix
force-pushed
the
dev/srozsival/binding-improvements
branch
from
0e74370 to
aa710f6
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 32382Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 32382" |
…ate becomes non-null When an intermediate object in a TypedBinding path starts as null and later becomes non-null (or is replaced with a different object), the binding must re-establish INPC subscriptions to the new object's nested properties. The previous optimization used a '_isSubscribed' boolean flag to skip IPropertyChangeHandler.Subscribe() after the first Apply. This prevented re-subscribing when intermediate objects changed. For example: vm.Child = null // subscribe to vm, but NOT vm.Child.Name (null) vm.Child = new Child() // Subscribe skipped due to _isSubscribed=true vm.Child.Name = "x" // binding never fires — regression! The fix removes '_isSubscribed' and always calls Subscribe on each Apply. Both IPropertyChangeHandler implementations (PropertyChangeHandler and LegacyPropertyChangeHandler) are already idempotent — they use reference equality checks to avoid re-subscribing to unchanged objects — so calling Subscribe on every Apply is safe and has minimal overhead. Adds two regression tests: - TypedBinding_NestedProperty_ResubscribesAfterNullIntermediateBecomesNonNull - TypedBinding_NestedProperty_ResubscribesAfterIntermediateReplaced Reported in: #32382 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@simonrozsival I think this should also be merged into main/inflight branches? |
|
@lucastitus I don't intend to backport this to .NET 10 servicing release unless there's a good reason to do so. |
|
@simonrozsival You've already merged your initial commits: acc2476 However you did not merge the recent null intermediary binding fix I flagged. Any upcoming .NET 10 servicing releases will have the binding issues mentioned currently. |
|
@simonrozsival As expected, this regression has made its way into release 10.0.50 |
|
@lucastitus can you please open an issue with a repro project? or is there one open already? |
simonrozsival
force-pushed
the
dev/srozsival/binding-improvements
branch
from
5d6e5a2 to
03ba0b7
Compare
…ate becomes non-null When an intermediate object in a TypedBinding path starts as null and later becomes non-null (or is replaced with a different object), the binding must re-establish INPC subscriptions to the new object's nested properties. The previous optimization used a '_isSubscribed' boolean flag to skip IPropertyChangeHandler.Subscribe() after the first Apply. This prevented re-subscribing when intermediate objects changed. For example: vm.Child = null // subscribe to vm, but NOT vm.Child.Name (null) vm.Child = new Child() // Subscribe skipped due to _isSubscribed=true vm.Child.Name = "x" // binding never fires — regression! The fix removes '_isSubscribed' and always calls Subscribe on each Apply. Both IPropertyChangeHandler implementations (PropertyChangeHandler and LegacyPropertyChangeHandler) are already idempotent — they use reference equality checks to avoid re-subscribing to unchanged objects — so calling Subscribe on every Apply is safe and has minimal overhead. Adds two regression tests: - TypedBinding_NestedProperty_ResubscribesAfterNullIntermediateBecomesNonNull - TypedBinding_NestedProperty_ResubscribesAfterIntermediateReplaced Reported in: #32382 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ate becomes non-null When an intermediate object in a TypedBinding path starts as null and later becomes non-null (or is replaced with a different object), the binding must re-establish INPC subscriptions to the new object's nested properties. The previous optimization used a '_isSubscribed' boolean flag to skip IPropertyChangeHandler.Subscribe() after the first Apply. This prevented re-subscribing when intermediate objects changed. For example: vm.Child = null // subscribe to vm, but NOT vm.Child.Name (null) vm.Child = new Child() // Subscribe skipped due to _isSubscribed=true vm.Child.Name = "x" // binding never fires — regression! The fix removes '_isSubscribed' and always calls Subscribe on each Apply. Both IPropertyChangeHandler implementations (PropertyChangeHandler and LegacyPropertyChangeHandler) are already idempotent — they use reference equality checks to avoid re-subscribing to unchanged objects — so calling Subscribe on every Apply is safe and has minimal overhead. Adds two regression tests: - TypedBinding_NestedProperty_ResubscribesAfterNullIntermediateBecomesNonNull - TypedBinding_NestedProperty_ResubscribesAfterIntermediateReplaced Reported in: #32382 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ate becomes non-null When an intermediate object in a TypedBinding path starts as null and later becomes non-null (or is replaced with a different object), the binding must re-establish INPC subscriptions to the new object's nested properties. The previous optimization used a '_isSubscribed' boolean flag to skip IPropertyChangeHandler.Subscribe() after the first Apply. This prevented re-subscribing when intermediate objects changed. For example: vm.Child = null // subscribe to vm, but NOT vm.Child.Name (null) vm.Child = new Child() // Subscribe skipped due to _isSubscribed=true vm.Child.Name = "x" // binding never fires — regression! The fix removes '_isSubscribed' and always calls Subscribe on each Apply. Both IPropertyChangeHandler implementations (PropertyChangeHandler and LegacyPropertyChangeHandler) are already idempotent — they use reference equality checks to avoid re-subscribing to unchanged objects — so calling Subscribe on every Apply is safe and has minimal overhead. Adds two regression tests: - TypedBinding_NestedProperty_ResubscribesAfterNullIntermediateBecomesNonNull - TypedBinding_NestedProperty_ResubscribesAfterIntermediateReplaced Reported in: #32382 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Squashed 26 commits for clean rebase onto net11.0.
Copy-paste error: 6 field assignments were duplicated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
force-pushed
the
dev/srozsival/binding-improvements
branch
from
08f94d7 to
b687e4c
Compare
|
@StephaneDelcroix @jfversluis could you please have a look at this PR and let me know what you think about it? could we merge this into net11.0 soon? |
|
|
Description
This PR improves the source-generated
TypedBindingimplementation with several enhancements:1. New
TypedBindingconstructor withhandlersCount+GetHandlersdelegateReplaces the old constructor that accepted a fixed-size
Tuple<Func<>, string>[]with a new one taking:int handlersCount— pre-computed count of property-change handlersFunc<TSource, IEnumerable<(INotifyPropertyChanged, string)>>? getHandlers— lazy handler enumerationThis eliminates closure allocations and avoids eagerly creating handler arrays at binding setup time.
2. INPC-aware handler generation
The source generators (both C#
BindingSourceGenand XAMLSourceGen) now analyze types at compile time to determine their INPC status:AllInterfaces→ directyield returnwithout runtime checkif (x is INPC p) yield return ...3. Dead-code elimination in generated handlers
When the remaining path parts will not produce any
yield returnstatements (e.g., because the type is a struct), the generator now skips emitting thevar pN = expr;assignment. This also eliminates the correspondingUnsafeAccessormethod that would have been generated for the dead variable.4. Cast combination & conditional access propagation
as X) following member access are combined into a single variable assignment?.) is properly propagated after nullable casts5. Benchmark pragma fixes
Added
#pragma warning disable RS0030to benchmark files to allow comparing old vs newTypedBindingconstructors (old constructor is banned viaeng/BannedSymbols.txt).Benchmark Results
Steady-state (property value already applied)
Setup path (binding creation + first apply)
The new implementation is 2-3x faster at steady state and ~2x faster on the setup path compared to the old TypedBinding, while also reducing memory allocations.