Skip to content

AVRO-3893: [csharp] Avoid per-call closure allocation in ObjectCreator.FindType #3966

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

Open
iemejia wants to merge 1 commit into
base: main
Choose a base branch
from
+57 −43
Open

AVRO-3893: [csharp] Avoid per-call closure allocation in ObjectCreator.FindType #3966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lang/csharp/src/apache/main/GlobalSuppressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1000:Do not declare static members on generic types", Justification = "Maintain public API", Scope = "member", Target = "~M:Avro.File.DataFileWriter`1.OpenAppendWriter(Avro.Generic.DatumWriter{`0},System.String)~Avro.File.IFileWriter{`0}")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1000:Do not declare static members on generic types", Justification = "Maintain public API", Scope = "member", Target = "~M:Avro.File.DataFileWriter`1.OpenAppendWriter(Avro.Generic.DatumWriter{`0},System.IO.Stream,System.IO.Stream)~Avro.File.IFileWriter{`0}")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Purposely catch any exception", Scope = "member", Target = "~M:Avro.File.DataFileReader`1.Sync(System.Int64)")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Purposely catch any exception", Scope = "member", Target = "~M:Avro.Specific.ObjectCreator.FindType(System.String)~System.Type")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Purposely catch any exception", Scope = "member", Target = "~M:Avro.Specific.ObjectCreator.FindTypeUncached(System.String)~System.Type")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "Purposely catch any exception", Scope = "member", Target = "~M:Avro.Specific.ObjectCreator.GetType(Avro.Schema)~System.Type")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1051:Do not declare visible instance fields", Justification = "Maintain public API", Scope = "member", Target = "~F:Avro.CodeGen.namespaceLookup")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1051:Do not declare visible instance fields", Justification = "Maintain public API", Scope = "member", Target = "~F:Avro.Generic.GenericFixed.value")]
Expand Down
98 changes: 56 additions & 42 deletions lang/csharp/src/apache/main/Specific/ObjectCreator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@
private readonly Assembly entryAssembly;
private readonly bool diffAssembly;

// Cached value factory for typeCacheByName.GetOrAdd. Stored once so that
// FindType does not allocate a new closure on every call (AVRO-3893).
private readonly Func<string, Type> findTypeFactory;

/// <summary>
/// Initializes a new instance of the <see cref="ObjectCreator"/> class.
/// </summary>
Expand All @@ -69,6 +73,8 @@

// entryAssembly returns null when running from NUnit
diffAssembly = entryAssembly != null && execAssembly != entryAssembly;

findTypeFactory = FindTypeUncached;
}

/// <summary>
Expand All @@ -81,62 +87,70 @@
/// </exception>
private Type FindType(string name)
{
return typeCacheByName.GetOrAdd(name, (_) =>
{
Type type = null;
return typeCacheByName.GetOrAdd(name, findTypeFactory);
}

if (TryGetIListItemTypeName(name, out var itemTypeName))
{
return GenericIListType.MakeGenericType(FindType(itemTypeName));
}
/// <summary>
/// Value factory used by <see cref="FindType"/>. Kept as a separate method (referenced
/// through the cached <c>findTypeFactory</c> delegate) so the lookup does not capture a
/// per-call closure. The <paramref name="name"/> argument is the cache key supplied by
/// <see cref="ConcurrentDictionary{TKey,TValue}.GetOrAdd(TKey, Func{TKey, TValue})"/>.
/// </summary>
private Type FindTypeUncached(string name)
{
Type type = null;

if (TryGetNullableItemTypeName(name, out itemTypeName))
{
return GenericNullableType.MakeGenericType(FindType(itemTypeName));
}
if (TryGetIListItemTypeName(name, out var itemTypeName))
{
return GenericIListType.MakeGenericType(FindType(itemTypeName));
}

// if entry assembly different from current assembly, try entry assembly first
if (diffAssembly)
{
type = entryAssembly.GetType(name);
}
if (TryGetNullableItemTypeName(name, out itemTypeName))
{
return GenericNullableType.MakeGenericType(FindType(itemTypeName));
}

// try current assembly and mscorlib
if (type == null)
{
type = Type.GetType(name);
}
// if entry assembly different from current assembly, try entry assembly first
if (diffAssembly)
{
type = entryAssembly.GetType(name);
}

// try current assembly and mscorlib
if (type == null)
{
type = Type.GetType(name);
}

// type is still not found, need to loop through all loaded assemblies
if (type == null)
// type is still not found, need to loop through all loaded assemblies
if (type == null)
{
foreach (Assembly assembly in AppDomain.CurrentDomain.GetAssemblies())
{
foreach (Assembly assembly in AppDomain.CurrentDomain.GetAssemblies())
// Loading all types from all assemblies could fail for a variety of
// non -fatal reasons. If we fail to load types from an assembly, continue.
try
{
// Loading all types from all assemblies could fail for a variety of
// non -fatal reasons. If we fail to load types from an assembly, continue.
try
// Change the search to look for Types by both NAME and FULLNAME
foreach (Type t in assembly.GetTypes())
{
// Change the search to look for Types by both NAME and FULLNAME
foreach (Type t in assembly.GetTypes())
if (name == t.Name || name == t.FullName || CodeGenUtil.Instance.UnMangle(name) == t.FullName)
{
if (name == t.Name || name == t.FullName || CodeGenUtil.Instance.UnMangle(name) == t.FullName)
{
type = t;
break;
}
type = t;
break;
}
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Where Note

This foreach loop
implicitly filters its target sequence
Loading
- consider filtering the sequence explicitly using '.Where(...)'.
catch
{
continue;
}
}
catch
{
continue;
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
}
}

return type
?? throw new AvroException($"Unable to find type '{name}' in all loaded " +
$"assemblies");
});
return type
?? throw new AvroException($"Unable to find type '{name}' in all loaded " +
$"assemblies");
}

private bool TryGetIListItemTypeName(string name, out string itemTypeName)
Expand Down
Loading