Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
86 changes: 58 additions & 28 deletions src/Controls/src/Core/BindableObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Microsoft.Extensions.Logging;
using Microsoft.Maui.Controls.Internals;
using Microsoft.Maui.Dispatching;
Expand Down Expand Up @@ -39,8 +38,8 @@ public BindableObject()
}

internal ushort _triggerCount = 0;
internal Dictionary<TriggerBase, SetterSpecificity> _triggerSpecificity = new();
readonly Dictionary<int, BindablePropertyContext> _properties = new(4);
internal Dictionary<TriggerBase, SetterSpecificity> _triggerSpecificity = new Dictionary<TriggerBase, SetterSpecificity>();
readonly Dictionary<BindableProperty, BindablePropertyContext> _properties = new Dictionary<BindableProperty, BindablePropertyContext>(4);
bool _applying;
WeakReference _inheritedContext;

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

internal LocalValueEnumerator GetLocalValueEnumerator() => new LocalValueEnumerator(this);

internal sealed class LocalValueEnumerator : IEnumerator<LocalValueEntry>
{
Dictionary<BindableProperty, BindablePropertyContext>.Enumerator _propertiesEnumerator;
internal LocalValueEnumerator(BindableObject bindableObject) => _propertiesEnumerator = bindableObject._properties.GetEnumerator();

object IEnumerator.Current => Current;
public LocalValueEntry Current { get; private set; }

public bool MoveNext()
{
if (_propertiesEnumerator.MoveNext())
{
Current = new LocalValueEntry(_propertiesEnumerator.Current.Key, _propertiesEnumerator.Current.Value.Values.GetValue(), _propertiesEnumerator.Current.Value.Attributes);
return true;
}
return false;
}

public void Dispose() => _propertiesEnumerator.Dispose();

void IEnumerator.Reset()
{
((IEnumerator)_propertiesEnumerator).Reset();
Current = null;
}
}

internal sealed class LocalValueEntry
{
internal LocalValueEntry(BindableProperty property, object value, BindableContextAttributes attributes)
{
Property = property;
Value = value;
Attributes = attributes;
}

public BindableProperty Property { get; }
public object Value { get; }
public BindableContextAttributes Attributes { get; }
}

internal (bool IsSet, T Value)[] GetValues<T>(BindableProperty[] propArray)
{
var properties = _properties;
Dictionary<BindableProperty, BindablePropertyContext> properties = _properties;
var resultArray = new (bool IsSet, T Value)[propArray.Length];

for (int i = 0; i < propArray.Length; i++)
{
ref var result = ref resultArray[i];
if (properties.TryGetValue(propArray[i].InternalId, out var context))
if (properties.TryGetValue(propArray[i], out var context))
{
var pair = context.Values.GetSpecificityAndValue();
result.IsSet = pair.Key != SetterSpecificity.DefaultValue;
result.Value = (T)pair.Value;
resultArray[i].IsSet = pair.Key != SetterSpecificity.DefaultValue;
resultArray[i].Value = (T)pair.Value;
}
else
{
resultArray[i].IsSet = false;
resultArray[i].Value = default(T);
}
}

Expand Down Expand Up @@ -716,7 +762,7 @@ static void BindingContextPropertyChanged(BindableObject bindable, object oldval
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
BindablePropertyContext CreateContext(BindableProperty property)
BindablePropertyContext CreateAndAddContext(BindableProperty property)
{
var defaultValueCreator = property.DefaultValueCreator;
var context = new BindablePropertyContext { Property = property };
Expand All @@ -725,31 +771,15 @@ BindablePropertyContext CreateContext(BindableProperty property)
if (defaultValueCreator != null)
context.Attributes = BindableContextAttributes.IsDefaultValueCreated;

_properties.Add(property, context);
return context;
}

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

[MethodImpl(MethodImplOptions.AggressiveInlining)]
BindablePropertyContext GetOrCreateContext(BindableProperty property)
{
#if NETSTANDARD
var context = GetContext(property);
if (context is null)
{
context = CreateContext(property);
_properties.Add(property.InternalId, context);
}
#else
ref var context = ref CollectionsMarshal.GetValueRefOrAddDefault(_properties, property.InternalId, out var exists);
if (!exists)
{
context = CreateContext(property);
}
#endif
return context;
}
BindablePropertyContext GetOrCreateContext(BindableProperty property) => GetContext(property) ?? CreateAndAddContext(property);

void RemoveBinding(BindableProperty property, BindablePropertyContext context, SetterSpecificity specificity)
{
Expand Down
6 changes: 0 additions & 6 deletions src/Controls/src/Core/BindableProperty.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Reflection;
using System.Threading;
using Microsoft.Maui.Controls.Xaml;
using Microsoft.Maui.Graphics;
using Microsoft.Maui.Graphics.Converters;
Expand Down Expand Up @@ -182,9 +181,6 @@ public sealed class BindableProperty
/// <summary>A sentinel object used to indicate that a BindableProperty value has not been set.</summary>
public static readonly object UnsetValue = new object();

private static int _nextInternalId = int.MinValue;
internal readonly int InternalId;

BindableProperty(string propertyName, [DynamicallyAccessedMembers(ReturnTypeMembers)] Type returnType, [DynamicallyAccessedMembers(DeclaringTypeMembers)] Type declaringType, object defaultValue, BindingMode defaultBindingMode = BindingMode.OneWay,
ValidateValueDelegate validateValue = null, BindingPropertyChangedDelegate propertyChanged = null, BindingPropertyChangingDelegate propertyChanging = null,
CoerceValueDelegate coerceValue = null, BindablePropertyBindingChanging bindingChanging = null, bool isReadOnly = false, CreateDefaultValueDelegate defaultValueCreator = null)
Expand All @@ -195,8 +191,6 @@ public sealed class BindableProperty
throw new ArgumentNullException(nameof(returnType));
if (declaringType is null)
throw new ArgumentNullException(nameof(declaringType));

InternalId = Interlocked.Increment(ref _nextInternalId);

// don't use Enum.IsDefined as its redonkulously expensive for what it does
if (defaultBindingMode != BindingMode.Default && defaultBindingMode != BindingMode.OneWay && defaultBindingMode != BindingMode.OneWayToSource && defaultBindingMode != BindingMode.TwoWay && defaultBindingMode != BindingMode.OneTime)
Expand Down
94 changes: 94 additions & 0 deletions src/Controls/tests/Core.UnitTests/BindableObjectUnitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1564,6 +1564,100 @@ public void GetValues()
Assert.Equal(5, values[2]);
}

[Fact]
public void GetValuesReturnsSetStateAndValue()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ Testing — These two new tests add useful coverage for the restored paths, but they pass under both the optimized (#33584) and reverted implementations, so they would not have caught the #33584 regression:

If the goal (per @PureWeen's request) is a test that fails without this revert, the most likely #33584 culprit was the ref from CollectionsMarshal.GetValueRefOrAddDefault being invalidated when the dictionary resizes during a re-entrant add — e.g. a DefaultValueCreator that sets several other properties mid-evaluation. A test that forces that re-entrancy would fail against #33584 and pass here.

(This root cause is a hypothesis; the PR frames the revert as precautionary. If no specific failure was reproduced, it's worth stating that explicitly in the thread rather than implying these are true regression tests.)

Flagged by: 3/3 reviewers

{
var prop = BindableProperty.Create("Foo", typeof(int), typeof(MockBindable), 0);
var prop1 = BindableProperty.Create("Foo1", typeof(int), typeof(MockBindable), 1);
var prop2 = BindableProperty.Create("Foo2", typeof(int), typeof(MockBindable), 2);
var bindable = new MockBindable();

bindable.SetValue(prop, 3);
bindable.SetValue(prop2, 5);

var values = bindable.GetValues<int>(new[] { prop, prop1, prop2 });

Assert.Equal(3, values.Length);
Assert.True(values[0].IsSet);
Assert.Equal(3, values[0].Value);
Assert.False(values[1].IsSet);
Assert.Equal(0, values[1].Value);
Assert.True(values[2].IsSet);
Assert.Equal(5, values[2].Value);
}

[Fact]
public void LocalValueEnumeratorReturnsLocallySetValues()
{
var prop = BindableProperty.Create("Foo", typeof(int), typeof(MockBindable), 0);
var prop1 = BindableProperty.Create("Foo1", typeof(int), typeof(MockBindable), 1);
var prop2 = BindableProperty.Create("Foo2", typeof(int), typeof(MockBindable), 2);
var bindable = new MockBindable();

bindable.SetValue(prop, 3);
bindable.SetValue(prop2, 5);

var sawFirst = false;
var sawSecond = false;

using var enumerator = bindable.GetLocalValueEnumerator();
while (enumerator.MoveNext())
{
var current = enumerator.Current;

if (current.Property == prop)
{
sawFirst = true;
Assert.Equal(3, current.Value);
}
else if (current.Property == prop2)
{
sawSecond = true;
Assert.Equal(5, current.Value);
}
}

Assert.True(sawFirst);
Assert.True(sawSecond);
}

[Fact]
public void DefaultValueCreatorCachesValueWhenReentrantPropertyAddsResizeStore()
{
var reentrantProperties = new BindableProperty[8];
for (var i = 0; i < reentrantProperties.Length; i++)
{
reentrantProperties[i] = BindableProperty.Create($"Reentrant{i}", typeof(int), typeof(MockBindable), 0);
}

var defaultValueCreatorInvocations = 0;
var propertyWithCreator = BindableProperty.Create(
"ReentrantDefault",
typeof(int),
typeof(MockBindable),
0,
defaultValueCreator: b =>
{
defaultValueCreatorInvocations++;
for (var i = 0; i < reentrantProperties.Length; i++)
{
b.SetValue(reentrantProperties[i], i + 1);
}

return 42;
});

var bindable = new MockBindable();

var first = (int)bindable.GetValue(propertyWithCreator);
var second = (int)bindable.GetValue(propertyWithCreator);

Assert.Equal(42, first);
Assert.Equal(42, second);
Assert.Equal(1, defaultValueCreatorInvocations);
Assert.True(bindable.IsSet(propertyWithCreator));
}

class BindingContextConverter
: IValueConverter
{
Expand Down
38 changes: 0 additions & 38 deletions src/Core/tests/Benchmarks/Benchmarks/BindableObjectBenchmarker.cs

This file was deleted.

Loading