-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Value store v3.0 #8600
Value store v3.0 #8600
Conversation
Most (but not all) tests passing, all features mostly implemented exception coercion.
To remove `InheritanceFrame` - it was unneeded, we can just point to the nearest ancestor value store with inherited values.
With associated unit tests.
- Always evaluate the active state from current information, don't rely on subscriptions to fire as the current state may not be up-to-date - Don't notify the `IStyleActivatorSink` of a change immediately on subscription
And use the concrete `ValueFrame` (renamed from `ValueFrameBase`) as this was the only implementation of the interface.
If a `StyleBase` has no activator, and its `Setter`s don't need to be separately instanced on the control, then we can share a `StyleInstance` between controls. This causes failing tests because `TopLevel` applies styles in its constructor, but doesn't set `_styled` to true, meaning that styles get applied twice. This issue will need to be addressed separately.
Due to #8549.
And rename collection to indicate this.
You can test this PR using the following package version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also after main changes are done - it would be worth it and go over added classes and seal everything to help devirtualization.
target.StyleApplied(instance); | ||
instance.Start(); | ||
if (target is not AvaloniaObject ao) | ||
throw new InvalidOperationException("Styles can only be applied to AvaloniaObjects."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to GH formatting this almost looks like as it happens all the time. Maybe adding braces here wouln't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here?
Added `IValueEntry<T>` interface back in and use that if present to get the value from the entry.
You can test this PR using the following package version. |
Decreases the number of metadata lookups needed. Previously we were doing two lookups on construction now we're only doing one.
You can test this PR using the following package version. |
You can test this PR using the following package version. |
What does the pull request do?
This PR implements a new value store for
AvaloniaObject
. Previously, each property value was stored in a dictionary insideValueStore
which mappedproperty
->value
, wherevalue
changed type depending on what was stored (single local value, single binding, multiple values of differing priorities).This PR changes
ValueStore
to store property values in a different manner;EffectiveValue<T>
containing the effective value and base value + prioritiesEffectiveValue<T>
for the propertyImmediateValueFrame
: values set bySetValue
orAddBinding
with non-LocalValue priority.StyleInstance
: a frame containing theSetter
s for a styleIValueEntry
sSome improvements the new value store enables:
ValueStore
s meaning thatStyle
s which don't have activators (andControlTheme
s) can be instanced just once instead of needing to be instanced on eachAvaloniaObject
separately.Setter
is anIValueEntry
meaning that it can be stored directly a frame unless it needs to be explicitly instanced on each controlControlTheme
can be changed without affecting the rest of the styles applied to it (this isn't actually implemented yet - will require a change to ourBindingPriority
s in order to distinguish between control themes and styles)LocalValue
styled property bindings, removing the need for 2 lists perAvaloniaObject
Benchmarks
Of these,
InitializeButton
is only 26ns slower which is on the limit of what can be measured.The nearly 19x improvement in reading inherited values should really make a difference in some applications.
Memory benchmarks have now been added. Most of increases in memory are because of the particular scenarios the benchmarks are testing and should be offset by the decreases in a real app.
Breaking Changes
Removes
BeginBatchUpdate
/EndBatchUpdate
and replaces it with internalBeginStyling
/EndStyling
as the current batch update system introduced in 0.10.1 isn't zero cost, whereas the more limitedBeginStyling
/EndStyling
is.I want to re-think and maybe re-implement batch updates separately from this PR, at least for initialization, but if anyone is using them please let me know how/where.
Depends on #8571