Batching for AvaloniaObject property values. #5070
Conversation
We shouldn't subscribe to bindings until needed.
As that's where #5027 was showing up most.
When subscribing to a `PropertySetterBindingInstance`, if the owner style is not active then there's no need to subscribe to the inner observable as this can cause resource lookups etc. Part of fixing #5027.
Part of fixing #5027.
Cuts down the amount of notifications raised when controls' stying is attached/detached. Part of fixing #5027.
| @@ -22,6 +23,8 @@ internal class BindingEntry<T> : IBindingEntry, IPriorityValueEntry<T>, IObserve | |||
| private readonly IAvaloniaObject _owner; | |||
| private IValueSink _sink; | |||
| private IDisposable? _subscription; | |||
| private bool _isSubscribed; | |||
There was a problem hiding this comment.
Probably worth packing these into an flags enum to save 4 bytes per value stored.
There was a problem hiding this comment.
If only life would be that simple 😄 Depending on the type of the entry alignment of this class will change, for example Sharplab reducing size will do nothing since there is no free space and it will get aligned.
| { | ||
| private readonly IAvaloniaObject _owner; | ||
| private readonly IValueSink _sink; | ||
| private readonly List<IPriorityValueEntry<T>> _entries = new List<IPriorityValueEntry<T>>(); | ||
| private readonly Func<IAvaloniaObject, T, T>? _coerceValue; | ||
| private Optional<T> _localValue; | ||
| private Optional<T> _value; | ||
| private bool _isCalculatingValue; | ||
| private bool _batchUpdate; |
There was a problem hiding this comment.
Probably worth packing these into an flags enum to save 4 bytes per value stored.
LocalValue bindings are special case due to their interaction with the local value set by a non-binding.
There was a problem hiding this comment.
I've been running this branch in my app for a few days already. I've definitely noticed that resource lookup count is more manageable and random templates are not being instantiated.
Left two perf comments but mostly for keeping track of it, I can fix it later (such setters are quite rare anyway).
| data.result = new PropertySetterLazyInstance<T>( | ||
| data.target, | ||
| property, | ||
| () => (T)template.Build()); |
There was a problem hiding this comment.
perf: eliminate closure and delegate allocation.
| data.result = new PropertySetterLazyInstance<T>( | ||
| data.target, | ||
| property, | ||
| () => (T)template.Build()); |
There was a problem hiding this comment.
perf: eliminate closure and delegate allocation.
| AssetLoader.RegisterResUriParsers(); | ||
| return new Styles | ||
| { | ||
| new StyleInclude(new Uri("avares://Avalonia.Benchmarks")) |
There was a problem hiding this comment.
I guess this can be replaced now by new FluentTheme()?
grokys
changed the title
What does the pull request do?
Cuts down the amount of notifications raised when controls' styling is attached/detached:
AvaloniaObjectby callingBeginBatchUpdate/EndBatchUpdate. During batch update, no property change notifications are sent and bindings are not subscribedPropertySetterBindingInstance's inner observable if the containing style instance is not active: this reduces unnecessary resource lookups<Template>s in settersPart of fixing #5027.
Not likely to be ready for 0.10.0 - will need a fair bit of testing.
Benchmarks
before
after
Checklist