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

Store LocalValues directly without creating a PriorityValue. #1703

Merged
merged 5 commits into from Jul 3, 2018

Conversation

Projects
None yet
3 participants
@grokys
Member

grokys commented Jun 26, 2018

There are lots of cases of styled properties that have only a local value. Currently we are creating a PriorityValue for each set property on an AvaloniaObject, even if it is to store just a single local value.

This PR changes that: when a property has just a single local value it will be stored directly without creating a PriorityValue. When the property is bound to, or when a non-LocalValue is assigned to it, then a PriorityValue will be created as before.

This PR refactors the value dictionary into a ValueStore class to make this manageable. The overhead of this is non-zero, but in the figures below only represents about 0.5mb. I think this is an acceptable overhead for the sake of maintenance (yes, I did try a version where the code to manage this was placed directly in AvaloniaObject).

Memory Usage

Memory usage was measured via the VS2017 diagnostic tools by running ControlCatalog in Release mode, opening all pages and then taking a memory snapshot:

Note that this PR depends on #1695:

On #1695:

Objects Heap Size (KB)
1,086,578 63,557

This PR:

Objects Heap Size (KB)
966,731 56,749

Depends on #1695

grokys added some commits Jun 26, 2018

Store LocalValues in value store as plain values.
Instead of using a `PriorityValue`, when a property is assigned a simple `LocalValue` just store the value directly in the value store.

@grokys grokys requested a review from AvaloniaUI/core Jun 26, 2018

@wieslawsoltes

This comment has been minimized.

Contributor

wieslawsoltes commented Jun 27, 2018

@grokys I get this error:

Could not convert object '!HideChrome' (of type System.String) to {clr-namespace:Avalonia;assembly=Avalonia.Base}AvaloniaProperty: Could not find AvaloniaProperty 'MetroWindow.!HideChrome'.
   at Portable.Xaml.XamlObjectWriterInternal.GetCorrectlyTypedValue(XamlMember xm, XamlType xt, Object value) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlObjectWriter.cs:line 694
   at Portable.Xaml.XamlObjectWriterInternal.PopulateObject(Boolean considerPositionalParameters, IList`1 contents) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlObjectWriter.cs:line 562
   at Portable.Xaml.XamlObjectWriterInternal.OnWriteEndMember() in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlObjectWriter.cs:line 519
   at Portable.Xaml.XamlWriterInternalBase.WriteEndMember() in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlWriterInternalBase.cs:line 270
   at Portable.Xaml.XamlServices.Transform(XamlReader xamlReader, XamlWriter xamlWriter, Boolean closeWriter) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlServices.cs:line 128
   at Avalonia.Markup.Xaml.AvaloniaXamlLoader.LoadFromReader(XamlReader reader, AvaloniaXamlContext context, IAmbientProvider parentAmbientProvider) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\AvaloniaXamlLoader.cs:line 208
   at Avalonia.Markup.Xaml.Templates.TemplateContent.Load() in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\Templates\TemplateContent.cs:line 35
   at Avalonia.Controls.Primitives.TemplatedControl.ApplyTemplate() in C:\projects\Avalonia\src\Avalonia.Controls\Primitives\TemplatedControl.cs:line 259
   at Avalonia.Layout.Layoutable.MeasureCore(Size availableSize) in C:\projects\Avalonia\src\Avalonia.Layout\Layoutable.cs:line 474
   at Avalonia.Layout.Layoutable.Measure(Size availableSize) in C:\projects\Avalonia\src\Avalonia.Layout\Layoutable.cs:line 318
   at Avalonia.Layout.LayoutManager.Measure(ILayoutable control) in C:\projects\Avalonia\src\Avalonia.Layout\LayoutManager.cs:line 178
   at Avalonia.Layout.LayoutManager.ExecuteInitialLayoutPass(ILayoutRoot root) in C:\projects\Avalonia\src\Avalonia.Layout\LayoutManager.cs:line 118
   at Avalonia.Controls.Window.Show() in C:\projects\Avalonia\src\Avalonia.Controls\Window.cs:line 366
   at Core2D.Avalonia.App.Start(IServiceProvider serviceProvider, AboutInfo aboutInfo) in C:\DOWNLOADS\GitHub\Core2D\src\Core2D.Avalonia\App.xaml.cs:line 146
Could not find AvaloniaProperty 'MetroWindow.!HideChrome'.
   at Avalonia.Markup.Xaml.Converters.AvaloniaPropertyTypeConverter.ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, Object value) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\Converters\AvaloniaPropertyTypeConverter.cs:line 44
   at Portable.Xaml.XamlObjectWriterInternal.GetCorrectlyTypedValue(XamlMember xm, XamlType xt, Object value) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlObjectWriter.cs:line 687

Code:
https://github.com/wieslawsoltes/Dock/blob/a1ca42c5b4519b33f343421f657aa787c41b9cd0/src/Dock.Avalonia/Controls/MetroWindow.xaml.cs#L40

https://github.com/wieslawsoltes/Dock/blob/a1ca42c5b4519b33f343421f657aa787c41b9cd0/src/Dock.Avalonia/Controls/MetroWindow.xaml#L18

@wieslawsoltes

This comment has been minimized.

Contributor

wieslawsoltes commented Jun 27, 2018

It also throws when property is really missing, which I think is good 👍

Could not convert object 'TitleBarContent' (of type System.String) to {clr-namespace:Avalonia;assembly=Avalonia.Base}AvaloniaProperty: Could not find AvaloniaProperty 'MetroWindow.TitleBarContent'.
   at Portable.Xaml.XamlObjectWriterInternal.GetCorrectlyTypedValue(XamlMember xm, XamlType xt, Object value) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlObjectWriter.cs:line 694
   at Portable.Xaml.XamlObjectWriterInternal.PopulateObject(Boolean considerPositionalParameters, IList`1 contents) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlObjectWriter.cs:line 562
   at Portable.Xaml.XamlObjectWriterInternal.OnWriteEndMember() in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlObjectWriter.cs:line 519
   at Portable.Xaml.XamlWriterInternalBase.WriteEndMember() in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlWriterInternalBase.cs:line 270
   at Portable.Xaml.XamlServices.Transform(XamlReader xamlReader, XamlWriter xamlWriter, Boolean closeWriter) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlServices.cs:line 128
   at Avalonia.Markup.Xaml.AvaloniaXamlLoader.LoadFromReader(XamlReader reader, AvaloniaXamlContext context, IAmbientProvider parentAmbientProvider) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\AvaloniaXamlLoader.cs:line 208
   at Avalonia.Markup.Xaml.Templates.TemplateContent.Load() in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\Templates\TemplateContent.cs:line 35
   at Avalonia.Controls.Primitives.TemplatedControl.ApplyTemplate() in C:\projects\Avalonia\src\Avalonia.Controls\Primitives\TemplatedControl.cs:line 259
   at Avalonia.Layout.Layoutable.MeasureCore(Size availableSize) in C:\projects\Avalonia\src\Avalonia.Layout\Layoutable.cs:line 474
   at Avalonia.Layout.Layoutable.Measure(Size availableSize) in C:\projects\Avalonia\src\Avalonia.Layout\Layoutable.cs:line 318
   at Avalonia.Layout.LayoutManager.Measure(ILayoutable control) in C:\projects\Avalonia\src\Avalonia.Layout\LayoutManager.cs:line 178
   at Avalonia.Layout.LayoutManager.ExecuteInitialLayoutPass(ILayoutRoot root) in C:\projects\Avalonia\src\Avalonia.Layout\LayoutManager.cs:line 118
   at Avalonia.Controls.Window.Show() in C:\projects\Avalonia\src\Avalonia.Controls\Window.cs:line 366
   at Core2D.Avalonia.App.Start(IServiceProvider serviceProvider, AboutInfo aboutInfo) in C:\DOWNLOADS\GitHub\Core2D\src\Core2D.Avalonia\App.xaml.cs:line 146
Could not find AvaloniaProperty 'MetroWindow.TitleBarContent'.
   at Avalonia.Markup.Xaml.Converters.AvaloniaPropertyTypeConverter.ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, Object value) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\Converters\AvaloniaPropertyTypeConverter.cs:line 44
   at Portable.Xaml.XamlObjectWriterInternal.GetCorrectlyTypedValue(XamlMember xm, XamlType xt, Object value) in C:\projects\Avalonia\src\Markup\Avalonia.Markup.Xaml\PortableXaml\portable.xaml.github\src\Portable.Xaml\Portable.Xaml\XamlObjectWriter.cs:line 687

The TitleBarContent is really missing.

@wieslawsoltes

This comment has been minimized.

Contributor

wieslawsoltes commented Jun 27, 2018

@grokys Other than the issue I have reported with property PR looks good.

@wieslawsoltes

Tested with Core2D and all works now (fixes Core2D issues by refactoring property definitions and adding missing properties).

@jkoritzinsky jkoritzinsky added the perf label Jul 2, 2018

@jkoritzinsky

This comment has been minimized.

Member

jkoritzinsky commented Jul 3, 2018

@wieslawsoltes The error you're getting on that TemplateBinding is a change by design in #1695. You should change that binding to be a regular binding with a RelativeSourceMode of TemplatedParent and a priority of TemplatedParent.

@jkoritzinsky jkoritzinsky merged commit 5d8b084 into master Jul 3, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jkoritzinsky jkoritzinsky deleted the valuestore branch Jul 3, 2018

@grokys grokys referenced this pull request Jul 20, 2018

Merged

Fix devtools #1769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment