Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
12 changes: 10 additions & 2 deletions src/Controls/src/Core/Brush/Brush.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#nullable disable
using System;
using Microsoft.Maui.Controls.Internals;
using Microsoft.Maui.Graphics;
Comment on lines 1 to 3
using GraphicsGradientStop = Microsoft.Maui.Graphics.PaintGradientStop;

Expand All @@ -11,10 +13,16 @@ namespace Microsoft.Maui.Controls
[System.ComponentModel.TypeConverter(typeof(BrushTypeConverter))]
public abstract partial class Brush : Element
{
static readonly ICache<Color, ImmutableBrush> _cache = new CacheWithSwitch(51);

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] 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 implicit operator Brush(Paint paint)
{
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 thread
pictos marked this conversation as resolved.
Comment thread
pictos marked this conversation as resolved.
Comment on lines 19 to +23
Comment on lines 19 to +23
Comment on lines 19 to +23


if (paint is GradientPaint gradientPaint)
{
Expand Down Expand Up @@ -100,7 +108,7 @@ public static implicit operator Paint(Brush brush)
/// </summary>
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);

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


Comment thread
pictos marked this conversation as resolved.
/// <summary>
/// When overridden in a derived class, indicates whether the given brush represents the empty brush.
Expand Down
63 changes: 63 additions & 0 deletions src/Controls/src/Core/Internals/CacheWithSwitch.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
using System;
using System.Collections.Generic;
using System.Runtime.InteropServices;
using Microsoft.Maui.Graphics;

namespace Microsoft.Maui.Controls.Internals;

sealed class CacheWithSwitch : ICache<Color, ImmutableBrush>
{
ICache<Color, ImmutableBrush> _cache;

#if NETSTANDARD
readonly object _lock = new();
#else
readonly System.Threading.Lock _lock = new();
#endif

public CacheWithSwitch(int capacity)
{
if (capacity <= 0)
{
throw new ArgumentOutOfRangeException(nameof(capacity));
}

_cache = new SimpleCache(capacity);
}

private class SimpleCache(int capacity) : ICache<Color, ImmutableBrush>
{
readonly Dictionary<Color, ImmutableBrush> _dict = new (capacity);
public bool IsAtCapacity => _dict.Count == capacity;

public ImmutableBrush Get(Color key)
{
#if NETSTANDARD
if (!_dict.TryGetValue(key, out var value))
{
value = new ImmutableBrush(key);
_dict[key] = value;
}
#else
ref var value = ref CollectionsMarshal.GetValueRefOrAddDefault(_dict, key, out _);

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] Build / netstandard compatibilityControls.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.

value ??= new ImmutableBrush(key);
#endif
return value;
}

public LRUBrushCache Promote() => new(capacity, _dict);
}

public ImmutableBrush Get(Color key)
{
lock (_lock)
{
if (_cache is SimpleCache { IsAtCapacity: true } simple)
{
_cache = simple.Promote();
}

return _cache.Get(key);
}
}
}
6 changes: 6 additions & 0 deletions src/Controls/src/Core/Internals/ICache.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace Microsoft.Maui.Controls.Internals;

interface ICache<TKey, TValue>
{
TValue Get(TKey key);
}
89 changes: 89 additions & 0 deletions src/Controls/src/Core/Internals/LRUBrushCache.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
using System;
using System.Collections.Generic;
using Microsoft.Maui.Graphics;

namespace Microsoft.Maui.Controls.Internals;

/// <summary>
/// Provides a small, fixed-capacity least-recently-used (LRU) cache for <see cref="ImmutableBrush"/> instances,
/// keyed by <see cref="Color"/>.
/// </summary>

sealed class LRUBrushCache : ICache<Color, ImmutableBrush>
{
/// <summary>
/// Creates a new instance of <see cref="LRUBrushCache"/>
/// </summary>
/// <remarks>
/// This cache helps reduce allocations by reusing <see cref="ImmutableBrush"/> instances for frequently used colors.
/// When the cache exceeds <paramref name="capacity"/>, the least-recently accessed entry is evicted.
/// This type is not thread-safe.
/// </remarks>
Comment thread
pictos marked this conversation as resolved.
/// <param name="capacity">The maximum number of cached brushes to keep.</param>
/// <exception cref="ArgumentOutOfRangeException">Thrown when <paramref name="capacity"/> is zero or negative.</exception>
public LRUBrushCache(int capacity)
{
if (capacity <= 0)
{
throw new ArgumentOutOfRangeException(nameof(capacity));
}

_capacity = capacity;
}
Comment on lines +28 to +38
Comment on lines +28 to +38

public LRUBrushCache(int capacity, Dictionary<Color, ImmutableBrush> brushes)
{
if (capacity <= 0)
{
throw new ArgumentOutOfRangeException(nameof(capacity));
}

_ = brushes ?? throw new ArgumentNullException(nameof(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 +40 to +58
foreach (var pair in brushes)
{
var color = pair.Key;
var brush = pair.Value;
var node = _lru.AddFirst(brush);
_cache.Add(color, node);
}
}

readonly Dictionary<Color, LinkedListNode<ImmutableBrush>> _cache = [];
readonly LinkedList<ImmutableBrush> _lru = [];
readonly int _capacity;

public ImmutableBrush Get(Color key)
{
if (_cache.TryGetValue(key, out var node))
{
_lru.Remove(node);
_lru.AddFirst(node);
return node.Value;
}

var brush = new ImmutableBrush(key);

if (_cache.Count >= _capacity)
{
var last = _lru.Last!;
_lru.RemoveLast();
_cache.Remove(last.Value.Color);
}

var newNode = _lru.AddFirst(brush);
_cache[key] = newNode;

return brush;
}
}
32 changes: 32 additions & 0 deletions src/Controls/tests/Core.UnitTests/BrushTypeConverterUnitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,5 +111,37 @@ public void InvalidOperationExceptionWhenSettingParentOnImmutableBrush()
{
Assert.Throws<InvalidOperationException>(() => SolidColorBrush.Green.Parent = new Grid());
}

[Fact]
public void ImplicitConversionFromNullColorReturnsEmptyBrush()
{
var exception = Record.Exception(() =>
{
Brush brush = (Color)null;

Assert.NotNull(brush);
Assert.True(brush.IsEmpty);
Assert.True(Brush.IsNullOrEmpty(brush));
Assert.Null(((SolidColorBrush)brush).Color);
});

Assert.Null(exception);
}

[Fact]
public void ImplicitConversionFromSolidPaintWithNullColorReturnsEmptyBrush()
{
var exception = Record.Exception(() =>
{
Brush brush = new SolidPaint { Color = null };

Assert.NotNull(brush);
Assert.True(brush.IsEmpty);
Assert.True(Brush.IsNullOrEmpty(brush));
Assert.Null(((SolidColorBrush)brush).Color);
});

Assert.Null(exception);
}
}
}
133 changes: 133 additions & 0 deletions src/Controls/tests/Core.UnitTests/CacheWithSwitchUnitTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
using System;
using System.Collections.Generic;
using System.Reflection;
using Microsoft.Maui.Controls.Internals;
using Microsoft.Maui.Graphics;
using Xunit;

namespace Microsoft.Maui.Controls.Core.UnitTests
{
public class CacheWithSwitchUnitTests : BaseTestFixture
{
[Theory]
[InlineData(0)]
[InlineData(-1)]
public void CacheWithSwitchCtorThrowsWhenCapacityIsNotPositive(int capacity)
{
Assert.Throws<ArgumentOutOfRangeException>(() => new CacheWithSwitch(capacity));
}

[Fact]
public void CacheWithSwitchReturnsSameBrushForSameColor()
{
var cache = new CacheWithSwitch(5);

var first = cache.Get(Colors.Red);
var second = cache.Get(Colors.Red);

Assert.Same(first, second);
}

[Fact]
public void CacheWithSwitchPromotesToLruWhenAtCapacity()
{
var cache = new CacheWithSwitch(2);

cache.Get(Colors.Red);
cache.Get(Colors.Green);

Assert.Equal("SimpleCache", GetInnerCache(cache).GetType().Name);

cache.Get(Colors.Red);

Assert.IsType<LRUBrushCache>(GetInnerCache(cache));
Comment thread
pictos marked this conversation as resolved.
Outdated
}

[Fact]
public void CacheWithSwitchPromotionPreservesEntriesAndAppliesLruEviction()
{
var cache = new CacheWithSwitch(2);

var red = cache.Get(Colors.Red);
var green = cache.Get(Colors.Green);

var redAfterPromotion = cache.Get(Colors.Red);
Assert.Same(red, redAfterPromotion);

var blue = cache.Get(Colors.Blue);
var blueAgain = cache.Get(Colors.Blue);
Assert.Same(blue, blueAgain);
Assert.Same(red, cache.Get(Colors.Red));

var greenAfterEviction = cache.Get(Colors.Green);

Assert.NotSame(green, greenAfterEviction);
}

[Theory]
[InlineData(0)]
[InlineData(-1)]
public void LruBrushCacheCtorThrowsWhenCapacityIsNotPositive(int capacity)
{
Assert.Throws<ArgumentOutOfRangeException>(() => new LRUBrushCache(capacity));
}

[Fact]
public void LruBrushCacheSeededCtorThrowsWhenBrushesAreNull()
{
Assert.Throws<ArgumentNullException>(() => new LRUBrushCache(1, null));
}

[Fact]
public void LruBrushCacheSeededCtorThrowsWhenBrushCountExceedsCapacity()
{
var brushes = new Dictionary<Color, ImmutableBrush>
{
[Colors.Red] = new ImmutableBrush(Colors.Red),
[Colors.Green] = new ImmutableBrush(Colors.Green),
};

Assert.Throws<ArgumentException>(() => new LRUBrushCache(1, brushes));
}

[Fact]
public void LruBrushCacheSeededCtorReusesSeededBrushInstances()
{
var red = new ImmutableBrush(Colors.Red);
var green = new ImmutableBrush(Colors.Green);

var brushes = new Dictionary<Color, ImmutableBrush>
{
[Colors.Red] = red,
[Colors.Green] = green,
};

var cache = new LRUBrushCache(2, brushes);

Assert.Same(red, cache.Get(Colors.Red));
Assert.Same(green, cache.Get(Colors.Green));
}

[Fact]
public void LruBrushCacheEvictsLeastRecentlyUsedEntry()
{
var cache = new LRUBrushCache(2);

var red = cache.Get(Colors.Red);
var green = cache.Get(Colors.Green);

cache.Get(Colors.Red);
var blue = cache.Get(Colors.Blue);

Assert.Same(red, cache.Get(Colors.Red));
Assert.Same(blue, cache.Get(Colors.Blue));
Assert.NotSame(green, cache.Get(Colors.Green));
}

static object GetInnerCache(CacheWithSwitch cache)
{
var field = typeof(CacheWithSwitch).GetField("_cache", BindingFlags.Instance | BindingFlags.NonPublic);
return field?.GetValue(cache) ?? throw new InvalidOperationException("Cache backing field was not found.");
}
}
}
Loading
Loading