Use a CacheKey struct for the ObjectCreator cache #2317
Conversation
The implementation of Dictionary will use hash codes under the hood, but handle collisions correctly. By using a hashcode for the dictionary key we're preventing the .NET class from doing the right thing. This may degrade performance because cache hits need to do the full Equals call to verify that it is not a hash collision. This may degrade performance because of changes in inlining behavior.
|
|
||
| public override int GetHashCode() | ||
| { | ||
| var hashCode = new HashCode(); |
There was a problem hiding this comment.
Just swinging by from a performance point, can you make the properties private fields? (The Equals would still see them) Then you could store this value after first calculation?
There was a problem hiding this comment.
I've moved Type and Args to be private, readonly fields so that the compiler can do as much optimization with them as it can.
However, I don't think we'd actually get any performance boost from storing the hash code. The implementation of Dictionary already stores the hashcode on the entry struct, so each item added to the dictionary only has GetHashCode called once.
src/CsvHelper/ObjectCreator.cs
Outdated
| public bool Equals(CacheKey other) | ||
| { | ||
| if (other.Args.Length != Args.Length) return false; | ||
| if (other.Type != Type) return false; |
There was a problem hiding this comment.
I would suspect the type would vary more often then the number of arguments. Think of all the false positives you will get with default constructors.
- use private fields instead of private fields instead of public properties - check type first instead of argument type (default constructors will all have 0 arguments, so it's not a good first check)
This is a candidate fix for #2315
The implementation of Dictionary will use hash codes under the hood, but handle collisions correctly.
By using a hashcode for the dictionary key the existing code prevents the .NET class from doing the right thing.
Notes:
This may degrade performance because cache hits need to do the full Equals call to verify that it is not a hash collision.
This may degrade performance because of changes in inlining behavior.