-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Avoid cloning AssemblyName #2658
Conversation
This is causing 11% of all allocations building Roslyn.sln.
src/Shared/AssemblyUtilities.cs
Outdated
@@ -65,12 +65,19 @@ public static AssemblyName CloneIfPossible(this AssemblyName assemblyNameToClone | |||
name.SetPublicKey(assemblyNameToClone.GetPublicKey()); | |||
name.SetPublicKeyToken(assemblyNameToClone.GetPublicKeyToken()); | |||
name.Version = assemblyNameToClone.Version; | |||
name.Flags = assemblyNameToClone.Flags; | |||
name.CultureName = assemblyNameToClone.CultureName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting CultureName
will throw NIE on mono. You can set AssemblyName.CultureInfo
instead. See mono@d4c8715 and mono@42ba59a .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radical could you please file a bugzilla so we get this implemented? it should be trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assigning it when !MONO
. CultureInfo gets set via FEATURE_ASSEMBLYNAME_CULTUREINFO
src/Shared/AssemblyUtilities.cs
Outdated
name.ContentType = assemblyNameToClone.ContentType; | ||
name.ProcessorArchitecture = assemblyNameToClone.ProcessorArchitecture; | ||
|
||
#if FEATURE_ASSEMBLYNAME_CULTUREINFO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this section be restricted to setting name.CultureInfo
only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that. All the members under the flag are not supported on .net core. Since we already have a mess of API flags, I didn't want to increase the proliferation. I'll switch it with RUNTIME_TYPE_NETCORE
instead.
src/Shared/AssemblyUtilities.cs
Outdated
name.SetPublicKeyToken(assemblyNameToClone.GetPublicKeyToken()); | ||
name.Version = assemblyNameToClone.Version; | ||
name.Flags = assemblyNameToClone.Flags; | ||
name.ContentType = assemblyNameToClone.ContentType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContentType is a flag, this isn't needed and was deliberately omitted.
This is causing 11% of all allocations building Roslyn.sln.