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

Jump to bottom

Remove unnecessary work and refactor CompareAssemblyIdentity #5973

Merged
merged 4 commits into from
Jan 15, 2021
Merged

Remove unnecessary work and refactor CompareAssemblyIdentity #5973

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
814f5f0
Remove unnecessary work and refactor CompareAssemblyIdentity (#3930)
AR-May Dec 9, 2020
bbb61cc
Remove the native call for CompareAssemblyIdentity (fusion.dll) in Ar…
AR-May Dec 22, 2020
a7563b8
Move AreAssembliesEquivalent from NativeMethods.cs
AR-May Dec 22, 2020
b5d46e2
Fix comment wording
AR-May Dec 22, 2020
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
6 changes: 2 additions & 4 deletions src/Tasks/AssemblyDependency/ReferenceTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2279,14 +2279,12 @@ private static int ResolveAssemblyNameConflict(AssemblyNameReference assemblyRef
bool rightConflictLegacyUnified = !isNonUnified && assemblyReference1.reference.IsPrimary;

// This is ok here because even if the method says two versions are equivalent the algorithm below will still pick the highest version.
NativeMethods.CompareAssemblyIdentity
bool equivalent = NativeMethods.AreAssembliesEquivalent
(
leftConflictFusionName,
leftConflictLegacyUnified,
rightConflictFusionName,
rightConflictLegacyUnified,
out bool equivalent,
out _
rightConflictLegacyUnified
);

Version leftConflictVersion = assemblyReference0.assemblyName.Version;
Expand Down
51 changes: 12 additions & 39 deletions src/Tasks/NativeMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1108,18 +1108,16 @@ internal static extern int CompareAssemblyIdentityWindows
string pwzAssemblyIdentity2,
[MarshalAs(UnmanagedType.Bool)] bool fUnified2,
[MarshalAs(UnmanagedType.Bool)] out bool pfEquivalent,
out AssemblyComparisonResult pResult
out int pResult
);

// TODO: Verify correctness of this implementation and
// extend to more cases.
internal static void CompareAssemblyIdentity(
internal static bool AreAssembliesEquivalent(
string assemblyIdentity1,
bool fUnified1,
string assemblyIdentity2,
bool fUnified2,
out bool pfEquivalent,
out AssemblyComparisonResult pResult)
bool fUnified2)
{
#if FEATURE_FUSION_COMPAREASSEMBLYIDENTITY
if (NativeMethodsShared.IsWindows)
Expand All @@ -1129,46 +1127,38 @@ internal static void CompareAssemblyIdentity(
fUnified1,
assemblyIdentity2,
fUnified2,
out pfEquivalent,
out pResult);
out bool pfEquivalent,
out _);
return pfEquivalent;
Copy link
Member

Choose a reason for hiding this comment

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

Without the new return statement the results of CompareAssemblyIdentityWindows were ignored so it looks like it would be safer to not call this method at all and keep comparing identities in C# on all platforms. Is performance the reason why you chose to keep the Windows-only DllImport?

Copy link
Member

Choose a reason for hiding this comment

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

On the one hand, going back to the original, long-proven code is nice. On the other, evidently this managed implementation is ok since the return was ignored! I wish we had more testing in this area to keep that true, so I filed #5974. But that doesn't need to block this!

Since this is a very perf-sensitive area, it might be nice to do some testing--if the native implementation is noticeably faster, we should probably keep it. Unfortunately I don't think it'll be easy to isolate that with a good set of test cases.

Copy link
Member Author

@AR-May AR-May Dec 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I did some benchmarking and the fusion implementation seems to be ~10 times faster than its managed replacement. I, however, not sure it should mean so much for a decision, as I am not sure we run this code often enough for the difference to be noticeable: it is called only when we have a conflict (+ there are also other conditions). It is also clear that the managed implementation is meant to be just a replacement for the absent fusion function (see the TODO comment, line 1114) and I believe it might be less safe than the original fusion.dll code. From the other point of view, this managed code was shipped and also worked so far. So, well, it is hard to decide what is actually safer: to keep the fusion.dll or this managed implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for following up! It would probably make sense to copy your comment to the PR description and consequently to the commit message.

Copy link
Member

Choose a reason for hiding this comment

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

Team triage: talked with @ladipro and @Forgind. We now think we should just delete the native call entirely--that's what we've been doing, it's a perf improvement on Windows (since the native call won't happen), and it gets us to a consistent implementation everywhere.

}
#endif

AssemblyName an1 = new AssemblyName(assemblyIdentity1);
AssemblyName an2 = new AssemblyName(assemblyIdentity2);

//pfEquivalent = AssemblyName.ReferenceMatchesDefinition(an1, an2);
pfEquivalent = RefMatchesDef(an1, an2);
if (pfEquivalent)
if (RefMatchesDef(an1, an2))
{
pResult = AssemblyComparisonResult.ACR_EquivalentFullMatch;
return;
return true;
}

if (!an1.Name.Equals(an2.Name, StringComparison.OrdinalIgnoreCase))
{
pResult = AssemblyComparisonResult.ACR_NonEquivalent;
pfEquivalent = false;
return;
return false;
}

var versionCompare = an1.Version.CompareTo(an2.Version);

if ((versionCompare < 0 && fUnified2) || (versionCompare > 0 && fUnified1))
{
pResult = AssemblyComparisonResult.ACR_NonEquivalentVersion;
pfEquivalent = true;
return;
return true;
}

if (versionCompare == 0)
{
pResult = AssemblyComparisonResult.ACR_EquivalentFullMatch;
pfEquivalent = true;
return;
return true;
}

pResult = pfEquivalent ? AssemblyComparisonResult.ACR_EquivalentFullMatch : AssemblyComparisonResult.ACR_NonEquivalent;
return false;
}

// Based on coreclr baseassemblyspec.cpp (https://github.com/dotnet/coreclr/blob/4cf8a6b082d9bb1789facd996d8265d3908757b2/src/vm/baseassemblyspec.cpp#L330)
Expand Down Expand Up @@ -1229,23 +1219,6 @@ private static bool CompareRefToDef(AssemblyName @ref, AssemblyName def)
return true;
}

internal enum AssemblyComparisonResult
{
ACR_Unknown, // Unknown
ACR_EquivalentFullMatch, // all fields match
ACR_EquivalentWeakNamed, // match based on weak-name, version numbers ignored
ACR_EquivalentFXUnified, // match based on FX-unification of version numbers
ACR_EquivalentUnified, // match based on legacy-unification of version numbers
ACR_NonEquivalentVersion, // all fields match except version field
ACR_NonEquivalent, // no match

ACR_EquivalentPartialMatch,
ACR_EquivalentPartialWeakNamed,
ACR_EquivalentPartialUnified,
ACR_EquivalentPartialFXUnified,
ACR_NonEquivalentPartialVersion
}

//------------------------------------------------------------------------------
// PFXImportCertStore
//------------------------------------------------------------------------------
Expand Down