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

improve performance of RegistrationKey #41

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bollhals
Copy link

@bollhals bollhals commented Apr 20, 2021

PR 1 extracted out of #40

Focused on the RegistrationKey.
2 main changes:

  • Implement IEquateable to avoid boxing in equality / hashcode methods
  • Cache the typegroup to improve the hashcode performance

Performance measurements:

From ResolveFromType, same performance, but less allocation

Method Mean Error StdDev Min Max Median Gen 0 Gen 1 Gen 2 Allocated
Master 220.3 ns 0.63 ns 0.56 ns 219.5 ns 221.3 ns 220.4 ns 0.0730 - - 344 B
Current 221.8 ns 2.19 ns 1.94 ns 220.3 ns 226.7 ns 220.9 ns 0.0577 - - 272 B

From ResolveFromGenericType, faster performance, less allocations

Method Mean Error StdDev Min Max Median Gen 0 Gen 1 Gen 2 Allocated
Current 676.1 ns 5.69 ns 5.32 ns 669.4 ns 685.6 ns 674.6 ns 0.0916 - - 432 B
Master 877.2 ns 5.14 ns 4.81 ns 869.9 ns 884.2 ns 877.6 ns 0.1068 - - 504 B
v1.4 829.0 ns 4.72 ns 4.42 ns 822.7 ns 838.0 ns 827.9 ns 0.0639 - - 304 B
v1.BoDi_Concurrent_Dictionary_And_Lazy 1,026.5 ns 4.18 ns 3.70 ns 1,020.5 ns 1,032.4 ns 1,025.6 ns 0.1564 - - 736 B

var isInvertable = other.TypeGroup == Type || other.Type == TypeGroup || other.Type == Type;
return isInvertable && String.Equals(other.Name, Name, StringComparison.CurrentCultureIgnoreCase);
var isInvertable = other.Type == Type || other.typeGroup == Type || other.Type == typeGroup;
return isInvertable && other.Name == Name;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh forgot this is still in. Need to check the impact first

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From ResolveFromType
| Current | 226.3 ns | 1.15 ns | 1.07 ns | 224.2 ns | 227.8 ns | 226.2 ns | 0.0577 | - | - | 272 B |
=> ~2.5% difference

Is the CurrentCultureIgnoreCase required here? What is the intention for it? Wouldn't you always resolve with the exact same key as it was registered?

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

Successfully merging this pull request may close these issues.

1 participant