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

Avoid using AddClassHandler in BCL whenever possible #7009

Closed
workgroupengineering opened this issue Nov 25, 2021 · 15 comments
Closed

Avoid using AddClassHandler in BCL whenever possible #7009

workgroupengineering opened this issue Nov 25, 2021 · 15 comments

Comments

@workgroupengineering
Copy link
Contributor

Is your feature request related to a problem? Please describe.
AddClassHandler is expensive

Describe the solution you'd like
Override OnPropertyChanged when it is possible

Describe alternatives you've considered

Additional context

@Takoooooo
Copy link
Contributor

@workgroupengineering I am curious, do you have benchmarks?

@grokys
Copy link
Member

grokys commented Nov 25, 2021

Yeah I know that AddClassHandler is slow compared to just listening to property changes in on OnPropertyChanged, but benchmarks would be good just to see how slow they are ;)

@workgroupengineering
Copy link
Contributor Author

I don't have an avalonia benchmark, but I used a solution similar to AddClassHandler in a microorm and besides the slowness I had excessive memory usage.

@maxkatz6
Copy link
Member

maxkatz6 commented Nov 25, 2021

@workgroupengineering do you need a help with running Avalonia Benchmarks? It would be great to see improvements before and after your PRs. Though they are good to merge anyway as for me.

This project and I think # 8 benchmark is the most important here
https://github.com/AvaloniaUI/Avalonia/tree/master/tests/Avalonia.Benchmarks

@workgroupengineering
Copy link
Contributor Author

do you need a help with running Avalonia Benchmarks? It would be great to see improvements before and after your PRs.

Yes, I have no idea how to do the Benchmarks before and after the PRs.

@maxkatz6
Copy link
Member

maxkatz6 commented Nov 25, 2021

@workgroupengineering it's quite simple. You can either just run Avalonia.Benchmarks.csproj and select benchmark # 8 or it's better to build it in release mode, closed everything that can interfere (including VS/Rider) and run built project from command line. Everything else will be handled by the Benchmark.NET and it will create folder with results, as well input in the console. Resulting table will be the most interesting part, just like in this PR #6836 (comment)

You will need to run these benchmarks twice - for current master and after your PRs (you can combine them locally just to run benchmarks). If you want, you can run them per PR, but it will be time consuming.

@workgroupengineering
Copy link
Contributor Author

These are the Benchmarks

Master: e0bae82

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19044
Intel Core i7-7700K CPU 4.20GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=6.0.100
  [Host]     : .NET Core 6.0.0 (CoreCLR 6.0.21.52210, CoreFX 6.0.21.52210), X64 RyuJIT
  DefaultJob : .NET Core 6.0.0 (CoreCLR 6.0.21.52210, CoreFX 6.0.21.52210), X64 RyuJIT

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
CreateCalendar 17,901.1 μs 357.93 μs 843.68 μs 718.7500 281.2500 62.5000 4393.75 KB
CreateButton 121.5 μs 2.31 μs 2.05 μs 10.2539 0.9766 - 42.5 KB
CreateTextBox 1,001.9 μs 19.26 μs 31.09 μs 54.6875 29.2969 3.9063 214.65 KB

Conbined PRs #7012, #7013, #7014, #7017

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19044
Intel Core i7-7700K CPU 4.20GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=6.0.100
  [Host]     : .NET Core 6.0.0 (CoreCLR 6.0.21.52210, CoreFX 6.0.21.52210), X64 RyuJIT
  DefaultJob : .NET Core 6.0.0 (CoreCLR 6.0.21.52210, CoreFX 6.0.21.52210), X64 RyuJIT

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
CreateCalendar 17,755.4 μs 353.76 μs 529.49 μs 718.7500 281.2500 62.5000 4393.65 KB
CreateButton 123.8 μs 1.77 μs 1.48 μs 10.2539 0.9766 - 42.5 KB
CreateTextBox 949.2 μs 13.49 μs 12.62 μs 52.7344 27.3438 3.9063 214.64 KB

The Benchmark result is partial because I have not changed the Visual derived controls.

@grokys
Copy link
Member

grokys commented Nov 26, 2021

Hmm, so no improvement in those cases. I think we need a benchmark which just measures the overhead of that specific feature. I'm writing some now.

@workgroupengineering
Copy link
Contributor Author

Hmm, so no improvement in those cases. I think we need a benchmark which just measures the overhead of that specific feature.

I see Benchmarks 8 it mesure only creation.

the impact of changes should be when a control is changed multiple times. Can you suggest a more relevant Benchmarks or give me some advice on how to make one?

@grokys
Copy link
Member

grokys commented Nov 26, 2021

Ok created a benchmark: https://gist.github.com/grokys/fe7a87dc95d48fe09b4aac40e7201b7b

Results show that's there's actually very little difference between the two:

|            Method |     Mean |    Error |   StdDev | Ratio |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------ |---------:|---------:|---------:|------:|-------:|------:|------:|----------:|
|   AddClassHandler | 13.50 us | 0.202 us | 0.189 us |  1.00 | 1.5717 |     - |     - |   6.47 KB |
| OnPropertyChanged | 11.32 us | 0.069 us | 0.054 us |  0.83 | 1.5717 |     - |     - |   6.47 KB |

I'm surprised by this. Based on that I'm not sure the change is worth it. Though it could be that the benchmark is flawed.

There's a 17% improvement, but that's in an isolated microbenchmark and I suspect that improvement would be a tiny percentage of overall time in a more holistic benchmark.

@grokys
Copy link
Member

grokys commented Nov 26, 2021

@workgroupengineering I'd be interested to see more benchmarks on this if you want to create them. As for advice on creating benchmarks I'm no expert and not sure anything I could write here would be better than information you can find online and by looking at other benchmarks in our repository.

Or was there something in particular you need to know?

@workgroupengineering
Copy link
Contributor Author

It would take a benchmark to measure performance when a hierarchy of many controls is invalidated.

Tell me, is it worth investing some time in this. Sure it will be less than 17%, but from my little experience, micro optimizations make themselves felt in the end.

@Takoooooo
Copy link
Contributor

IMO it doesn't worth to invest time in that, we have much bigger perf problems than that, 2us saved time will not cost the time you spend on fixing that

@grokys
Copy link
Member

grokys commented Nov 26, 2021

Note: 2us for 100 iterations.

@workgroupengineering
Copy link
Contributor Author

ok

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

No branches or pull requests

5 participants