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

Switch to WPF-styled property validation/coercion #3287

Open
wants to merge 5 commits into
base: refactor/value-store
from

Conversation

@grokys
Copy link
Member

grokys commented Nov 24, 2019

What does the pull request do?

#3255 refactored AvaloniaObject's value store but removed property validation/coercion.

This PR adds that back in, but changes the API/semantics to match WPF's:

  • Validation is now a "static" property of an AvaloniaProperty and can't be overridden.
  • Coercion is now separate, and recieves an IAvaloniaObject and the value. Its semantics have also changed such that the underlying base value is kept in the property system and so if the restrictions on the valid value change a call to AvaloniaObject.CoerceValue can cause the original base value to reappear

Previously Validation/Coercion imposed an overhead for all properties whether they defined validation/coercion callbacks or not. I tried to avoid that in this implementation:

  • Because validation now doesn't require a metadata lookup it can be done as a simple method call
  • The coercion callback requires a metadata lookup, but this is done only once: the callback is then stored in PriorityValue. In addition setting a coercion callback on a property sets a flag on the property to indicate that a metadata lookup needs to take place

Because I wanted to introduce as little as possible overhead for non-coerced properties, if a property has a coercion callback defined then it is promoted straight to a PriorityValue in the ValueStore. This means that coercion does not affect values that are stored as simple LocalValueEntrys, ConstantValueEntrys or BindingEntrys.

Benchmarks

|                                Method |      Mean |     Error |    StdDev |   Gen 0 | Gen 1 | Gen 2 | Allocated |
|-------------------------------------- |----------:|----------:|----------:|--------:|------:|------:|----------:|
|           Set_Int_Property_LocalValue |  15.57 us | 0.2080 us | 0.1946 us |  0.2441 |     - |     - |     248 B |
|  Set_Int_Property_Multiple_Priorities | 211.15 us | 2.0095 us | 1.8797 us | 25.6348 |     - |     - |   24696 B |
|      Set_Int_Property_TemplatedParent |  34.95 us | 0.5178 us | 0.4590 us |  6.7139 |     - |     - |    6504 B |
|          Bind_Int_Property_LocalValue |  10.72 us | 0.0623 us | 0.0583 us |  0.3967 |     - |     - |     392 B |
| Bind_Int_Property_Multiple_Priorities |  45.28 us | 0.6510 us | 0.6090 us |  1.5259 |     - |     - |    1512 B |
| Set_Validated_Int_Property_LocalValue |  16.02 us | 0.0164 us | 0.0153 us |  0.2441 |     - |     - |     248 B |
|   Set_Coerced_Int_Property_LocalValue |  17.93 us | 0.2233 us | 0.2089 us |  0.3357 |     - |     - |     344 B |

Note

This only affects property validation. Data validation remains unchanged.

This PR targets #3255

cc: @mstr2

@grokys grokys mentioned this pull request Jan 17, 2020
1 of 1 task complete
Copy link
Member

MarchingCube left a comment

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.