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

Remove generics from IAvaloniaObject. #7969

Closed
wants to merge 3 commits into from

Conversation

grokys
Copy link
Member

@grokys grokys commented Apr 12, 2022

What does the pull request do?

We have a problem with our usage of generic virtual and generic interface methods, in that they cause performance problems in the JIT. We've decided that for 11.0 we need to at least remove generic interface methods from the public API. This PR removes generic methods from the IAvaloniaObject interface.

In order to do so, I had to throw exceptions in quite a few places if the IAvaloniaObject instance isn't an AvaloniaObject. Reimplementing this interface wasn't really feasible anyway so this shouldn't cause a problem in practise (and in future we will probably want to remove this interface completely but baby steps for now).

Breaking Changes

Yes :P

@workgroupengineering
Copy link
Contributor

For curiosity. How much does performance improve?

@grokys
Copy link
Member Author

grokys commented Apr 12, 2022

I doubt that it's changed much from just this PR - there are a couple of other more impactful changes that I'm about to make, and I'll benchmark once those are done. Wanted to separate the PRs though for easier review.

@grokys
Copy link
Member Author

grokys commented Apr 13, 2022

A quick preview of a few selected benchmarks after this and some other changes:

master

|                             Method |       Mean |     Error |    StdDev | Ratio | RatioSD |   Gen 0 | Allocated |
|----------------------------------- |-----------:|----------:|----------:|------:|--------:|--------:|----------:|
|               SetClrPropertyValues |   5.069 us | 0.0237 us | 0.0222 us |  1.00 |    0.00 |       - |         - |
|                          SetValues | 164.066 us | 0.8528 us | 0.7560 us | 32.36 |    0.18 | 41.9922 |  88,000 B |
| Set_Local_Values_With_Style_Values | 253.437 us | 1.6218 us | 1.3542 us | 49.97 |    0.43 | 80.0781 | 168,001 B |

|                                     Method |     Mean |   Error |  StdDev |    Gen 0 | Allocated |
|------------------------------------------- |---------:|--------:|--------:|---------:|----------:|
|          Setup_Dispose_LocalValue_Bindings | 380.7 us | 1.85 us | 1.64 us | 215.8203 |    441 KB |
|                   Fire_LocalValue_Bindings | 135.6 us | 0.82 us | 0.73 us |  45.4102 |     93 KB |
| Fire_LocalValue_Bindings_With_Style_Values | 253.5 us | 1.86 us | 1.55 us |  86.9141 |    178 KB |

|         Method |        Mean |       Error |      StdDev |      Median |   Gen 0 |   Gen 1 | Allocated |
|--------------- |------------:|------------:|------------:|------------:|--------:|--------:|----------:|
| CreateCalendar | 27,451.2 us | 2,055.70 us | 5,898.20 us | 24,671.8 us |       - |       - |  4,356 KB |
|   CreateButton |    206.3 us |     0.88 us |     0.78 us |    206.3 us | 20.7520 |  0.4883 |     43 KB |
|  CreateTextBox |  1,355.8 us |     7.59 us |     6.73 us |  1,357.6 us | 78.1250 | 23.4375 |    225 KB |

after refactoring

|                             Method |       Mean |     Error |    StdDev | Ratio | RatioSD |   Gen 0 | Allocated |
|----------------------------------- |-----------:|----------:|----------:|------:|--------:|--------:|----------:|
|               SetClrPropertyValues |   5.018 us | 0.0084 us | 0.0070 us |  1.00 |    0.00 |       - |         - |
|                          SetValues | 134.237 us | 0.6255 us | 0.5545 us | 26.75 |    0.12 | 41.9922 |  88,000 B |
| Set_Local_Values_With_Style_Values | 217.688 us | 1.2422 us | 1.1619 us | 43.35 |    0.26 | 80.3223 | 168,000 B |

|                                     Method |     Mean |   Error |  StdDev |    Gen 0 | Allocated |
|------------------------------------------- |---------:|--------:|--------:|---------:|----------:|
|          Setup_Dispose_LocalValue_Bindings | 310.6 us | 2.15 us | 1.79 us | 219.2383 |    448 KB |
|                   Fire_LocalValue_Bindings | 107.3 us | 0.76 us | 0.68 us |  45.6543 |     93 KB |
| Fire_LocalValue_Bindings_With_Style_Values | 199.0 us | 1.00 us | 0.94 us |  87.1582 |    178 KB |

|         Method |        Mean |       Error |      StdDev |      Median |   Gen 0 |   Gen 1 | Allocated |
|--------------- |------------:|------------:|------------:|------------:|--------:|--------:|----------:|
| CreateCalendar | 25,469.2 us | 2,107.63 us | 6,081.00 us | 22,432.5 us |       - |       - |  4,365 KB |
|   CreateButton |    190.1 us |     0.90 us |     0.85 us |    189.8 us | 20.5078 |  0.2441 |     42 KB |
|  CreateTextBox |  1,284.4 us |    10.76 us |    10.06 us |  1,283.2 us | 76.1719 | 21.4844 |    225 KB |

So looks like we're seeing a 10-20% speedup in some benchmarks.

@grokys
Copy link
Member Author

grokys commented Apr 14, 2022

Full benchmarks after the first batch of changes at #7980

@grokys
Copy link
Member Author

grokys commented Apr 14, 2022

Closing this as I think #7980 should be the one we merge (which includes this and #7979).

@grokys grokys closed this Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants