Cache Brushes - #36405
Merged
Merged
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36405Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36405" |
Contributor
|
Hey there @@pictos! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Contributor
|
Hey there @pictos! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request targets issue #35302 by reducing SolidColorBrush/ImmutableBrush churn on hot PropertyChanged paths, primarily by caching color-to-brush conversions in Brush implicit operators, and by adding benchmarks to quantify the allocation/runtime impact.
Changes:
- Added a two-stage
Color -> ImmutableBrushcache (simple dictionary that promotes to a small LRU) underMicrosoft.Maui.Controls.Internals. - Updated
Brushimplicit conversions fromColorandSolidPaintto reuse cachedImmutableBrushinstances. - Added BenchmarkDotNet benchmarks focused on property-change propagation and background/brush assignment allocations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/src/Core/Brush/Brush.cs | Switches implicit conversions to cached ImmutableBrush instances (allocation/perf optimization). |
| src/Controls/src/Core/Internals/ICache.cs | Introduces a small internal cache abstraction for brush caching. |
| src/Controls/src/Core/Internals/CacheWithSwitch.cs | Implements a two-stage cache (simple → LRU) for ImmutableBrush keyed by Color. |
| src/Controls/src/Core/Internals/LRUBrushCache.cs | Adds a fixed-capacity LRU cache implementation used after promotion. |
| src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs | Adds benchmarks to measure property-change propagation performance/allocations (esp. background changes). |
Comment on lines
+24
to
+29
| public LRUBrushCache(int capacity) | ||
| { | ||
| ArgumentOutOfRangeException.ThrowIfNegativeOrZero(capacity); | ||
|
|
||
| _capacity = capacity; | ||
| } |
Comment on lines
+31
to
+44
| public LRUBrushCache(int capacity, Dictionary<Color, ImmutableBrush> brushes) | ||
| { | ||
| ArgumentOutOfRangeException.ThrowIfNegativeOrZero(capacity); | ||
| ArgumentNullException.ThrowIfNull(brushes); | ||
|
|
||
| if (brushes.Count > capacity) | ||
| { | ||
| throw new ArgumentException("Brush count must not exceed capacity.", nameof(brushes)); | ||
| } | ||
|
|
||
| _capacity = capacity; | ||
| _cache = new Dictionary<Color, LinkedListNode<ImmutableBrush>>(capacity); | ||
| _lru = []; | ||
|
|
Comment on lines
+12
to
+17
| public CacheWithSwitch(int capacity) | ||
| { | ||
| ArgumentOutOfRangeException.ThrowIfNegativeOrZero(capacity); | ||
|
|
||
| _cache = new SimpleCache(capacity); | ||
| } |
|
|
||
| private class SimpleCache(int capacity) : ICache<Color, ImmutableBrush> | ||
| { | ||
| readonly Dictionary<Color, ImmutableBrush> _dict = []; |
Comment on lines
19
to
+23
| if (paint is SolidPaint solidPaint) | ||
| return new SolidColorBrush { Color = solidPaint.Color }; | ||
| { | ||
| var color = solidPaint.Color; | ||
| return color is null ? Default : _cache.Get(solidPaint.Color); | ||
| } |
| /// Properties tested: | ||
| /// - HeightRequest (double) | ||
| /// - Background (Brush – exercises color-to-brush conversion and caching) | ||
| /// - IsEnabled (bool – coerced through the visual tree, uses boxed value caching) |
Comment on lines
+21
to
+22
| /// The Background property is particularly interesting as it exercises implicit Color-to-Brush | ||
| /// conversion, which can benefit from brush instance caching. IsEnabled tests boxed bool reuse. |
Comment on lines
19
to
+23
| if (paint is SolidPaint solidPaint) | ||
| return new SolidColorBrush { Color = solidPaint.Color }; | ||
| { | ||
| var color = solidPaint.Color; | ||
| return color is null ? Default : _cache.Get(solidPaint.Color); | ||
| } |
Comment on lines
+45
to
+49
| foreach (var (color, brush) in brushes) | ||
| { | ||
| var node = _lru.AddFirst(brush); | ||
| _cache.Add(color, node); | ||
| } |
Comment on lines
+15
to
+19
| /// - HeightRequest (double) | ||
| /// - Background (Brush – exercises color-to-brush conversion and caching) | ||
| /// - IsEnabled (bool – coerced through the visual tree, uses boxed value caching) | ||
| /// - Opacity (double – coerced to [0,1]) | ||
| /// - FontSize (double – Button only) |
This was referenced Jul 6, 2026
Contributor
|
@pictos I love this PR like all the performance ones! |
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
reviewed
Jul 6, 2026
MauiBot
left a comment
MauiBot
left a comment
Collaborator
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
|
|
||
| public ImmutableBrush Get(Color key) | ||
| { | ||
| ref var value = ref CollectionsMarshal.GetValueRefOrAddDefault(_dict, key, out _); |
Collaborator
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[critical] Build / netstandard compatibility — Controls.Core.csproj still targets netstandard2.0/netstandard2.1, but this new unconditional code uses APIs that are not available in the netstandard reference assemblies (CollectionsMarshal.GetValueRefOrAddDefault here, plus ThrowIfNegativeOrZero/ThrowIfNull in the new cache types). Existing code such as BindableObject.GetOrCreateContext guards CollectionsMarshal with #if !NETSTANDARD; this needs the same fallback or netstandard-safe dictionary code.
| [System.ComponentModel.TypeConverter(typeof(BrushTypeConverter))] | ||
| public abstract partial class Brush : Element | ||
| { | ||
| static readonly ICache<Color, ImmutableBrush> _cache = new CacheWithSwitch(51); |
Collaborator
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Threading/static state — This makes every public Color/SolidPaint to Brush conversion mutate one process-wide cache, but the cache implementation uses Dictionary/LinkedList and explicitly documents LRUBrushCache as not thread-safe. The previous conversion was stateless. Concurrent conversions can corrupt the cache or throw; make the static cache synchronized/thread-safe or keep the conversion allocation-local.
| public static Brush Default => defaultBrush ??= new(null); | ||
|
|
||
| public static implicit operator Brush(Color color) => new SolidColorBrush(color); | ||
| public static implicit operator Brush(Color color) => color is null ? Default : _cache.Get(color); |
Collaborator
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Public behavior change — The implicit Color conversion used to return a fresh mutable SolidColorBrush; it now returns a cached ImmutableBrush. Code such as var b = (SolidColorBrush)(Brush)Colors.Red; b.Color = Colors.Blue; used to update that independent brush, but now silently no-ops and shares the instance globally. Avoid changing the mutability contract of this public conversion, or explicitly account for the breaking behavior with API review/tests.
MauiBot
added
s/agent-gate-failed
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/Controls/src/Core/Brush/Brush.cs:24
- In the SolidPaint -> Brush implicit conversion,
solidPaint.Coloris read into a local variable (color) but the cached lookup usessolidPaint.Coloragain. Using the local variable avoids an extra property access and guarantees consistent behavior ifColorwere to change between reads.
var color = solidPaint.Color;
return color is null ? Default : _cache.Get(solidPaint.Color);
}
src/Controls/src/Core/Brush/Brush.cs:112
- The Color/SolidPaint -> Brush implicit conversions now return cached
ImmutableBrushinstances (instead of allocating a newSolidColorBrush). This is an observable behavior change: callers may now receive a shared, effectively immutable brush instance (e.g.,((SolidColorBrush)brush).Color = ...is a no-op due toImmutableBrush.Coloroverride). If this is intentional, it would be good to explicitly lock in/validate the new contract (e.g., additional tests around mutability/reference reuse) to avoid regressions; if not, the cache may need to be internal-only to the hot path rather than exposed via the public implicit operators.
public static Brush Default => defaultBrush ??= new(null);
public static implicit operator Brush(Color color) => color is null ? Default : _cache.Get(color);
src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs:57
- This benchmark file introduces long dashed "section divider" comments (e.g.,
// -------------------------------------------------------------------------). The Benchmarks folder generally uses shorter single-line section headers (e.g.,// --- #NNNNN: ... ---), and the long separators add noise without providing extra information. Consider replacing them with a simple one-line header comment (and apply the same change to the similar block around the Background benchmarks).
// -------------------------------------------------------------------------
// HeightRequest property (double)
// -------------------------------------------------------------------------
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (7)
src/Controls/src/Core/Brush/Brush.cs:24
- In the SolidPaint conversion, you already store
solidPaint.Colorin a local variable but then call the property again for the cache lookup. Using the localcoloravoids a redundant property access and guarantees the value you null-checked is the value used as the cache key.
if (paint is SolidPaint solidPaint)
{
var color = solidPaint.Color;
return color is null ? Default : _cache.Get(solidPaint.Color);
}
src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs:83
- Avoid banner-style separator comments; they add noise and are discouraged in this repo’s guidelines. A simple one-line header comment is enough here.
// -------------------------------------------------------------------------
// Background property (Color)
// -------------------------------------------------------------------------
src/Controls/src/Core/Internals/LRUBrushCache.cs:64
_cache/_lruare initialized at the field declaration and re-assigned in the seeded constructor, which means the seeded constructor pays for two allocations that are immediately discarded. Consider removing the field initializers and always initializing the fields explicitly in the constructors.
readonly Dictionary<Color, LinkedListNode<ImmutableBrush>> _cache = [];
readonly LinkedList<ImmutableBrush> _lru = [];
readonly int _capacity;
src/Controls/src/Core/Internals/LRUBrushCache.cs:32
- After removing the field initializers for
_cache/_lru, the primary constructor should initialize them (and can pre-size the dictionary) so the instance is usable when constructed without seeding.
public LRUBrushCache(int capacity)
{
if (capacity <= 0)
{
throw new ArgumentOutOfRangeException(nameof(capacity));
}
_capacity = capacity;
}
src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs:4
using System;is unused in this benchmark file.
using System;
using BenchmarkDotNet.Attributes;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Graphics;
src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs:58
- Avoid banner-style separator comments; they add noise and are discouraged in this repo’s guidelines. A simple one-line header comment is enough here.
This issue also appears on line 80 of the same file.
// -------------------------------------------------------------------------
// HeightRequest property (double)
// -------------------------------------------------------------------------
src/Controls/src/Core/Brush/Brush.cs:4
using System;appears unused in this file; leaving it in can trigger IDE0005 (or similar analyzers) and adds noise.
#nullable disable
using System;
using Microsoft.Maui.Controls.Internals;
using Microsoft.Maui.Graphics;
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (7)
src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs:6
- This benchmarks project consistently uses the
Microsoft.Maui.Handlers.Benchmarksnamespace (e.g.,src/Core/tests/Benchmarks/Program.cs:3). UsingMicrosoft.Maui.Benchmarkshere is inconsistent and makes it harder to locate related benchmarks.
namespace Microsoft.Maui.Benchmarks
src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs:57
- The long dashed separator comment blocks add noise and don’t match the existing style in this benchmarks project (which typically uses short
// ---section headers). Consider removing the dashed lines and keeping just the one-line section header.
// -------------------------------------------------------------------------
// HeightRequest property (double)
// -------------------------------------------------------------------------
src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs:82
- Same as above: consider removing the dashed separator lines and keeping only the one-line section header.
// -------------------------------------------------------------------------
// Background property (Color)
// -------------------------------------------------------------------------
src/Controls/src/Core/Internals/LRUBrushCache.cs:65
LRUBrushCache(int capacity, Dictionary<Color, ImmutableBrush> brushes)seeds the LRU list with brush instances but later evicts entries by removing_cacheusinglast.Value.Coloras the key. If any seeded brush’sColordoesn’t exactly match its dictionary key (or a seeded value is null), eviction can fail to remove the correct key and corrupt the cache state. Add validation while seeding to ensure the brush instance is non-null and itsColormatches the key.
foreach (var pair in brushes)
{
var color = pair.Key;
var brush = pair.Value;
var node = _lru.AddFirst(brush);
src/Controls/src/Core/Internals/InlineLRUCache/Lru64ColorVectorInlineBrushCache.cs:4
using System;is unused in this file.
#nullable disable
using System;
using Microsoft.Maui.Graphics;
src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs:4
using System;is unused in this file.
This issue also appears in the following locations of the same file:
- line 6
- line 55
- line 80
using System;
using BenchmarkDotNet.Attributes;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Graphics;
src/Controls/src/Core/Brush/Brush.cs:16
- The PR description references a
CacheWithSwitchtwo-stage cache and aCacheWithSwitch.csfile, but the current implementation wired intoBrushusesLru64ColorVectorInlineBrushCachedirectly and noCacheWithSwitch.csexists in the repo. Please update the PR description to match the current approach and the files actually added/modified.
static readonly ICache<Color, ImmutableBrush> _cache = new Lru64ColorVectorInlineBrushCache(64);
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MartyIX
reviewed
Jul 31, 2026
| @@ -0,0 +1,299 @@ | |||
| #nullable disable | |||
Contributor
There was a problem hiding this comment.
Isn't it better to avoid this for new code?
MauiBot
added
s/agent-changes-requested
Collaborator
AI Review Summary
🗂️ Review Sessions — click to expand🚦 Gate — Test Before & After FixGate Result: ✅ PASSEDPlatform: ANDROID · Base: net11.0 · Merge base: ✅ Verified (new API / feature) — this PR adds new API and a test that references it in the same project, so reverting the fix un-compiles the test: there is no valid "fails without the fix" baseline to establish (a compile-coupled baseline). The gate instead verified the fix by a clean build + pass with the fix, so this is a real PASS rather than a non-committal INCONCLUSIVE.
🔴 Without fix — 🧪 BrushTypeConverterUnitTests: 🛠️ BUILD ERROR · 80sError-relevant lines (filtered from the build log): 🟢 With fix — 🧪 BrushTypeConverterUnitTests: PASS ✅ · 50s(no coded error found; showing last 1200 chars) 🔴 Without fix — 🧪 Lru64ColorVectorInlineBrushCacheUnitTests: 🛠️ BUILD ERROR · 24sError-relevant lines (filtered from the build log): 🟢 With fix — 🧪 Lru64ColorVectorInlineBrushCacheUnitTests: PASS ✅ · 20s(no coded error found; showing last 1200 chars) 🔴 Without fix — 🧪 LRUBrushCacheUnitTests: 🛠️ BUILD ERROR · 20sError-relevant lines (filtered from the build log): 🟢 With fix — 🧪 LRUBrushCacheUnitTests: PASS ✅ · 19s(no coded error found; showing last 1200 chars)
|
| Category | Tests | Snapshot diffs |
|---|---|---|
Brush |
42/42 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
📋 Pre-Flight — Context & Validation
Issue: #35302 - Improve PropertyChanged performance scenarios
PR: #36405 - Cache Brushes
Platforms Affected: Android for requested testing; implementation is shared Controls/Core code affecting all platforms.
Files Changed: 5 implementation, 5 test/benchmark
Key Findings
- Issue Improve PropertyChanged performance scenarios #35302 reports high
SolidColorBrushallocation churn from implicitColor->Brushconversions during common property-change scenarios. - PR Cache Brushes #36405 addresses the allocation path by changing public
Color/SolidPaint->Brushconversions to return cachedImmutableBrushinstances from a process-wide LRU cache. - Gate artifacts already present for this run show Controls.Core unit tests passed with the PR fix and build-failed without the compile-coupled new cache types; gate verification was not re-run.
- Prior review comments show earlier null-handling and thread-safety issues were addressed, but the public mutability/identity contract concern remains unresolved.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: low
Errors: 1 | Warnings: 0 | Suggestions: 0
Key code review findings:
- ✗
src/Controls/src/Core/Brush/Brush.cs:22andsrc/Controls/src/Core/Brush/Brush.cs:110— publicColor/SolidPaintconversions now return shared immutable brushes instead of fresh mutableSolidColorBrushinstances. - ✗ Prior MauiBot finding about the cached immutable conversion remains unresolved in the current diff.
- Failure mode: code that casts the converted brush to
SolidColorBrushand then setsColorused to mutate an independent brush; with the PR fix, the setter is a no-op onImmutableBrush. - Blast radius: every public implicit
Color/SolidPaint->Brushconversion uses the static cache, so this affects all platforms and all controls using those conversions.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #36405 | Process-wide LRU cache returning ImmutableBrush from public implicit conversions. |
✅ PASSED (Gate) | Brush.cs, cache files, unit tests, benchmarks |
Original PR; performant but changes public conversion mutability/identity semantics. |
🔬 Code Review — Deep Analysis
Code Review — PR #36405
Independent Assessment
What this changes: Adds a process-wide cache for Color/SolidPaint → Brush conversions, returning cached ImmutableBrush instances instead of allocating new SolidColorBrush objects. Also adds LRU cache implementations, tests, and benchmarks.
Inferred motivation: Reduce allocation churn in hot property-change/background paths.
Reconciliation with PR Narrative
Author claims: PR optimizes color-to-brush conversion allocation/performance for issue #35302.
Agreement/disagreement: The implementation matches the performance goal, but the narrative does not address the public behavioral change from fresh mutable brushes to shared immutable brushes. The PR body also appears stale in places, referencing CacheWithSwitch, which is not present in the current diff.
Prior Review Reconciliation
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
| Static cache was not thread-safe | MauiBot inline review | ✅ Fixed | Current Lru64ColorVectorInlineBrushCache.Get locks around all cache access. |
Color conversion now returns cached immutable brush |
MauiBot inline review | ❌ Unresolved | Current Brush.cs:110 still returns _cache.Get(color), which produces ImmutableBrush. |
| Added tests passed without fix / did not catch bug | MauiBot gate reviews | ❌ Unresolved | Current tests cover null conversions and cache mechanics, but not the mutability/identity contract change or a failing pre-fix allocation regression. |
External Output Contract
| Consumer token/pattern | Producer location | Producer emission condition | Consumer assumption | Ordinary negative case | Downstream effect |
|---|---|---|---|---|---|
| N/A | N/A | No changed code classifies external tool output with regex/string tokens. | N/A | N/A | N/A |
Blast Radius Assessment
- Runs for all instances: Yes — every public implicit
Color/SolidPainttoBrushconversion uses the static cache. - Startup impact: Low direct startup impact, but
Brushtype initialization now creates a static cache. - Static/shared state: Yes — one process-wide brush cache shared across all callers.
- Public API behavior: High impact — conversion operators are public and widely used.
CI Status
- Required-check result:
gh pr checks --requiredcould not run becauseghis unauthenticated. - Fallback result: public check-runs show
maui-prfailed on head06aacbf7902fafa528b5a72549846fb3a36a114b. - Classification: failures appear in Samples integration builds with generated XAML nullability errors in Sandbox pages, not in changed brush/cache files; PR-causality is not established.
- Action taken: loaded
azdo-build-investigatorcontext and inspected public AzDO timeline/logs. Confidence capped low because CI is red/tooling could not determine required checks directly.
Findings
❌ Error — Public Color/SolidPaint conversions now return immutable shared brushes
src/Controls/src/Core/Brush/Brush.cs:22 and src/Controls/src/Core/Brush/Brush.cs:110
The previous conversion returned a fresh mutable SolidColorBrush:
Brush brush = Colors.Red;
((SolidColorBrush)brush).Color = Colors.Blue;That used to mutate only this caller’s brush. The new path returns a cached ImmutableBrush; its Color setter is a no-op, and the same instance can be shared globally. This is a breaking behavior change for a public conversion operator and is not called out or tested.
Failure-Mode Probing
- Reusing a converted brush across controls: now shares one immutable instance globally; parent assignment is guarded, but inherited bindable state/identity semantics differ from fresh instances.
- Mutating after conversion: no longer works;
ImmutableBrush.Colorsilently ignores assignment. - Concurrent conversion: current implementation is locked, so the prior cache-corruption issue appears fixed.
- Cache eviction: LRU mechanics and
Color.ToUint()keying align with existingColor.Equalsbyte-precision behavior.
Verdict: NEEDS_CHANGES
Confidence: low
Summary: The optimization is plausible, and the cache implementation looks mostly sound, but it changes the public conversion contract from fresh mutable brushes to shared immutable brushes. CI is also red, so this should not merge as-is.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Preserve public implicit conversion semantics by returning fresh mutable SolidColorBrush instances and leaving cache types unused by public operators. |
✅ PASS | 1 file | Correctness-safe but loses the default allocation win for direct Background = Color. |
| 2 | try-fix | Make cached immutable implicit conversions opt-in via AppContext, with compatible fresh mutable conversions by default. |
✅ PASS | 1 file | Safer rollout, but default behavior no longer fixes #35302; opt-in mode has the same mutability tradeoff as the PR. |
| PR | PR #36405 | Process-wide LRU cache returning shared ImmutableBrush from public implicit Color/SolidPaint conversions. |
✅ PASSED (Gate) | 10 files | Best allocation result, but code review found an unresolved public conversion mutability/identity contract risk. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| gpt-5.5 / maui-expert-reviewer | 1 | Yes | Suggested three classes of alternatives: lightweight fresh brush, internal/binding-only cache, and explicit/opt-in cached immutable conversion. |
| gpt-5.5 / maui-expert-reviewer | 2 | No | NO NEW IDEAS: preserving fresh mutable identity means each public C# implicit conversion must create a distinct brush; the conversion happens before a Brush-typed property setter can intercept it. |
Learning Summary
- The public API compatibility concern is real: the PR's current fix changes
Brush brush = Colors.Red; ((SolidColorBrush)brush).Color = Colors.Blue;from mutating a fresh brush to no-oping on a sharedImmutableBrush. - Candidate 1 proves the compatibility-safe fallback passes tests, but it does not solve the measured direct-assignment allocation path.
- Candidate 2 provides a possible explicit rollout mechanism, but it cannot be a transparent default fix without accepting the same contract break.
- A lightweight fresh brush could reduce secondary
BindableObjectstorage cost, but it would still allocate a brush per conversion and would require a deeperSolidColorBrushredesign with high risk around bindings,SetValue,PropertyChanged, and dynamic resources; it was not implemented as a trivial variation.
Exhausted: Yes
Selected Fix: PR #36405 remains the only tested candidate that preserves the default allocation optimization, but it is not demonstrably safe because of the unresolved public implicit conversion contract change. No alternative tested here is demonstrably better than the PR's fix.
📝 Recommended PR Title & Description
Assessment: ✏️ Recommend updating — the current title is too vague and the description is stale/inaccurate in places, including references to CacheWithSwitch and a cache capacity that do not match the current diff; it also omits the expert-reviewed compatibility constraints.
Recommended title
[Controls] Brush: Cache color-to-brush conversions
Recommended description
<!--
!!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!!
-->
### Issues Fixed
Fixes #35302
### Description of Change
This pull request reduces allocation churn from repeated `Color`/`SolidPaint` to `Brush` conversions in common property-change scenarios such as setting `VisualElement.Background`.
The implementation adds a shared brush cache for solid-color conversions and wires the `Brush` conversion operators into that cache. On modern .NET targets the cache uses `Lru64ColorVectorInlineBrushCache`, an inline-array/SIMD-backed fixed-capacity LRU cache for `ImmutableBrush` instances. On `NETSTANDARD` targets it falls back to `LRUBrushCache`, a dictionary plus linked-list LRU implementation with the same public cache behavior.
Key changes:
- Adds `ICache<TKey, TValue>` as the small internal cache abstraction used by the brush caches.
- Adds `Lru64ColorVectorInlineBrushCache` and its `Lru64ColorVectorInline<TValue>` backing storage for the optimized .NET path.
- Adds `LRUBrushCache` as the fallback cache implementation.
- Updates `Brush` conversion paths for `Color` and `SolidPaint` to use the shared cache instead of allocating a new `SolidColorBrush` for every conversion.
- Adds unit tests for cache capacity, identity reuse, eviction, concurrency, and null conversion behavior.
- Adds `BrushCacheBenchmarker` and `PropertyChangePropagationBenchmarker` benchmark coverage for brush cache and property-change allocation/performance scenarios.
### Expert Review Notes
The final implementation must preserve public conversion compatibility and exact color behavior:
- Public `Color`/`SolidPaint` to `Brush` conversion previously returned a fresh mutable `SolidColorBrush`. If cached immutable brushes remain on this public path, document the behavior change explicitly and add regression coverage; otherwise keep the public conversion compatible and restrict caching to an explicit/internal path.
- Cache keys must not cause a brush initialized with one `Color` object/component set to be returned for a different requested `Color` that only collides at byte precision.
### Platforms Tested
- [x] Android
🏁 Report — Final Recommendation
Comparative Report — PR #36405
Candidates compared
| Candidate | Approach | Regression tests | Correctness / compatibility | Performance goal | Rank |
|---|---|---|---|---|---|
pr-plus-reviewer |
PR fix with expert feedback applied in sandbox: preserve public mutable conversion semantics or make cached immutable conversion explicit/opt-in, and fix exact-color cache keying if the cache remains exposed. | ✅ Passed by available gate/candidate evidence | Best correctness profile: addresses both expert findings. | Needs additional design work to retain default allocation win without public contract break. | 1 |
try-fix-1 |
Return fresh mutable SolidColorBrush from public Color/SolidPaint conversions; leave cache implementation unused by those operators. |
✅ Pass | Fixes the public mutability/identity issue. | Does not solve the direct Background = Color allocation path by default. |
2 |
try-fix-2 |
Keep compatible fresh conversions by default; allow cached immutable conversions behind AppContext switch. |
✅ Pass | Compatible by default, but opt-in mode still has the immutable/shared semantics risk unless documented and tested. | Does not fix #35302 by default; opt-in only. | 3 |
pr |
Unconditionally return cached shared ImmutableBrush from public conversions. |
✅ Gate passed | Fails expert review: public conversion mutability/identity changes, and inline cache can return a brush with the wrong exact Color for ToUint() collisions. |
Best measured default allocation reduction. | 4 |
Key comparison
The raw PR is the only submitted implementation that directly fixes the allocation hot path by default, because C# performs the implicit Color to Brush conversion before the Brush-typed property setter can intercept it. However, the expert review found two concrete correctness issues in that default path: public conversions no longer produce a fresh mutable brush, and the inline cache uses a byte-precision key that can lose exact color component identity.
try-fix-1 and try-fix-2 are safer than the raw PR for compatibility, and both passed targeted tests, but neither preserves the PR's default allocation benefit. try-fix-1 simply reverts the hot path to allocation-per-conversion. try-fix-2 is a safer rollout model, but with the switch disabled it does not fix the issue by default, and with the switch enabled it needs the same immutable/shared semantics and exact-color-key fixes as the PR.
pr-plus-reviewer wins because it starts from the PR and applies the expert correctness requirements. That candidate should not merge until the author chooses the exact implementation shape, but among the available candidates it is the only one that both acknowledges the performance objective and removes the high-confidence correctness defects.
Winning candidate
Winner: pr-plus-reviewer
Rationale: Candidates that passed regression tests outrank any failed candidates; all provided candidates passed, so correctness and product fit decide the ranking. The raw PR has the strongest performance result but carries unresolved public API behavior and exact-color bugs. The reviewer-adjusted PR is the best path forward because it fixes those defects while keeping the PR as the base for a safer cache design.
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
MauiBot
removed
the
s/agent-review-in-progress
kubaflo
approved these changes
Jul 31, 2026
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.
Issues Fixed
Fixes #35302
Description of Change
This pull request introduces a new caching mechanism for
ImmutableBrushinstances, specifically optimizing color-to-brush conversions in theBrushclass. It adds a two-stage cache (simple and LRU) to reduce allocations and improve performance when repeatedly converting colors to brushes. Additionally, a suite of benchmarks is included to measure the impact of these changes on property-change propagation in common controls.Caching improvements for brush creation:
CacheWithSwitchclass that provides a two-stage caching strategy forImmutableBrushinstances keyed byColor. It starts with a simple dictionary cache and automatically promotes to an LRU cache when capacity is reached, reducing memory allocations and improving brush reuse. (src/Controls/src/Core/Internals/CacheWithSwitch.cs)ICache<TKey, TValue>interface to standardize cache implementations used for brush caching. (src/Controls/src/Core/Internals/ICache.cs)LRUBrushCacheclass for least-recently-used caching of brushes, used as the second stage ofCacheWithSwitch. (src/Controls/src/Core/Internals/LRUBrushCache.cs)Integration with Brush class:
Brushclass to use the new cache for implicit conversions fromColorandSolidPaint, ensuring brush instances are reused whenever possible. (src/Controls/src/Core/Brush/Brush.cs) [1] [2] [3]Benchmarking and performance validation:
Label,Button, andEntrycontrols, with a focus on background color changes that exercise the new brush caching logic. (src/Core/tests/Benchmarks/Benchmarks/PropertyChangePropagationBenchmarker.cs)Brush cache benchmark: branch vs
mainSetup:
mainhas no brush cache — everyPaint → Brushconversion allocatesnew SolidColorBrush { Color = c }. This branch addsLru64ColorVectorInlineBrushCache(inline-array + SIMD lookup) and wires it intoBrush.cs. Benchmarked over identical color sets, ShortRun, Apple M4 Max.Production path —
InlineLruCache(branch) vs no-cache (main)Full branch run (net11, ShortRun)
Takeaways
SolidColorBrush(~1 KB) on every conversion, the cache returns a shared instance.LRUBrushCache.