Skip to content

Commit

Permalink
perf(memory): Fix datacontext propagation on precedence level change
Browse files Browse the repository at this point in the history 
This change ensures that the DependencyProperty value precedence changes do not keep references to instances into inaccessible precedence levels.

This change also introduces more flags to get states of DependencyProperty and avoid type casts.
  • Loading branch information
@jeromelaban
jeromelaban committed Feb 7, 2023
1 parent ab2ee2a commit 4f7300a
Show file tree
Hide file tree
Showing 6 changed files with 312 additions and 76 deletions.
98 changes: 98 additions & 0 deletions src/Uno.UI.Tests/DependencyProperty/Given_DependencyProperty.Propagation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,104 @@ public void When_ControlTemplate_And_Animation()

Assert.IsNotNull(anim);
}

[TestMethod]
public async Task When_PrecedenceChanged_Then_Released()
{
var SUT = new ContentControl();

WeakReference Build()
{
var dc = new object();
SUT.DataContext = dc;
SUT.SetValue(ContentControl.ForegroundProperty, new SolidColorBrush(Windows.UI.Colors.Red));

SUT.DataContext = null;

return new(dc);
}

await AssertCollectedReference(Build());
}


[TestMethod]
public async Task When_PrecedenceChanged_And_Back_Then_Restored()
{
var SUT = new ContentControl();

WeakReference Build()
{
var dc = new object();
SUT.DataContext = dc;

var originalBrush = SUT.Foreground as Brush;
var newBrush = new SolidColorBrush(Windows.UI.Colors.Red);

SUT.SetValue(ContentControl.ForegroundProperty, newBrush);

Assert.IsNull(originalBrush.DataContext);
Assert.IsNotNull(newBrush.DataContext);

SUT.ClearValue(ContentControl.ForegroundProperty);

Assert.AreEqual(dc, originalBrush.DataContext);
Assert.IsNull(newBrush.DataContext);

SUT.DataContext = null;

return new(dc);
}

await AssertCollectedReference(Build());
}

[TestMethod]
public async Task When_PrecedenceChanged_To_Null_And_Back_Then_Restored()
{
var SUT = new ContentControl();

WeakReference Build()
{
var dc = new object();
SUT.DataContext = dc;

var originalBrush = SUT.Foreground as Brush;

SUT.SetValue(ContentControl.ForegroundProperty, null);

Assert.IsNull(originalBrush.DataContext);

SUT.ClearValue(ContentControl.ForegroundProperty);

Assert.AreEqual(dc, originalBrush.DataContext);

SUT.DataContext = null;

return new(dc);
}

await AssertCollectedReference(Build());
}

private async Task AssertCollectedReference(WeakReference reference)
{
var sw = Stopwatch.StartNew();
while (sw.Elapsed < TimeSpan.FromSeconds(2))
{
GC.Collect(2);
GC.WaitForPendingFinalizers();

if (!reference.IsAlive)
{
return;
}

await Task.Delay(100);
}

Assert.IsFalse(reference.IsAlive);
}
}

public partial class MyObject : DependencyObject
Expand Down
47 changes: 8 additions & 39 deletions src/Uno.UI/UI/Xaml/DependencyObjectStore.Binder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -566,24 +566,19 @@ private void OnDependencyPropertyChanged(DependencyPropertyDetails propertyDetai
{
SetBindingValue(propertyDetails, args.NewValue);

GetPropertyInheritanceConfiguration(
propertyDetails: propertyDetails,
args: args,
hasValueInherits: out var hasValueInherits,
hasValueDoesNotInherit: out var hasValueDoesNotInherits
);

if (!hasValueDoesNotInherits)
if (!propertyDetails.HasValueDoesNotInherit)
{
var newValueAsProvider = args.NewValue as IDependencyObjectStoreProvider;

if (hasValueInherits)
if (propertyDetails.HasValueInherits)
{
if (newValueAsProvider != null)
if (newValueAsProvider is not null)
{
SetChildrenBindableValue(
propertyDetails,
newValueAsProvider.Store.Parent != ActualInstance ? args.NewValue : null);

// Ensure DataContext propagation loops cannot happen
ReferenceEquals(newValueAsProvider.Store.Parent, ActualInstance) ? null : args.NewValue);
}
else
{
Expand All @@ -592,8 +587,8 @@ private void OnDependencyPropertyChanged(DependencyPropertyDetails propertyDetai
}
else
{
if (newValueAsProvider is { }
&& !(newValueAsProvider is UIElement))
if (newValueAsProvider is not null
&& newValueAsProvider is not UIElement)
{
SetChildrenBindableValue(propertyDetails, newValueAsProvider);
}
Expand All @@ -604,32 +599,6 @@ private void OnDependencyPropertyChanged(DependencyPropertyDetails propertyDetai
private void SetChildrenBindableValue(DependencyPropertyDetails propertyDetails, object? value)
=> _childrenBindable[GetOrCreateChildBindablePropertyIndex(propertyDetails.Property)] = value;

private void GetPropertyInheritanceConfiguration(
DependencyPropertyDetails propertyDetails,
DependencyPropertyChangedEventArgs args,
out bool hasValueInherits,
out bool hasValueDoesNotInherit)
{
if (propertyDetails.Property == _templatedParentProperty
|| propertyDetails.Property == _dataContextProperty)
{
// TemplatedParent is a DependencyObject but does not propagate datacontext
hasValueInherits = false;
hasValueDoesNotInherit = true;
return;
}

if (propertyDetails.Metadata is FrameworkPropertyMetadata propertyMetadata)
{
hasValueInherits = propertyMetadata.Options.HasValueInheritsDataContext();
hasValueDoesNotInherit = propertyMetadata.Options.HasValueDoesNotInheritDataContext();
return;
}

hasValueInherits = false;
hasValueDoesNotInherit = false;
}

/// <summary>
/// Gets or create an index in the <see cref="_childrenBindable"/> list, to avoid enumerating <see cref="_childrenBindableMap"/>.
/// </summary>
Expand Down
91 changes: 75 additions & 16 deletions src/Uno.UI/UI/Xaml/DependencyObjectStore.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,8 @@ private void InnerSetValue(DependencyProperty property, object? value, Dependenc
OnDataContextChanged(value, newValue, precedence);
}

TryApplyDataContextOnPrecedenceChange(property, propertyDetails, previousValue, previousPrecedence, newValue, newPrecedence);

TryUpdateInheritedAttachedProperty(property, propertyDetails);

if (this.Log().IsEnabled(Uno.Foundation.Logging.LogLevel.Debug))
Expand Down Expand Up @@ -518,6 +520,77 @@ private void InnerSetValue(DependencyProperty property, object? value, Dependenc
}
}

/// <summary>
/// Tries to apply the DataContext to the new and previous values when DataContext Value is inherited
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void TryApplyDataContextOnPrecedenceChange(
DependencyProperty property,
DependencyPropertyDetails propertyDetails,
object? previousValue,
DependencyPropertyValuePrecedences previousPrecedence,
object? newValue,
DependencyPropertyValuePrecedences newPrecedence)
{
if (property.IsUnoType
&& propertyDetails.HasValueInherits
&& !propertyDetails.HasValueDoesNotInherit)
{
// This block is used to synchronize the DataContext property of DependencyProperty values marked as
// being able to inherit the DataContext.
// When a DependencyProperty has a bindable default value instance (e.g. ContentControl.Foreground), and that the
// ContentControl has a DataContext, setting a local precedence value should clear the default value instance DataContext
// property. This avoid inaccessible DataContext instances to be strongly kept alive by a lower precedence that
// may never be used again.

// If a value is set with a higher precedence, the lower precedence DataContext must be cleared
if (newPrecedence < previousPrecedence)
{
if (previousValue is IDependencyObjectStoreProvider childProviderClear)
{
// Clears the DataContext of the previous precedence value
childProviderClear.Store.ClearInheritedDataContext();
}

if (newValue is IDependencyObjectStoreProvider childProviderClearNewValue
&& !ReferenceEquals(childProviderClearNewValue.Store.Parent, ActualInstance))
{
// Sets the DataContext of the new precedence value
childProviderClearNewValue.Store.RestoreInheritedDataContext(_properties.DataContextPropertyDetails.GetValue());
}
}

// If a value is set with a lower precedence, the higher precedence DataContext must be set to the current DataContext
if (newPrecedence > previousPrecedence)
{
if (newValue is IDependencyObjectStoreProvider childProviderSet
&& !ReferenceEquals(childProviderSet.Store.Parent, ActualInstance))
{
// Sets the DataContext of the new precedence value
childProviderSet.Store.RestoreInheritedDataContext(_properties.DataContextPropertyDetails.GetValue());
}

if (previousValue is IDependencyObjectStoreProvider childProviderSetNewValue)
{
// Clears the DataContext of the previous precedence value
childProviderSetNewValue.Store.ClearInheritedDataContext();
}
}
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void ClearInheritedDataContext()
{
ClearValue(_dataContextProperty, DependencyPropertyValuePrecedences.Inheritance);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void RestoreInheritedDataContext(object? dataContext)
{
SetValue(_dataContextProperty, dataContext, DependencyPropertyValuePrecedences.Inheritance);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void TryClearBinding(object? value, DependencyPropertyDetails propertyDetails)
{
Expand Down Expand Up @@ -1181,7 +1254,7 @@ private void CleanupInheritedProperties()
{
var propertyDetails = _properties.GetPropertyDetails(localProperty);

if (HasInherits(propertyDetails))
if (propertyDetails.HasInherits)
{
return (localProperty, propertyDetails);
}
Expand All @@ -1194,8 +1267,7 @@ private void CleanupInheritedProperties()
else if (
property.IsAttached
&& _properties.FindPropertyDetails(property) is DependencyPropertyDetails attachedDetails
&& HasInherits(attachedDetails)
)
&& attachedDetails.HasInherits)
{
return (property, attachedDetails);
}
Expand Down Expand Up @@ -1648,19 +1720,6 @@ private static bool IsAncestor(DependencyObject? instance, AncestorsDictionary m
return isAncestor;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private bool HasInherits(DependencyPropertyDetails propertyDetails)
{
var metadata = propertyDetails.Metadata;

if (metadata is FrameworkPropertyMetadata frameworkMetadata)
{
return frameworkMetadata.Options.HasInherits();
}

return false;
}

public DependencyObject? ActualInstance
=> _hardOriginalObjectRef ?? _originalObjectRef.Target as DependencyObject;

Expand Down
Loading

0 comments on commit 4f7300a

Please sign in to comment.