Minor optimization for `MergedStyle.SetStyle` #21540

MartyIX · 2024-03-30T21:15:47Z

Description of Change

This PR just removes a few allocations. There is no big idea.

Benchmark

cd src/Core/tests/Benchmarks
dotnet build -c Release --framework net8.0 && dotnet run --framework net8.0 -c Release -- --runtimes net8.0 --filter *StyleBenchmarker*

MAIN

Method	Mean	Error	StdDev	Median	Gen0	Gen1	Gen2	Allocated
MergedStyle	66.95 us	2.668 us	7.867 us	63.44 us	4.1504	4.0283	1.3428	25.79 KB

PR

Method	Mean	Error	StdDev	Median	Gen0	Gen1	Gen2	Allocated
MergedStyle	65.98 us	2.538 us	7.443 us	63.34 us	4.1504	4.0283	1.3428	25.79 KB

Old results

MAIN

| Method      | Mean     | Error    | StdDev   | Median   | Gen0   | Gen1   | Gen2   | Allocated |
|------------ |---------:|---------:|---------:|---------:|-------:|-------:|-------:|----------:|
| MergedStyle | 66.18 us | 2.060 us | 5.977 us | 63.38 us | 4.5166 | 4.3945 | 0.7324 |  28.25 KB |


PR

| Method      | Mean     | Error    | StdDev   | Median   | Gen0   | Gen1   | Gen2   | Allocated |
|------------ |---------:|---------:|---------:|---------:|-------:|-------:|-------:|----------:|
| MergedStyle | 64.74 us | 1.820 us | 5.367 us | 62.34 us | 4.5166 | 4.3945 | 0.7324 |  28.25 KB |

The improvement is not really big but I find it better than nothing.

Additional findings

It appears that MergedStyle.ReRegisterImplicitStyles is not in use (at least not in this repo).

CA1859 is reported on line

maui/src/Controls/src/Core/MergedStyle.cs

Line 15 in d7dac41

    
           static readonly IList<Type> s_stopAtTypes = new List<Type> { typeof(View), typeof(Compatibility.Layout<>), typeof(VisualElement), typeof(NavigableElement), typeof(Element) };

and it looks like it can be trivially changed and nothing wrong would happen.

Issues Fixed

Fixes #21539

MartyIX · 2024-03-30T21:18:21Z

src/Core/tests/Benchmarks/Benchmarks/StyleBenchmarker.cs

+public class StyleBenchmarker
+{
+	[Benchmark]
+	public void MergedStyle()


This is a copy of StyleTests.StyleClassAreCorrecltyMerged.

MartyIX · 2024-03-30T21:18:48Z

src/Controls/tests/Core.UnitTests/LabelTests.cs

@@ -119,7 +119,7 @@ public void FontSizeCanBeSetFromStyle()

 			Assert.Equal(label.GetDefaultFontSize(), label.FontSize);

-			label.SetValue(Label.FontSizeProperty, 1.0, new SetterSpecificity(SetterSpecificity.StyleImplicit, 0, 0, 0));
+			label.SetValue(Label.FontSizeProperty, 1.0, SetterSpecificity.ImplicitSetter);


Just use the 'constants' in tests too.

rmarinho · 2024-04-01T10:29:40Z

/azp run

azure-pipelines · 2024-04-01T10:29:58Z

Azure Pipelines successfully started running 3 pipeline(s).

MartyIX · 2024-04-12T11:24:47Z

@StephaneDelcroix Does this PR make sense to you please?

MartyIX · 2024-04-19T09:06:01Z

@jonathanpeppers Does the proposed changes make sense to you please?

jonathanpeppers · 2024-04-19T13:36:48Z

src/Controls/src/Core/MergedStyle.cs

-			ImplicitStyle?.Apply(bindable, new SetterSpecificity(SetterSpecificity.StyleImplicit, 0, 0, 0));
+			ImplicitStyle?.Apply(bindable, SetterSpecificity.ImplicitSetter);


So what is interesting, in your numbers on the PR description, before & after both say Allocated: 28.25 KB.

SetterSpecificity is a struct, and so maybe that Allocated column is heap allocations only?

The fact the time improved, does indicate your change helps, though.

SetterSpecificity is a struct, and so maybe that Allocated column is heap allocations only?

https://benchmarkdotnet.org/articles/configs/diagnosers.html#restrictions mentions:

In order to get the number of allocated bytes in cross platform way we are using GC.GetAllocatedBytesForCurrentThread which recently got exposed for netcoreapp1.1.

and GC.GetAllocatedBytesForCurrentThread is described as follows:

The GetAllocatedBytesForCurrentThread method returns the total number of bytes allocated on the managed heap [emphasis mine] during the lifetime of a thread, not the total number of bytes that have survived garbage collection. The returned value also does not include any native allocations.

jonathanpeppers · 2024-04-19T13:38:16Z

src/Controls/src/Core/SetterSpecificity.cs

+		public static readonly SetterSpecificity ImplicitSetter = new(StyleImplicit, 0, 0, 0);
+		public static readonly SetterSpecificity LocalSetter = new(StyleLocal, 0, 0, 0);
+		public static readonly SetterSpecificity LocalClassSetter = new(StyleLocal, 0, 1, 0);


I think the main question here is if @StephaneDelcroix likes consolidating these into fields, as he is more familiar with this code. It does seem like it cleans up the code to me.

Won't the reference to this field make a copy anyway? There should be no perf difference between new vs copy... Or maybe there is. No need to execute instructions, just copy memory...

There might be some runtime optimization, where it is faster to copy from a field (I guess?). I'm just judging from the benchmark results. This only helps performance a small amount, so the question is if it cleans up the code. If it does, seems worth it to me.

maui/src/Controls/src/Core/SetterSpecificity.cs

Line 7 in e33d176

internal readonly struct SetterSpecificity : IComparable<SetterSpecificity>, IEquatable<SetterSpecificity>

- it is a readonly struct. I naively expected that it is not copied at all because it's a struct that does not change so it can be on heap and just accessed and not copied at all.

jonathanpeppers · 2024-04-19T13:40:19Z

src/Core/tests/Benchmarks/Benchmarks/StyleBenchmarker.cs

+		var buttonStyle = new Style(typeof(Button))
+		{
+			Setters = {
+				new Setter { Property = Button.TextColorProperty, Value = Colors.Pink },
+			},
+			Class = "pink",
+			ApplyToDerivedTypes = true,
+		};
+		var labelStyle = new Style(typeof(Label))
+		{
+			Setters = {
+				new Setter { Property = Button.BackgroundColorProperty, Value = Colors.Pink },
+			},
+			Class = "pink",
+			ApplyToDerivedTypes = false,
+		};


What if the styles were created as member variables in the Benchmark class? It might narrow the work happening in the MergedStyle() method and get us a more accurate benchmark? It will run that method many times.

If you could get the benchmark down to just merging the styles and creating the fewest objects, that would be good.

I modified the benchmark to be as follows: 5dc6577 and updated OP #21540 (comment) with new numbers.

MartyIX requested a review from a team as a code owner March 30, 2024 21:15

MartyIX requested review from mattleibow and PureWeen March 30, 2024 21:15

dotnet-policy-service bot added the community ✨ Community Contribution label Mar 30, 2024

MartyIX commented Mar 30, 2024

View reviewed changes

jsuarezruiz added the area-xaml XAML, CSS, Triggers, Behaviors label Apr 1, 2024

jsuarezruiz requested a review from StephaneDelcroix April 1, 2024 06:02

jonathanpeppers reviewed Apr 19, 2024

View reviewed changes

MartyIX force-pushed the feature/2024-03-30-merged-style-optimization branch from 567a201 to 9683b14 Compare May 17, 2024 06:57

MartyIX added 3 commits June 27, 2024 19:10

Add StyleBenchmarker

16ed2e3

SetterSpecificity: Allocate less

f7e93e9

Use 'constants' in tests

a950b75

MartyIX force-pushed the feature/2024-03-30-merged-style-optimization branch from 9683b14 to a950b75 Compare June 27, 2024 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor optimization for `MergedStyle.SetStyle` #21540

Minor optimization for `MergedStyle.SetStyle` #21540

MartyIX commented Mar 30, 2024 •

edited

Loading

MartyIX Mar 30, 2024 •

edited

Loading

MartyIX Mar 30, 2024

rmarinho commented Apr 1, 2024

azure-pipelines bot commented Apr 1, 2024

MartyIX commented Apr 12, 2024

MartyIX commented Apr 19, 2024

jonathanpeppers Apr 19, 2024

MartyIX May 20, 2024

jonathanpeppers Apr 19, 2024

mattleibow Apr 28, 2024

jonathanpeppers Apr 29, 2024

MartyIX Apr 29, 2024

jonathanpeppers Apr 19, 2024

MartyIX May 17, 2024

		ImplicitStyle?.Apply(bindable, new SetterSpecificity(SetterSpecificity.StyleImplicit, 0, 0, 0));
		ImplicitStyle?.Apply(bindable, SetterSpecificity.ImplicitSetter);

Minor optimization for MergedStyle.SetStyle #21540

Are you sure you want to change the base?

Minor optimization for MergedStyle.SetStyle #21540

Conversation

MartyIX commented Mar 30, 2024 • edited Loading

Description of Change

Benchmark

Additional findings

Issues Fixed

MartyIX Mar 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmarinho commented Apr 1, 2024

azure-pipelines bot commented Apr 1, 2024

MartyIX commented Apr 12, 2024

MartyIX commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Minor optimization for `MergedStyle.SetStyle` #21540

Minor optimization for `MergedStyle.SetStyle` #21540

MartyIX commented Mar 30, 2024 •

edited

Loading

MartyIX Mar 30, 2024 •

edited

Loading