[Perf] Reduce allocations in inherited BindingContext propagation - #34135
[Perf] Reduce allocations in inherited BindingContext propagation#34135simonrozsival wants to merge 3 commits into
Conversation
Fixes #34129 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
6453f68 to
dcd3a94
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34135Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34135" |
There was a problem hiding this comment.
Pull request overview
This PR targets a hot path in the Controls binding engine (BindableObject.SetInheritedBindingContext) to reduce per-element allocations when BindingContext propagates down the visual tree.
Changes:
- Reuse an existing
_inheritedContextWeakReferenceby updating.Targetinstead of allocating a new instance each time. - Remove the
_properties.Values.ToArray()snapshot inApplyBindingsand enumerate_properties.Valuesdirectly.
| var prop = _properties.Values.ToArray(); | ||
|
|
||
| for (int i = 0, propLength = prop.Length; i < propLength; i++) | ||
| foreach (var context in _properties.Values) |
There was a problem hiding this comment.
Iterating directly over _properties.Values here is not safe: ApplyBinding ultimately calls into BindingBase.Apply/BindingExpression.ApplyCore, which can invoke SetValueCore and property-changed callbacks. Those callbacks can add/remove bindable property contexts (modifying _properties) and will cause the dictionary enumerator to throw InvalidOperationException mid-iteration. The previous ToArray() snapshot avoided this; consider keeping a snapshot (potentially via pooled/rented array) or using a guarded pattern (e.g., retry or fallback snapshot when a modification is detected) so BindingContext propagation can’t crash on re-entrant property changes.
| foreach (var context in _properties.Values) | |
| var contexts = _properties.Values.ToArray(); | |
| foreach (var context in contexts) |
| var prop = _properties.Values.ToArray(); | ||
|
|
||
| for (int i = 0, propLength = prop.Length; i < propLength; i++) | ||
| foreach (var context in _properties.Values) |
There was a problem hiding this comment.
This change alters ApplyBindings iteration semantics in a way that can be sensitive to re-entrant property changes. It would be helpful to add a unit test which triggers a binding application that, via a propertyChanged callback (or similar), adds/removes a different BindableProperty context while ApplyBindings is running, and asserts no InvalidOperationException is thrown.
| foreach (var context in _properties.Values) | |
| // Take a snapshot of the current property contexts so that re-entrant | |
| // modifications to the _properties collection (e.g., via propertyChanged | |
| // callbacks triggered during ApplyBinding) do not invalidate the enumerator. | |
| var propertyContexts = _properties.Values.ToArray(); | |
| foreach (var context in propertyContexts) |
ApplyBinding can trigger property-changed callbacks that modify _properties mid-iteration. Keep the snapshot to avoid InvalidOperationException. Added explanatory comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Closing — after reverting the unsafe ToArray removal, the remaining WeakReference reuse alone isn't impactful enough to justify the PR. May revisit as part of a broader BindingContext optimization effort. |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Fixes #34129
Description
Two allocation reductions on the hot
SetInheritedBindingContextpath (called recursively for every descendant when BindingContext changes):1. Reuse existing
WeakReferenceInstead of
new WeakReference(value)on every descendant, reuse the existingWeakReferenceby updating its.Targetproperty.2. Eliminate
.ToArray()inApplyBindingsApplyBindingswas calling.ToArray()on the properties dictionary values before iterating. SinceApplyBindingonly reads bindings and does not modify the dictionary, directforeachiteration is safe and avoids the array allocation.