[Perf] Cache PropertyChanged/ChangingEventArgs on BindableProperty - #34136
[Perf] Cache PropertyChanged/ChangingEventArgs on BindableProperty#34136simonrozsival wants to merge 4 commits into
Conversation
simonrozsival
added
perf/general
fbd80fc to
37f3a7e
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34136Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34136" |
simonrozsival
changed the title
There was a problem hiding this comment.
Pull request overview
This PR targets allocation/performance hot paths in BindableObject.SetValue/ClearValue by caching PropertyChangedEventArgs and PropertyChangingEventArgs per BindableProperty, so repeated property updates don’t allocate new event-args instances.
Changes:
- Add lazy cached
PropertyChangedEventArgs/PropertyChangingEventArgsinstances onBindableProperty. - Add new
BindableObjectoverloads that raise events using pre-allocated event args. - Update
SetValueActual,OnBindablePropertySet, andClearValueCoreto use cached event args.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Controls/src/Core/BindableProperty.cs | Adds lazy per-property cached event-args accessors. |
| src/Controls/src/Core/BindableObject.cs | Uses cached event args via new overloads during SetValue/ClearValue event raising. |
| { | ||
| property.PropertyChanging?.Invoke(this, original.Value, newValue); | ||
| OnPropertyChanging(property.PropertyName); | ||
| OnPropertyChanging(property.CachedPropertyChangingEventArgs); |
There was a problem hiding this comment.
These calls now use the new internal overload OnPropertyChanging(PropertyChangingEventArgs) instead of the existing protected virtual OnPropertyChanging(string). That bypasses overrides in derived types (e.g., Cell.OnPropertyChanging) and can change behavior during SetValue/ClearValue. Keep invoking the virtual OnPropertyChanging(string) hook (or refactor so the cached args path still routes through the virtual method) so derived behavior remains intact while avoiding allocations.
| OnPropertyChanging(property.CachedPropertyChangingEventArgs); | |
| OnPropertyChanging(property.PropertyName); |
| property.PropertyChanging?.Invoke(this, original, value); | ||
|
|
||
| OnPropertyChanging(property.PropertyName); | ||
| OnPropertyChanging(property.CachedPropertyChangingEventArgs); |
There was a problem hiding this comment.
Using OnPropertyChanging(property.CachedPropertyChangingEventArgs) calls the new non-virtual overload and bypasses protected virtual OnPropertyChanging(string) overrides in derived classes. This is a behavioral breaking change (e.g., Cell relies on OnPropertyChanging("Parent")). The allocation optimization needs to preserve the virtual hook; consider routing cached args through the existing virtual path (or introducing a new virtual core method that derived types can override without breaking existing overrides).
| OnPropertyChanging(property.CachedPropertyChangingEventArgs); | |
| OnPropertyChanging(property.PropertyName); |
| if (willFirePropertyChanged) | ||
| { | ||
| OnPropertyChanged(property.PropertyName); | ||
| OnPropertyChanged(property.CachedPropertyChangedEventArgs); |
There was a problem hiding this comment.
OnBindablePropertySet now calls the new internal OnPropertyChanged(PropertyChangedEventArgs) overload, which directly raises PropertyChanged and bypasses the existing protected virtual OnPropertyChanged(string) hook. Many controls (notably Element) override OnPropertyChanged(string) for important behavior (effects, handler updates), so this changes runtime semantics and will likely break tests. The cached-args optimization should still invoke the virtual OnPropertyChanged(string) override chain (or otherwise preserve that extensibility point) while avoiding per-call allocations.
| OnPropertyChanged(property.CachedPropertyChangedEventArgs); | |
| OnPropertyChanged(property.PropertyName); |
…bleProperty Fixes #34092 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… caching The Element.cs change (_currentPropertyBeingSet tracking) was fragile. Keep the core optimization: cache PropertyChanged/ChangingEventArgs on BindableProperty and use them from BindableObject.SetValueCore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8697409 to
896c058
Compare
The previous implementation bypassed OnPropertyChanged/OnPropertyChanging virtual overrides in 20+ derived classes. This fix moves the caching to static dictionaries keyed by property name, and uses them inside the existing virtual methods so derived class overrides are still called. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@mattleibow @StephaneDelcroix 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? |
|
|
||
| internal ValidateValueDelegate ValidateValue { get; private set; } | ||
|
|
||
| private static readonly ConcurrentDictionary<string, PropertyChangedEventArgs> s_changedArgsCache = new(); |
There was a problem hiding this comment.
I believe we should set up a max value here. For Maui properties, this can be fixed size and will not cause any memory pressure, but for apps, the large ones mainly, with custom controls and different property names, it can cause a memory pressure. In the wild, I can say that people choose funny names for their properties; for example, instead of choosing Text as a property name, it can be Title, TitleText, etc., causing this collection to grow and may cause a memory pressure at some point.
|
/review -b feature/refactor-copilot-yml |
This comment has been minimized.
This comment has been minimized.
AI code review for net11.0 targetVerdict: Needs discussion (one subtle behavior change to confirm; otherwise a clean, well-scoped perf change) This is an independent review (diff-first, then reconciled with the PR narrative). It is not an approval — a human still needs to sign off. What the PR doesCaches Findings
CIAll required Confidence: high on the analysis; the only open item is the |
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
NEEDS_DISCUSSION (confidence: medium) — multi-model review (3 ND, 1 NC); CI green.
This is a clean, well-scoped perf change: it caches PropertyChanged/PropertyChanging event args by name in two static ConcurrentDictionarys and reuses them from the hot OnPropertyChanged/OnPropertyChanging paths, cutting per-notification allocations.
Points to resolve before merge
- (warning) Null property name throws.
GetOrAdd(propertyName, …)rejects null keys; raising a change notification with a null/empty name (the .NET "all properties changed" convention) would now throwArgumentNullExceptionwhere it previously didn't. Add a null guard. - (warning) Unbounded static-cache growth. Keyed by arbitrary strings off the public API, the cache can grow without bound for dynamically-named properties (indexers, generated names). Consider bounding it or restricting caching to known
BindablePropertynames. (3/4 models flagged this; gemini rated it higher.)
Otherwise the optimization is sound and tests/CI are green.
| private static readonly ConcurrentDictionary<string, PropertyChangingEventArgs> s_changingArgsCache = new(); | ||
|
|
||
| internal static PropertyChangedEventArgs GetCachedPropertyChangedEventArgs(string propertyName) | ||
| => s_changedArgsCache.GetOrAdd(propertyName, static name => new PropertyChangedEventArgs(name)); |
There was a problem hiding this comment.
ConcurrentDictionary<string,T>.GetOrAdd(propertyName, ...) throws ArgumentNullException when propertyName is null. OnPropertyChanged/OnPropertyChanging are protected virtual and, by .NET convention, can be raised with a null/empty property name to signal "all properties changed". Before this change that path was harmless; now it would throw. Please null-guard propertyName before the cache lookup (e.g. fall back to new PropertyChangedEventArgs(propertyName) when null).
|
|
||
| internal ValidateValueDelegate ValidateValue { get; private set; } | ||
|
|
||
| private static readonly ConcurrentDictionary<string, PropertyChangedEventArgs> s_changedArgsCache = new(); |
There was a problem hiding this comment.
These caches are static (app-lifetime) and keyed by arbitrary strings flowing through the public OnPropertyChanged/OnPropertyChanging surface. For dynamically-named properties (e.g. indexer notifications like Item[0], Item[1], … or generated names) this grows unboundedly for the life of the process. Consider bounding the cache, or only caching for known BindableProperty.PropertyName values rather than arbitrary strings. (Raised by 3 of the 4 review models.)
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
added
s/agent-fix-win
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MauiBot
removed
the
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
| internal static PropertyChangedEventArgs GetCachedPropertyChangedEventArgs(string propertyName) | ||
| => s_changedArgsCache.GetOrAdd(propertyName, static name => new PropertyChangedEventArgs(name)); | ||
|
|
||
| internal static PropertyChangingEventArgs GetCachedPropertyChangingEventArgs(string propertyName) |
There was a problem hiding this comment.
[major] Memory Leak Prevention / Performance — These static caches are populated through the protected BindableObject.OnPropertyChanged(string) / OnPropertyChanging(string) hooks, so app or derived-control code can pass arbitrary property-name strings. Dynamic names such as indexer notifications (Item[123]) or generated names would now be retained for the app lifetime, whereas before the event args were short-lived. Since the optimization only needs known BindableProperty.PropertyName notifications, please scope caching to those known properties or avoid caching arbitrary protected-API strings.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@simonrozsival — new AI review results are available based on this last commit:
e3e616a. To request a fresh review after new comments or commits, comment/review rerun.
Review Sessions — click to expand
Gate — Test Before & After Fix
Gate Result: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
Pre-Flight — Context & Validation
Issue: #34092 - [Perf] Cache PropertyChangedEventArgs and PropertyChangingEventArgs on BindableProperty
PR: #34136 - [Perf] Cache PropertyChanged/ChangingEventArgs on BindableProperty
Platforms Affected: all platforms; requested test platform android
Files Changed: 2 implementation, 0 test
Key Findings
- PR optimizes hot-path
BindableObjectnotifications by cachingPropertyChangedEventArgsandPropertyChangingEventArgs. - Current PR revision preserves the protected virtual string hooks, addressing earlier override-bypass comments.
- Current PR still routes null protected notifications through
ConcurrentDictionary.GetOrAdd(null, ...), which throws where prior behavior allowed null. - Current PR uses app-lifetime static dictionaries keyed by arbitrary protected API strings rather than only known
BindablePropertynames. - Gate was already skipped by the caller because no tests were detected; no gate re-run was performed.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: low
Errors: 2 | Warnings: 0 | Suggestions: 0
Key code review findings:
- ❌
src/Controls/src/Core/BindableProperty.cs:265-269— null property names now throw throughConcurrentDictionary.GetOrAdd. - ❌
src/Controls/src/Core/BindableProperty.cs:262-263— static caches retain arbitrary protected API property-name strings for app lifetime.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34136 | Static ConcurrentDictionary<string, EventArgs> caches used from protected notification hooks |
BindableObject.cs, BindableProperty.cs |
Original PR; unresolved null-key and arbitrary-string retention concerns |
Code Review — Deep Analysis
Code Review — PR #34136
Independent Assessment
What this changes: Caches PropertyChangedEventArgs / PropertyChangingEventArgs in static dictionaries keyed by property name and uses those cached args from BindableObject.OnPropertyChanged / OnPropertyChanging.
Inferred motivation: Avoid hot-path allocations during BindableObject.SetValue / ClearValue notifications.
Reconciliation with PR Narrative
Author claims: The PR eliminates repeated event-args allocations for bindable property notifications.
Agreement/disagreement: The optimization target is valid, and the current revision preserves the protected virtual string hooks. However, the implementation changes null-name protected API behavior and retains arbitrary protected API property-name strings for app lifetime.
Prior Review Reconciliation
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
Bypassed virtual OnPropertyChanged / OnPropertyChanging overrides |
Copilot inline review | ✅ Fixed | Current BindableObject.cs still invokes the protected virtual string methods from bindable property set/clear paths. |
Null propertyName now throws |
Prior AI review / inline comment | ❌ Unresolved | BindableProperty.GetCachedPropertyChangedEventArgs(null) and GetCachedPropertyChangingEventArgs(null) call ConcurrentDictionary.GetOrAdd(null, ...). |
| Static cache grows for arbitrary protected API strings | Prior AI review / inline comment | ❌ Unresolved | Static dictionaries are keyed by any string passed to protected OnPropertyChanged(string) / OnPropertyChanging(string), not only BindableProperty.PropertyName. |
Blast Radius Assessment
- Runs for all instances: yes — base
BindableObjectnotification hooks are shared by all Controls types. - Startup impact: no direct startup path, but global notification behavior changes broadly.
- Static/shared state: yes — app-lifetime static caches in
BindableProperty.
CI Status
- Required-check result: undetermined locally because
ghis unauthenticated; public PR metadata/comments were accessible. - Classification: undetermined.
- Action taken: confidence capped low; no LGTM.
Findings
❌ Error — Null protected property notifications now throw
OnPropertyChanged([CallerMemberName] string propertyName = null) and OnPropertyChanging(... = null) previously accepted null because new PropertyChangedEventArgs(null) / new PropertyChangingEventArgs(null) is valid and follows .NET's all-properties-changed convention. The PR now routes null through ConcurrentDictionary.GetOrAdd, which throws ArgumentNullException.
❌ Error — Static cache retains arbitrary protected API property names
The cache is static and keyed by strings from protected virtual APIs. Dynamic names such as indexer notifications or generated names can be retained for the process lifetime, even though the issue only needs optimization for known BindableProperty notifications.
Failure-Mode Probing
OnPropertyChanged(null)from a user subclass: throws before raising the event.- Dynamic/indexer-like names: each unique string is retained globally.
- Normal bindable property set: virtual dispatch is preserved in the current revision.
- No subscribers: null-conditional event invocation prevents helper evaluation, so no args are created.
Verdict: NEEDS_CHANGES
Confidence: low
Summary: The performance goal is sound, but the current static arbitrary-string cache changes protected API behavior and can retain unbounded dynamic names. Alternative fixes should target actual BindableProperty notifications while preserving protected virtual hook semantics.
Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Keep PR static dictionaries; add null fast-paths | ✅ PASS | 1 file | Fixes null crash, but leaves arbitrary global string retention unchanged |
| 2 | try-fix | Keep PR static dictionaries; add null fast-paths and cap each cache at 1024 entries | ✅ PASS | 1 file | Bounds retention, but still caches arbitrary protected API strings globally and adds miss-path count logic |
| 3 | try-fix | Cache args on each BindableProperty; use per-BindableObject scoped current-property fields while preserving virtual hooks |
✅ PASS | 2 files | Avoids global arbitrary-string cache and null crash; adds two reference fields to every BindableObject |
| 4 | try-fix | Cache args on each BindableProperty; use [ThreadStatic] scoped current-property context while preserving virtual hooks |
✅ PASS | 2 files | Selected: avoids global retention, null crash, per-object field tax, and lambda allocations |
| PR | PR #34136 | Static ConcurrentDictionary<string, EventArgs> caches from protected notification hooks |
2 files | Original PR; unresolved null-key and arbitrary-string retention concerns |
Cross-Pollination
| Model/Reviewer | Round | New Ideas? | Details |
|---|---|---|---|
| code-review + maui-expert-reviewer | Pre-flight | Yes | Identified null-key crash and static arbitrary-string retention as root concerns |
| maui-expert-reviewer | Candidate generation | Yes | Proposed per-BindableProperty scoped context, weak-key cache, bounded static cache, known-BP-only caching, per-instance tiny cache, and conservative null-only fix |
| Test loop | try-fix-1 | Yes | Initial --no-restore run was blocked by missing assets; restore-enabled rerun passed, but approach did not address retention |
| Test loop | try-fix-2 | Yes | Bounded global cache passed but still retains arbitrary strings up to the cap and can be cache-polluted |
| Test loop | try-fix-3 | Yes | Per-BP scoped cache passed and fixed PR defects, but introduced per-object memory overhead |
| maui-expert-reviewer | try-fix-4 final review | No blocking issue | Confirmed Controls diff is better than the PR's static dictionary approach; requested targeted tests before merge |
Exhausted: Yes — explored conservative null-only static cache, bounded static cache, per-object scoped per-BindableProperty cache, and thread-scoped per-BindableProperty cache. Remaining alternatives are either weaker variants (weak-key/string cache, per-instance tiny cache) or tuning choices rather than better correctness approaches.
Selected Fix: Candidate #4 — caches only on actual BindableProperty instances, avoids arbitrary global string retention and null-key crashes, preserves protected virtual hooks, avoids per-BindableObject fields, and passed the focused Controls core regression slice.
Test Notes
- Gate was not rerun per instruction; existing gate result was skipped because no tests were detected in the PR.
- Candidate tests used:
dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~BindableObjectUnitTests|FullyQualifiedName~BindablePropertyUnitTests". - Requested platform was Android, but this PR changes shared Controls notification infrastructure and no Android/device tests were detected. Android-specific execution was not available in this Linux environment, so validation used shared Controls unit tests only.
Recommended PR Title & Description
Recommended title
Cache BindableProperty notification event args
Recommended description
Caches property changed/changing event args for BindableProperty-driven notifications while preserving direct protected notification behavior.
- Store cached PropertyChangedEventArgs and PropertyChangingEventArgs on each BindableProperty instead of global string-keyed caches.
- Scope bindable-property notifications so the protected virtual OnPropertyChanged and OnPropertyChanging hooks still run.
- Fall back to fresh event args for direct protected calls, including null or dynamic property names.
Report — Final Recommendation
Comparative Report — PR #34136
Candidate ranking
| Rank | Candidate | Regression result | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
✅ PASS by equivalence to try-fix-4 |
Best option. Applies the expert review feedback to the PR's performance goal: caches only known BindableProperty notifications, preserves protected virtual hooks, preserves null/direct protected-call behavior, avoids app-lifetime arbitrary string retention, and avoids per-object fields. |
| 2 | try-fix-4 |
✅ PASS | Same concrete implementation shape as pr-plus-reviewer: per-BindableProperty cached args plus thread-static scoped context. It is the strongest standalone try-fix candidate and was expert-reviewed with no blocker, but ranks below pr-plus-reviewer only because the reviewer-applied PR candidate represents the same fix integrated back into the PR path. |
| 3 | try-fix-3 |
✅ PASS | Correctly scopes caching to actual BindableProperty instances and preserves null/direct-call behavior, but adds two reference fields to every BindableObject, which is a broader memory cost than try-fix-4/pr-plus-reviewer. |
| 4 | try-fix-2 |
✅ PASS | Fixes null behavior and bounds cache growth, but still caches arbitrary protected-API strings globally up to the cap and adds cache-size checks on misses. This is safer than the raw PR but weaker than scoping cache entries to real bindable properties. |
| 5 | try-fix-1 |
✅ PASS after restore-enabled rerun | Fixes the null crash only. It leaves the raw PR's unbounded app-lifetime retention of arbitrary protected API strings unchanged. |
| 6 | pr |
Raw PR fix has no detected tests and still has two expert-confirmed blockers: null protected notifications throw, and arbitrary protected API strings are retained globally for app lifetime. |
Winning candidate
pr-plus-reviewer wins. It preserves the PR's intended allocation reduction while directly addressing both actionable expert findings. No candidate with failed regression tests outranked a passing candidate; all try-fix candidates that passed still rank above the skipped raw PR, and the winning candidate is backed by the passed try-fix-4 result because it uses the same scoped per-BindableProperty approach.
Recommendation
Update PR #34136 from the raw static-dictionary implementation to the pr-plus-reviewer / try-fix-4 scoped caching design, and add focused unit tests for null protected notifications and direct protected notifications with dynamic names before merge.
Future Action — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
|
The concern about indefinitely caching event args for properties that only fire once makes sense as a follow-up tradeoff to keep an eye on. I don't think we should complicate this PR with eviction yet unless we see evidence that one-shot property names meaningfully accumulate in real apps. The current change keeps the implementation simple and removes allocation churn for the common hot paths. If this does show up as retained memory in profiling, two reasonable follow-up designs would be:
A time window on the order of tens of milliseconds, e.g. ~50ms, might be enough to cover bursty UI activity without keeping rare entries indefinitely, but I would prefer to validate that with allocation/retention data before adding the extra complexity. |
github-actions
Bot
added
the
s/agent-ready-for-rerun
github-actions
Bot
removed
the
s/agent-ready-for-rerun
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
This comment has been minimized.
This comment has been minimized.
MauiBot
removed
the
s/agent-review-in-progress
A recurring android/mac gate failure is DotNet.csproj MSB6003 'System.IO.Pipes.dll ... cannot find the file': the shared Microsoft.NETCore.App pack was provisioned WITHOUT System.IO.Pipes.dll (an assembly that always ships in that framework). 'dotnet --version' still succeeds (it never loads that assembly) and the SDK carries a valid cake stamp, so cake REUSES the corrupt SDK and every build that spawns a process (Exec 'sh') dies. On the GATE stage this blocks the whole review: setup never completes, the sentinel is never written, and NO review is posted. Observed on #34136 (build 14753151 — last review was June 20, this build posted nothing) and #36657. The existing 'Prepare .dotnet' step can't catch it: it is windows-only and its --version probe can't see a missing runtime assembly. Add a non-windows, best-effort (continueOnError) pre-step that checks each installed Microsoft.NETCore.App pack for System.IO.Pipes.dll and, if missing, removes ./.dotnet so the build step reinstalls a complete SDK. It only ever deletes a provably-incomplete SDK and can never fail the build itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
This comment has been minimized.
This comment has been minimized.
In deferred posting, when the gate/review agent produced no PRAgent content (e.g. the Review stage failed before uploading its logs), the 'no PRAgent dir' branch only LOGGED 'falling back to posting deep results only' and posted NOTHING — the actual posting lives entirely in the else branch. So a build that ran the full deep suite could leave the author with zero feedback (observed on #34136 build 14755367: deep succeeded, review never posted). Post a standalone deep-results review in that branch so the deep outcome is always surfaced, with an honest note that the full AI summary didn't generate (infra) and to re-comment /review. Best-effort (try/catch + LASTEXITCODE reset) so it can never fail the Post task; scoped entirely to the previously-no-op branch so the normal (PRAgent-present) path is untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
…ults post Per feedback on #34136: when a review stage fails, the PR was left showing only the bare 'Automated review could not complete — no review summary was produced' notice, even though the deep UI tests ran. The deferred-post fallback (385d70d) posts a standalone deep-results review in that case, but it did NOT collapse the earlier review-incomplete notice — so the PR ended with BOTH a real deep-results review AND a contradictory 'no summary produced' warning. After the standalone deep-results review posts, collapse the review-incomplete notice (Hide-StaleMauiBotIssueComments -IncludeReviewIncomplete) so the PR is FINISHED with an actual summary — matching what the normal post path already does. Best-effort; never fatal. When there are genuinely no deep results either, the notice is left as the honest outcome. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
ℹ️ The review agent did not produce a full summary on this run (an infrastructure issue on the CI agent), but the deep UI tests completed — their results are below. Re-comment
/reviewfor a fresh full review.
✅ Deep UI tests — 354 passed, 0 failed across 3 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Button |
67/69 ✓ | 1 diff PNG |
Label |
96/98 ✓ | — |
Layout |
191/194 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
MauiBot
removed
the
s/agent-review-in-progress
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 #34092
Description
Every
SetValuecall allocates newPropertyChangedEventArgsandPropertyChangingEventArgsobjects. SinceBindableProperty.PropertyNameis immutable, these are now cached on theBindablePropertyinstance via lazyfield ??=fields.Changes
BindableProperty.cs— two new cached fields:CachedPropertyChangedEventArgsandCachedPropertyChangingEventArgs, lazily initialized on first accessBindableObject.cs— new internalOnPropertyChanged(PropertyChangedEventArgs)andOnPropertyChanging(PropertyChangingEventArgs)overloads that accept pre-allocated args;SetValueCoreandClearValueupdated to use the cached argsBenchmark
4.5× faster, zero allocation for PropertyChanged args.