Skip to content

Commit f9b311b

Browse files
CopilotrmarinhoPureWeen
authored
Revert PR #33584: restore BindableObject property-context lookup model (#35970)
> [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could <a href="https://github.com/dotnet/maui/wiki/Testing-PR-Builds">test the resulting artifacts</a> from this PR and let us know in a comment if this change resolves your issue. Thank you! This reverts the `BindableObject` micro-optimization changes from PR #33584 on `release/10.0.1xx-sr8` due to regression risk. The revert restores the previous property-context storage/access behavior and removes the `InternalId` path added in that PR. - **BindableObject revert (Controls.Core)** - Restore `_properties` to `Dictionary<BindableProperty, BindablePropertyContext>`. - Remove `CollectionsMarshal`-based lookup/add path and `NETSTANDARD` split logic. - Restore `CreateAndAddContext` behavior (`_properties.Add(property, context)`). - Revert `GetContext` and `GetValues<T>` to `BindableProperty`-keyed lookups. - Restore `GetLocalValueEnumerator()`, `LocalValueEnumerator`, and `LocalValueEntry`. - **BindableProperty revert (Controls.Core)** - Remove `InternalId` infrastructure: - `using System.Threading` - `_nextInternalId` - `InternalId` field - `Interlocked.Increment(...)` assignment in constructor - **Benchmark cleanup** - Remove `src/Core/tests/Benchmarks/Benchmarks/BindableObjectBenchmarker.cs` introduced by PR #33584. - **Regression tests added (Controls.Core.UnitTests)** - Add `GetValuesReturnsSetStateAndValue` to validate `GetValues<T>` preserves expected set-state/value semantics for set vs unset properties. - Add `LocalValueEnumeratorReturnsLocallySetValues` to validate the restored local value enumeration path (`GetLocalValueEnumerator` / `LocalValueEntry`) over locally set values. - Add `DefaultValueCreatorCachesValueWhenReentrantPropertyAddsResizeStore` to validate re-entrant `DefaultValueCreator` execution (including additional property sets that grow the backing store) still caches the created value and invokes the creator once. Example of restored lookup path: ```csharp readonly Dictionary<BindableProperty, BindablePropertyContext> _properties = new Dictionary<BindableProperty, BindablePropertyContext>(4); internal BindablePropertyContext GetContext(BindableProperty property) => _properties.TryGetValue(property, out var result) ? result : null; BindablePropertyContext GetOrCreateContext(BindableProperty property) => GetContext(property) ?? CreateAndAddContext(property); ``` --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: rmarinho <1235097+rmarinho@users.noreply.github.com> Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
1 parent a11840b commit f9b311b

4 files changed

Lines changed: 152 additions & 72 deletions

File tree

src/Controls/src/Core/BindableObject.cs

Lines changed: 58 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
using System.Diagnostics.CodeAnalysis;
77
using System.Linq;
88
using System.Runtime.CompilerServices;
9-
using System.Runtime.InteropServices;
109
using Microsoft.Extensions.Logging;
1110
using Microsoft.Maui.Controls.Internals;
1211
using Microsoft.Maui.Dispatching;
@@ -39,8 +38,8 @@ public BindableObject()
3938
}
4039

4140
internal ushort _triggerCount = 0;
42-
internal Dictionary<TriggerBase, SetterSpecificity> _triggerSpecificity = new();
43-
readonly Dictionary<int, BindablePropertyContext> _properties = new(4);
41+
internal Dictionary<TriggerBase, SetterSpecificity> _triggerSpecificity = new Dictionary<TriggerBase, SetterSpecificity>();
42+
readonly Dictionary<BindableProperty, BindablePropertyContext> _properties = new Dictionary<BindableProperty, BindablePropertyContext>(4);
4443
bool _applying;
4544
WeakReference _inheritedContext;
4645

@@ -173,19 +172,66 @@ public object GetValue(BindableProperty property)
173172
return context == null ? property.DefaultValue : context.Values.GetValue();
174173
}
175174

175+
internal LocalValueEnumerator GetLocalValueEnumerator() => new LocalValueEnumerator(this);
176+
177+
internal sealed class LocalValueEnumerator : IEnumerator<LocalValueEntry>
178+
{
179+
Dictionary<BindableProperty, BindablePropertyContext>.Enumerator _propertiesEnumerator;
180+
internal LocalValueEnumerator(BindableObject bindableObject) => _propertiesEnumerator = bindableObject._properties.GetEnumerator();
181+
182+
object IEnumerator.Current => Current;
183+
public LocalValueEntry Current { get; private set; }
184+
185+
public bool MoveNext()
186+
{
187+
if (_propertiesEnumerator.MoveNext())
188+
{
189+
Current = new LocalValueEntry(_propertiesEnumerator.Current.Key, _propertiesEnumerator.Current.Value.Values.GetValue(), _propertiesEnumerator.Current.Value.Attributes);
190+
return true;
191+
}
192+
return false;
193+
}
194+
195+
public void Dispose() => _propertiesEnumerator.Dispose();
196+
197+
void IEnumerator.Reset()
198+
{
199+
((IEnumerator)_propertiesEnumerator).Reset();
200+
Current = null;
201+
}
202+
}
203+
204+
internal sealed class LocalValueEntry
205+
{
206+
internal LocalValueEntry(BindableProperty property, object value, BindableContextAttributes attributes)
207+
{
208+
Property = property;
209+
Value = value;
210+
Attributes = attributes;
211+
}
212+
213+
public BindableProperty Property { get; }
214+
public object Value { get; }
215+
public BindableContextAttributes Attributes { get; }
216+
}
217+
176218
internal (bool IsSet, T Value)[] GetValues<T>(BindableProperty[] propArray)
177219
{
178-
var properties = _properties;
220+
Dictionary<BindableProperty, BindablePropertyContext> properties = _properties;
179221
var resultArray = new (bool IsSet, T Value)[propArray.Length];
180222

181223
for (int i = 0; i < propArray.Length; i++)
182224
{
183-
ref var result = ref resultArray[i];
184-
if (properties.TryGetValue(propArray[i].InternalId, out var context))
225+
if (properties.TryGetValue(propArray[i], out var context))
185226
{
186227
var pair = context.Values.GetSpecificityAndValue();
187-
result.IsSet = pair.Key != SetterSpecificity.DefaultValue;
188-
result.Value = (T)pair.Value;
228+
resultArray[i].IsSet = pair.Key != SetterSpecificity.DefaultValue;
229+
resultArray[i].Value = (T)pair.Value;
230+
}
231+
else
232+
{
233+
resultArray[i].IsSet = false;
234+
resultArray[i].Value = default(T);
189235
}
190236
}
191237

@@ -716,7 +762,7 @@ static void BindingContextPropertyChanged(BindableObject bindable, object oldval
716762
}
717763

718764
[MethodImpl(MethodImplOptions.AggressiveInlining)]
719-
BindablePropertyContext CreateContext(BindableProperty property)
765+
BindablePropertyContext CreateAndAddContext(BindableProperty property)
720766
{
721767
var defaultValueCreator = property.DefaultValueCreator;
722768
var context = new BindablePropertyContext { Property = property };
@@ -725,31 +771,15 @@ BindablePropertyContext CreateContext(BindableProperty property)
725771
if (defaultValueCreator != null)
726772
context.Attributes = BindableContextAttributes.IsDefaultValueCreated;
727773

774+
_properties.Add(property, context);
728775
return context;
729776
}
730777

731778
[MethodImpl(MethodImplOptions.AggressiveInlining)]
732-
internal BindablePropertyContext GetContext(BindableProperty property) => _properties.TryGetValue(property.InternalId, out var result) ? result : null;
779+
internal BindablePropertyContext GetContext(BindableProperty property) => _properties.TryGetValue(property, out var result) ? result : null;
733780

734781
[MethodImpl(MethodImplOptions.AggressiveInlining)]
735-
BindablePropertyContext GetOrCreateContext(BindableProperty property)
736-
{
737-
#if NETSTANDARD
738-
var context = GetContext(property);
739-
if (context is null)
740-
{
741-
context = CreateContext(property);
742-
_properties.Add(property.InternalId, context);
743-
}
744-
#else
745-
ref var context = ref CollectionsMarshal.GetValueRefOrAddDefault(_properties, property.InternalId, out var exists);
746-
if (!exists)
747-
{
748-
context = CreateContext(property);
749-
}
750-
#endif
751-
return context;
752-
}
782+
BindablePropertyContext GetOrCreateContext(BindableProperty property) => GetContext(property) ?? CreateAndAddContext(property);
753783

754784
void RemoveBinding(BindableProperty property, BindablePropertyContext context, SetterSpecificity specificity)
755785
{

src/Controls/src/Core/BindableProperty.cs

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
using System.Diagnostics.CodeAnalysis;
77
using System.Globalization;
88
using System.Reflection;
9-
using System.Threading;
109
using Microsoft.Maui.Controls.Xaml;
1110
using Microsoft.Maui.Graphics;
1211
using Microsoft.Maui.Graphics.Converters;
@@ -182,9 +181,6 @@ public sealed class BindableProperty
182181
/// <summary>A sentinel object used to indicate that a BindableProperty value has not been set.</summary>
183182
public static readonly object UnsetValue = new object();
184183

185-
private static int _nextInternalId = int.MinValue;
186-
internal readonly int InternalId;
187-
188184
BindableProperty(string propertyName, [DynamicallyAccessedMembers(ReturnTypeMembers)] Type returnType, [DynamicallyAccessedMembers(DeclaringTypeMembers)] Type declaringType, object defaultValue, BindingMode defaultBindingMode = BindingMode.OneWay,
189185
ValidateValueDelegate validateValue = null, BindingPropertyChangedDelegate propertyChanged = null, BindingPropertyChangingDelegate propertyChanging = null,
190186
CoerceValueDelegate coerceValue = null, BindablePropertyBindingChanging bindingChanging = null, bool isReadOnly = false, CreateDefaultValueDelegate defaultValueCreator = null)
@@ -195,8 +191,6 @@ public sealed class BindableProperty
195191
throw new ArgumentNullException(nameof(returnType));
196192
if (declaringType is null)
197193
throw new ArgumentNullException(nameof(declaringType));
198-
199-
InternalId = Interlocked.Increment(ref _nextInternalId);
200194

201195
// don't use Enum.IsDefined as its redonkulously expensive for what it does
202196
if (defaultBindingMode != BindingMode.Default && defaultBindingMode != BindingMode.OneWay && defaultBindingMode != BindingMode.OneWayToSource && defaultBindingMode != BindingMode.TwoWay && defaultBindingMode != BindingMode.OneTime)

src/Controls/tests/Core.UnitTests/BindableObjectUnitTests.cs

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1564,6 +1564,100 @@ public void GetValues()
15641564
Assert.Equal(5, values[2]);
15651565
}
15661566

1567+
[Fact]
1568+
public void GetValuesReturnsSetStateAndValue()
1569+
{
1570+
var prop = BindableProperty.Create("Foo", typeof(int), typeof(MockBindable), 0);
1571+
var prop1 = BindableProperty.Create("Foo1", typeof(int), typeof(MockBindable), 1);
1572+
var prop2 = BindableProperty.Create("Foo2", typeof(int), typeof(MockBindable), 2);
1573+
var bindable = new MockBindable();
1574+
1575+
bindable.SetValue(prop, 3);
1576+
bindable.SetValue(prop2, 5);
1577+
1578+
var values = bindable.GetValues<int>(new[] { prop, prop1, prop2 });
1579+
1580+
Assert.Equal(3, values.Length);
1581+
Assert.True(values[0].IsSet);
1582+
Assert.Equal(3, values[0].Value);
1583+
Assert.False(values[1].IsSet);
1584+
Assert.Equal(0, values[1].Value);
1585+
Assert.True(values[2].IsSet);
1586+
Assert.Equal(5, values[2].Value);
1587+
}
1588+
1589+
[Fact]
1590+
public void LocalValueEnumeratorReturnsLocallySetValues()
1591+
{
1592+
var prop = BindableProperty.Create("Foo", typeof(int), typeof(MockBindable), 0);
1593+
var prop1 = BindableProperty.Create("Foo1", typeof(int), typeof(MockBindable), 1);
1594+
var prop2 = BindableProperty.Create("Foo2", typeof(int), typeof(MockBindable), 2);
1595+
var bindable = new MockBindable();
1596+
1597+
bindable.SetValue(prop, 3);
1598+
bindable.SetValue(prop2, 5);
1599+
1600+
var sawFirst = false;
1601+
var sawSecond = false;
1602+
1603+
using var enumerator = bindable.GetLocalValueEnumerator();
1604+
while (enumerator.MoveNext())
1605+
{
1606+
var current = enumerator.Current;
1607+
1608+
if (current.Property == prop)
1609+
{
1610+
sawFirst = true;
1611+
Assert.Equal(3, current.Value);
1612+
}
1613+
else if (current.Property == prop2)
1614+
{
1615+
sawSecond = true;
1616+
Assert.Equal(5, current.Value);
1617+
}
1618+
}
1619+
1620+
Assert.True(sawFirst);
1621+
Assert.True(sawSecond);
1622+
}
1623+
1624+
[Fact]
1625+
public void DefaultValueCreatorCachesValueWhenReentrantPropertyAddsResizeStore()
1626+
{
1627+
var reentrantProperties = new BindableProperty[8];
1628+
for (var i = 0; i < reentrantProperties.Length; i++)
1629+
{
1630+
reentrantProperties[i] = BindableProperty.Create($"Reentrant{i}", typeof(int), typeof(MockBindable), 0);
1631+
}
1632+
1633+
var defaultValueCreatorInvocations = 0;
1634+
var propertyWithCreator = BindableProperty.Create(
1635+
"ReentrantDefault",
1636+
typeof(int),
1637+
typeof(MockBindable),
1638+
0,
1639+
defaultValueCreator: b =>
1640+
{
1641+
defaultValueCreatorInvocations++;
1642+
for (var i = 0; i < reentrantProperties.Length; i++)
1643+
{
1644+
b.SetValue(reentrantProperties[i], i + 1);
1645+
}
1646+
1647+
return 42;
1648+
});
1649+
1650+
var bindable = new MockBindable();
1651+
1652+
var first = (int)bindable.GetValue(propertyWithCreator);
1653+
var second = (int)bindable.GetValue(propertyWithCreator);
1654+
1655+
Assert.Equal(42, first);
1656+
Assert.Equal(42, second);
1657+
Assert.Equal(1, defaultValueCreatorInvocations);
1658+
Assert.True(bindable.IsSet(propertyWithCreator));
1659+
}
1660+
15671661
class BindingContextConverter
15681662
: IValueConverter
15691663
{

src/Core/tests/Benchmarks/Benchmarks/BindableObjectBenchmarker.cs

Lines changed: 0 additions & 38 deletions
This file was deleted.

0 commit comments

Comments
 (0)