Remove unnecessary work and refactor CompareAssemblyIdentity

[AR-May]

Fixes #3930.

We remove the unused native call of CompareAssemblyIdentity (fusion.dll) and keep the custom managed implementation that duplicates it. Fusion implementation seems to be faster than its managed replacement, but it is only available for Windows. We, however, not sure that we run this code often enough for the difference to be noticeable. Current changes get us to a consistent implementation everywhere and are also a perf improvement since the unused native call won't happen.

[ladipro]

Looks good, although I'm wondering if it wouldn't be safer to remove the DllImport altogether, given that MSBuild has shipped with this bug. Thank you!

[ladipro]

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?

[rainersigwald]

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.

[AR-May]

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.

[ladipro]

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

[rainersigwald]

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.

[ladipro]

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

[AR-May]

Method	fUnified1	fUnified2	Mean	Error	StdDev	Median
FusionResult	False	False	2.966 μs	0.4104 μs	1.2037 μs	2.302 μs
CustomResult	False	False	38.637 μs	2.6889 μs	7.5400 μs	35.459 μs
FusionResult	False	True	2.666 μs	0.1218 μs	0.3476 μs	2.513 μs
CustomResult	False	True	34.042 μs	0.6132 μs	1.3067 μs	34.081 μs
FusionResult	True	False	2.221 μs	0.0395 μs	0.0834 μs	2.203 μs
CustomResult	True	False	65.518 μs	6.1692 μs	18.0932 μs	73.632 μs
FusionResult	True	True	5.192 μs	0.1031 μs	0.0914 μs	5.176 μs
CustomResult	True	True	53.541 μs	7.9186 μs	23.3482 μs	37.439 μs

[rainersigwald]

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.

[ladipro]

👍

[benvillalobos]

LGTM!