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.

Sign up for GitHub

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

Use a CacheKey struct for the ObjectCreator cache #2317

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

Use a CacheKey struct for the ObjectCreator cache #2317

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f215ed8
Use a CacheKey struct for the ObjectCreator cache
scottfavre Jan 22, 2025
d194397
small perf improvements for CacheKey:
scottfavre Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 49 additions & 15 deletions src/CsvHelper/ObjectCreator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace CsvHelper;
/// </summary>
public class ObjectCreator
{
private readonly Dictionary<int, Func<object?[], object>> cache = new Dictionary<int, Func<object?[], object>>();
private readonly Dictionary<CacheKey, Func<object?[], object>> cache = new Dictionary<CacheKey, Func<object?[], object>>();

/// <summary>
/// Creates an instance of type T using the given arguments.
Expand Down Expand Up @@ -41,7 +41,7 @@ public object CreateInstance(Type type, params object?[] args)
private Func<object?[], object> GetFunc(Type type, object?[] args)
{
var argTypes = GetArgTypes(args);
var key = GetConstructorCacheKey(type, argTypes);
var key = new CacheKey(type, argTypes);
if (!cache.TryGetValue(key, out var func))
{
cache[key] = func = CreateInstanceFunc(type, argTypes);
Expand All @@ -62,19 +62,6 @@ private static Type[] GetArgTypes(object?[] args)
return argTypes;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int GetConstructorCacheKey(Type type, Type[] args)
{
var hashCode = new HashCode();
hashCode.Add(type.GetHashCode());
for (var i = 0; i < args.Length; i++)
{
hashCode.Add(args[i].GetHashCode());
}

return hashCode.ToHashCode();
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static Func<object?[], object> CreateInstanceFunc(Type type, Type[] argTypes)
{
Expand Down Expand Up @@ -195,4 +182,51 @@ private enum MatchType
Exact = 1,
Fuzzy = 2
}

internal struct CacheKey : IEquatable<CacheKey>
{
private readonly Type _type;
private readonly Type[] _args;

public CacheKey(Type type, Type[] args)
{
_type = type;
_args = args;
}

public override int GetHashCode()
{
var hashCode = new HashCode();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

See https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs#L1754

hashCode.Add(_type.GetHashCode());
for (var i = 0; i < _args.Length; i++)
{
hashCode.Add(_args[i].GetHashCode());
}

return hashCode.ToHashCode();
}

public override bool Equals(object? obj)
{
if (obj == null) return false;

if (obj is CacheKey key)
{
return Equals(key);
}

return false;
}

public bool Equals(CacheKey other)
{
if (other._type != _type) return false;
if (other._args.Length != _args.Length) return false;
for (var i = 0; i < _args.Length; i++)
{
if (other._args[i] != _args[i]) return false;
}
return true;
}
}
}