Use AssemblyName.Clone when possible. #2016
Conversation
src/Shared/AssemblyNameExtension.cs
Outdated
| #else | ||
| newExtension.asAssemblyName = new AssemblyName(asAssemblyName.FullName); | ||
| #endif | ||
| newExtension.asAssemblyName = asAssemblyName.Clone2(); |
src/Shared/AssemblyUtilities.cs
Outdated
| AssemblyName result = null; | ||
| try | ||
| { | ||
| MethodInfo method = assemblyNameToClone?.GetType().GetMethod("Clone"); |
There was a problem hiding this comment.
Seems like this MethodInfo should be in a static lazy so we don't have to reflect in the hot path?
There was a problem hiding this comment.
Not caching reflection results is quite slow too. You can use static field to store typeof(AssemblyName).GetType().GetMethod("Clone") result.
There was a problem hiding this comment.
Good point. @AndyGerlicher if you do this, can you please do it for GetAssemblyLocation too? :)
src/Shared/AssemblyUtilities.cs
Outdated
| MethodInfo method = assemblyNameToClone?.GetType().GetMethod("Clone"); | ||
| result = (AssemblyName) method?.Invoke(assemblyNameToClone, null); | ||
| } | ||
| catch (MemberAccessException) |
There was a problem hiding this comment.
For that matter, catching here seems prone to slow.
src/Shared/AssemblyUtilities.cs
Outdated
| @@ -39,5 +39,23 @@ public static Type GetTypeInfo(this Type t) | |||
| return t; | |||
| } | |||
| #endif | |||
|
|
|||
| public static AssemblyName Clone2(this AssemblyName assemblyNameToClone) | |||
src/Shared/AssemblyUtilities.cs
Outdated
| catch (MemberAccessException) | ||
| { } | ||
|
|
||
| return result ?? new AssemblyName(assemblyNameToClone.FullName); |
There was a problem hiding this comment.
You could look over Clone's implementation and copy it here? :)
AndyGerlicher
force-pushed
the
assemblynameclone
branch
3 times, most recently
from
a2f9e21 to
7a63ca8
Compare
|
@dotnet-bot test Windows_NT Build for CoreCLR please |
| /// </summary> | ||
| private static void Initialize() | ||
| { | ||
| if (s_initialized) return; |
There was a problem hiding this comment.
Is this guaranteed to be run single-threaded?
There was a problem hiding this comment.
I guess it doesn't really matter if some things race to plunk the same value down
AssemblyName.Clone is not available in early versions of .NET Core. Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the Clone method directly. This change will attempt to call .Clone using reflection and if not available fallback to the previous .NET Core behavior. Potential fix for dotnet#2015 (when running on .NET Core 2.0).
AndyGerlicher
force-pushed
the
assemblynameclone
branch
from
7a63ca8 to
99340d4
Compare
|
@dotnet-bot test Windows_NT Build for CoreCLR please |
AssemblyName.Clone() is not available in early versions of .NET Core. Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the Clone method directly. This change will attempt to call .Clone using reflection and if not available fallback to the previous .NET Core behavior. Potential fix for #2015 (when running on .NET Core 2.0).
AssemblyName.Clone is not available in early versions of .NET Core.
Until we can upgrade MSBuild to use .NET Core 2.0, we can't call the
Clone method directly. This change will attempt to call .Clone using
reflection and if not available fallback to the previous .NET Core
behavior.
Potential fix for #2015 (when running on .NET Core 2.0).