Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make styled property storage typed #3255

Merged
merged 60 commits into from Jan 23, 2020
Merged

Make styled property storage typed #3255

merged 60 commits into from Jan 23, 2020

Conversation

@grokys
Copy link
Member

grokys commented Nov 14, 2019

What does the pull request do?

Previously, when a value was set on a styled property, that property was boxed and added to the ValueStore. This PR refactors the value store and the API of AvaloniaObject to store values without boxing.

IAvaloniaObject has been changed to have two overloads of each get/set/bind/clear method:

  • an overload which takes StyledPropertyBase<T>
  • an overload which takes DirectPropertyBase<T>

In addition, the IObservable<> passed to Bind has changed to an IObservable<BindingValue<T>>: BindingValue<T> is a struct which allows representation of extra state in a typed manner:

  • A simple value
  • An unset value
  • Do nothing
  • Binding errors (with optional fallback value)
  • Data validation errors (with optional fallback value)

Because the get/set/bind/clear methods are now typed on the value T, the value store can now store values without boxing.

Untyped AvaloniaProperty access is still allowed and calls are routed through the AvaloniaProperty itself to the correct generic method.

Bindings which produce an IObservable<T> are wrapped in an adapter to convert them to IObservable<BindingValue<T>>.

Benchmarks

master:

|                                Method |     Mean |     Error |    StdDev |   Gen 0 | Gen 1 | Gen 2 | Allocated |
|-------------------------------------- |---------:|----------:|----------:|--------:|------:|------:|----------:|
|           Set_Int_Property_LocalValue | 19.93 us | 0.0541 us | 0.0452 us | 11.1084 |     - |     - |  10.45 KB |
|  Set_Int_Property_Multiple_Priorities | 72.47 us | 0.2524 us | 0.2361 us | 19.8975 |     - |     - |  18.73 KB |
|      Set_Int_Property_TemplatedParent | 27.86 us | 0.1242 us | 0.1101 us |  9.0637 |     - |     - |   8.52 KB |
|          Bind_Int_Property_LocalValue | 18.03 us | 0.2491 us | 0.2080 us |  9.3689 |     - |     - |    8.8 KB |
| Bind_Int_Property_Multiple_Priorities | 39.44 us | 0.3584 us | 0.3352 us | 21.9116 |     - |     - |  20.63 KB |

This PR:

|                                Method |       Mean |     Error |    StdDev |   Gen 0 | Gen 1 | Gen 2 | Allocated |
|-------------------------------------- |-----------:|----------:|----------:|--------:|------:|------:|----------:|
|           Set_Int_Property_LocalValue |  14.793 us | 0.0612 us | 0.0511 us |  0.2594 |     - |     - |     256 B |
|  Set_Int_Property_Multiple_Priorities | 217.471 us | 0.8287 us | 0.6470 us | 25.6348 |     - |     - |   24696 B |
|      Set_Int_Property_TemplatedParent |  34.932 us | 0.4928 us | 0.4610 us |  6.7139 |     - |     - |    6504 B |
|          Bind_Int_Property_LocalValue |   9.989 us | 0.0843 us | 0.0788 us |  0.4120 |     - |     - |     400 B |
| Bind_Int_Property_Multiple_Priorities |  41.421 us | 0.1188 us | 0.1053 us |  1.5259 |     - |     - |    1512 B |

Note that the Set_Int_Property_Multiple_Priorities and Set_Int_Property_TemplatedParent tests are now slower/allocate more due to the change in behavior listed under "Breaking Changes" -> "Values". However despite the fact they're storing multiple values instead of a single value, the total allocated memory is still a fraction of what it was before

Not addressed in this PR

  • All of our current infrastructure that produces bindings are still typed as IObservable<object> meaning that they have to be wrapped. This means that you won't see much perf improvement from this PR alone. Changes to fix this will come in later PRs.
  • The stackoverflow problems described in #855 and #824 are not addressed. We previously used DeferedSetter to get around these problems, but that had a non-zero performance impact. The fix for these issues will come in a separate PR to make reviewing of just that part more easy. Merged into this PR by #3258
  • More of the AvaloniaObject.GetValue/SetValue/Bind methods can be moved to extension methods, but that will require a change to the XAML compiler

Checklist

  • Added unit tests (if possible)?

Breaking changes

Properties

  • If your readonly property fields are declared as of type AvaloniaProperty<T> then you should change them to StyledProperty<> or DirectProperty<,> fields
  • The signature for OnPropertyChanged has changed and is now a generic method in order to avoid allocation and boxing. To cast the oldValue and newValue parameters to a concrete type, use .ValueOrDefault<T>() on Optional<T> and BindingValue<T>

Values

Previously all values set via SetValue were only active until a binding of the same or higher priority produced a value, in which case the SetValue value was lost.

This meant that style setters with needed to create a binding with a single value, otherwise the setter value would be lost when e.g. a setter with StyleTrigger binding was activated.

This PR changes that behavior: LocalValue priority values now work as before but other priority values are stored alongside bindings in the entries for the priority value and as such will not be overwritten by binding values. This will allow more efficient style values in a future PR as single values will no longer have to be created as single-value non-completing bindings

Binding

  • Many AvaloniaObject.Bind overloads have been moved to be extension methods, so you may have to add this. to your Bind() call when binding to this

Validation

Styled property value validation and coercion was a major performance bottleneck, because it imposed a metadata lookup on every styled property set and binding update, regardless of whether validation was enabled for the property (and for 99% of properties, it isn't)

In WPF/UWP value validation is defined outside of metadata, so we could bring validation back in a later PR in this form if needed. However even in WPF/UWP coercion imposes the overhead of a metadata lookup so it's unlikely we'd bring this back. The problem is less of an issue in Avalonia due to direct properties however.

#3287 implements WPF-style property validation and coercion and targets this branch. I've kept it as a separate PR for now to ease review. #3287 should be merged into this PR before this PR is merged. #3287 merged

Avalonia properties now take a separate validation and coercion callback the same as WPF. The validation callback cannot be overridden, though the coercion callback can.

Depends on #3287

grokys added 3 commits Nov 14, 2019
Major refactor of the Avalonia core to make the styled property store typed.
@grokys grokys force-pushed the refactor/value-store branch from 04c02b3 to 0cfa159 Nov 14, 2019
grokys added 3 commits Nov 14, 2019
It's not needed any more.
grokys added 4 commits Nov 14, 2019
Don't believe the new value given to us in the `AvaloniaPropertyChangedEventArgs`: it may have already changed. Instead, read the current value of the property from the object.
Alternative fix for binding stack overflow
Copy link
Member

MarchingCube left a comment

Glanced over most of your changes. I think it would be good to figure out if we can address LocalValue setter performance. After cobbling together a quick hack to store typed local values I was able to improve memory usage further and improve perf by like 10-15%.

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
Set_Int_Property_LocalValue 11.92 us 0.2315 us 0.2573 us 0.0610 - - 256 B
src/Avalonia.Base/Data/Optional.cs Show resolved Hide resolved
src/Avalonia.Base/Data/Optional.cs Outdated Show resolved Hide resolved
src/Avalonia.Base/DirectProperty.cs Outdated Show resolved Hide resolved
src/Avalonia.Base/ValueStore.cs Outdated Show resolved Hide resolved
src/Avalonia.Controls/Button.cs Outdated Show resolved Hide resolved
grokys added 3 commits Nov 16, 2019
Delete some files that should have been deleted in merge.
@mstr2

This comment has been minimized.

Copy link
Contributor

mstr2 commented Nov 18, 2019

Will this remove value coercion for StyledProperty altogether?

@grokys

This comment has been minimized.

Copy link
Member Author

grokys commented Nov 19, 2019

Will this remove value coercion for StyledProperty altogether?

It will, yes. Is this a problem for you? We may need to design a replacement before we commit this change. As I mentioned, the current way coercion works slows down every single property set (including values from bindings), so it'd probably have a different API/semantics.

@mstr2

This comment has been minimized.

Copy link
Contributor

mstr2 commented Nov 19, 2019

It will, yes. Is this a problem for you? We may need to design a replacement before we commit this change. As I mentioned, the current way coercion works slows down every single property set (including values from bindings), so it'd probably have a different API/semantics.

Yes, I’m using it for data consistency. It seems to me that it should be pretty easy to optimize the case where a property doesn’t use validation on any type by having a flag on the property that specifies whether any of its metadata has a validation function.

@mstr2

This comment has been minimized.

Copy link
Contributor

mstr2 commented Nov 19, 2019

Since this is a breaking API change anyway: did you consider moving the static registration methods to their respective subclass? I think it's a bit cleaner to write:

StyledProperty<double> FooProperty = StyledProperty.Register<...>(...);
DirectProperty<double> BarProperty = DirectProperty.Register<...>(...);
@grokys

This comment has been minimized.

Copy link
Member Author

grokys commented Nov 21, 2019

Since this is a breaking API change anyway: did you consider moving the static registration methods to their respective subclass? I think it's a bit cleaner to write:

We could move them and if necessary keep the existing deprecated methods, but this should be done in a separate PR. However, I'm not completely sure that it's a change worth making at this point, when everyone's code is using the existing methods...

grokys added 2 commits Nov 21, 2019
Further reduces allocated memory in the common case of only a local value being set.
@grokys

This comment has been minimized.

Copy link
Member Author

grokys commented Nov 21, 2019

@mstr2 regarding coercion/validation, any thoughts on the current API as compared to the WPF/UWP API where validation is a "static" callback and only coercion receives a reference to the control?

https://docs.microsoft.com/en-us/dotnet/framework/wpf/advanced/dependency-property-callbacks-and-validation

Not sure whether to change our API to match WPF/UWP or not.

Edit: UWP doesn't have validation/coercion; I was mistaken.

Doesn't actually fill in the "details" property for styled properties because this API is going to change shortly with the new devtools.
@mstr2

This comment has been minimized.

Copy link
Contributor

mstr2 commented Nov 21, 2019

@mstr2 regarding coercion/validation, any thoughts on the current API as compared to the WPF/UWP API where validation is a "static" callback and only coercion receives a reference to the control?
Not sure whether to change our API to match WPF/UWP or not.

From a very high-level perspective, it is my understanding that validation and coercion in WPF address two slightly different things.

Class-level validation is used to enforce that all valid objects be a class (as in set theory). More precisely, all valid objects share a common property, and an object that does not have that property can never be a valid object.

Instance-level coercion, on the other hand, is a constrained range of valid objects. This range can depend on many things and might change at any time.

While it might be elegant in theory, actual production code will often not make that distinction in practice. I understand that Avalonia is currently using a simpler approach, where the distinction between these two aspects is done by throwing an exception (object does not have a required property) or returning a coerced version of the object (object is valid, but out of range).

All other things being equal, I will always prefer the WPF behavior for the simple reason that matching this behavior will mean that, when faced with a coding problem, someone looking up solutions on the internet will find it more likely that WPF solutions will work in Avalonia.

grokys added 4 commits Jan 17, 2020
There's already an overload without the parameter.
To avoid a call to `VerifyAccess` (which shows up when profiling).
Copy link
Member

MarchingCube left a comment

Did a second review pass.

@MarchingCube

This comment has been minimized.

Copy link
Member

MarchingCube commented Jan 19, 2020

Additionally since you still remember what new implementation does - a few class level comments on some of the internals wouldn't hurt (like IValue, property store types, etc.) since right now only you and I know how that stuff even works.

grokys added 10 commits Jan 20, 2020
Compiler warns that `_value` is potentially null, but it can be a value or refernce type.
For typed bindings, we'll have an `AvaloniaPropertyChangedEventArgs<T>` where `T` is the same as the type of the `AvaloniaPropertyObservable`. For non-typed bindings we'll have `object` as `T` so use the non-typed `AvaloniaProperty`.
@grokys

This comment has been minimized.

Copy link
Member Author

grokys commented Jan 20, 2020

Thanks for the thorough review @MarchingCube! I think I've addressed all your feedback. In addition, I realized that the property store can be typed on IValue since we added LocalValueEntry so I simplified things a bit there too.

@grokys grokys requested a review from MarchingCube Jan 20, 2020
Copy link
Member

MarchingCube left a comment

LGTM!

grokys added 7 commits Jan 22, 2020
…cion

Switch to WPF-styled property validation/coercion
This reverts commit d0e2a84.
@grokys grokys merged commit 6c40771 into master Jan 23, 2020
4 checks passed
4 checks passed
Avalonia (Pull Requests) Avalonia (Pull Requests) succeeded
Details
AvaloniaUI.Avalonia #20200123.4 succeeded
Details
DEP All dependencies are resolved
WIP Ready for review
Details
@grokys grokys deleted the refactor/value-store branch Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.