Skip to content

[Perf] Cache PropertyChanged/ChangingEventArgs on BindableProperty #34136

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.

Sign up for GitHub

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

Open
simonrozsival wants to merge 4 commits into
base: net11.0
Choose a base branch
from
+12 −2
Open

[Perf] Cache PropertyChanged/ChangingEventArgs on BindableProperty #34136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19945fd
Cache PropertyChangedEventArgs and PropertyChangingEventArgs on Binda…
simonrozsival Feb 19, 2026
eba1e07
Use field keyword for cached EventArgs properties
simonrozsival Feb 19, 2026
896c058
Revert Element.cs changes - keep only BindableProperty/BindableObject…
simonrozsival Mar 11, 2026
e3e616a
Fix: preserve virtual method dispatch with cached EventArgs
simonrozsival Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Controls/src/Core/BindableObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,14 +399,14 @@ protected virtual void OnBindingContextChanged()
/// </summary>
/// <param name="propertyName">The name of the property that has changed.</param>
protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
=> PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
=> PropertyChanged?.Invoke(this, BindableProperty.GetCachedPropertyChangedEventArgs(propertyName));

/// <summary>
/// Raises the <see cref="PropertyChanging"/> event.
/// </summary>
/// <param name="propertyName">The name of the property that is changing.</param>
protected virtual void OnPropertyChanging([CallerMemberName] string propertyName = null)
=> PropertyChanging?.Invoke(this, new PropertyChangingEventArgs(propertyName));
=> PropertyChanging?.Invoke(this, BindableProperty.GetCachedPropertyChangingEventArgs(propertyName));

/// <summary>
/// Removes all current bindings from the current context.
Expand Down
10 changes: 10 additions & 0 deletions src/Controls/src/Core/BindableProperty.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#nullable disable
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics;
Expand Down Expand Up @@ -252,6 +253,15 @@ public sealed class BindableProperty

internal ValidateValueDelegate ValidateValue { get; private set; }

private static readonly ConcurrentDictionary<string, PropertyChangedEventArgs> s_changedArgsCache = new();

@pictos pictos May 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kubaflo kubaflo Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

private static readonly ConcurrentDictionary<string, PropertyChangingEventArgs> s_changingArgsCache = new();

internal static PropertyChangedEventArgs GetCachedPropertyChangedEventArgs(string propertyName)
=> s_changedArgsCache.GetOrAdd(propertyName, static name => new PropertyChangedEventArgs(name));

@kubaflo kubaflo Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 static PropertyChangingEventArgs GetCachedPropertyChangingEventArgs(string propertyName)

@MauiBot MauiBot Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 MauiBot Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Memory Leak Prevention — These are process-lifetime static caches with no eviction and no bound, keyed on an arbitrary caller-supplied string. OnPropertyChanged/OnPropertyChanging are public-surface (protected virtual) entry points, so any BindableObject subclass that raises change notifications with a dynamically composed name — indexer-style names such as $"Item[{i}]", names derived from collection items, or per-instance generated names — permanently adds one dictionary entry plus one PropertyChangedEventArgs/PropertyChangingEventArgs per distinct string, and neither the entry nor the string is ever released even after every object using it is collected. Unlike the previous per-call allocation (gen0, collected immediately), this converts a transient allocation into an unbounded gen2-rooted one. If the intent is to cache only the framework's fixed set of names, key the cache off the BindableProperty instance (which already owns a stable PropertyName) or store the args in a field on BindableProperty, and allocate normally for names that do not come from a registered property.

=> s_changingArgsCache.GetOrAdd(propertyName, static name => new PropertyChangingEventArgs(name));

// Properties that this property depends on - when getting this property's value,

@MauiBot MauiBot Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Performance-Critical Path — This replaces a small gen0 allocation on the property-change path with a string hash plus ConcurrentDictionary lookup on every notification, which is not obviously a win: PropertyChangedEventArgs is a 2-field object that the gen0 allocator handles in a few instructions, while GetOrAdd costs a full string hash (proportional to name length) plus a bucket probe and comparison. A cheaper design exists for the dominant call path — BindableObject.SetValue raises notifications with property.PropertyName, so the args instance could be stored in a field on the BindableProperty itself and reused with zero lookup. Since the change is justified purely on performance, it needs measured evidence (dotnet-trace / BenchmarkDotNet on a property-change-heavy scenario) showing the dictionary lookup beats the allocation it removes; otherwise this is a behavior-changing rewrite of a hot path with no proven benefit.

// if the dependency has a pending binding, return the default value instead.

@MauiBot MauiBot Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[critical] Logic and Correctness — ConcurrentDictionary<string,T>.GetOrAdd throws ArgumentNullException when the key is null, so a null propertyName now crashes where it previously worked. BindableObject.OnPropertyChanged(string propertyName = null) / OnPropertyChanging(string propertyName = null) are protected virtual with a null default, and derived overrides forward a nullable value straight to base (e.g. Border.cs:443, BoxView.cs:150, RefreshView.cs:187, RadioButton.cs:377 all call base.OnPropertyChanged(propertyName) with string?). Concrete scenario: a subclass (in-tree or third-party) calls OnPropertyChanged(null) — the documented INotifyPropertyChanged idiom meaning "all properties changed" — or forwards a null variable; before this change new PropertyChangedEventArgs(null) was valid and the event was raised, now the call throws. The same applies to GetCachedPropertyChangingEventArgs on line 269. Guard the null case (return a cached args instance built for null, or fall back to new PropertyChangedEventArgs(propertyName) when propertyName is null).

// This is used to fix timing issues where one property binding resolves before another.
Expand Down
Loading