Fix no-op restores for central package management#4523
Conversation
…tests Hash calculation for noop restore was was on String.GetHashCode() method. Unfortunately, there is a security feature (https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/runtime/userandomizedstringhashalgorithm-element#remarks) that makes string hashes calculated on a per application domain basis. Therefore same string produces different hash each time application is being run. It is being fixed by replacing String.GetHashCode() with SHA512 calculation.
| { | ||
| var hashCode = new HashCodeCombiner(); | ||
| foreach (var item in items) | ||
| using (var hashFunc = new Sha512HashFunction()) |
There was a problem hiding this comment.
I think the idea behind that might've been a perf optimization.
I can only imagine the perf gains are now gone right? We can just add it to the original hashing function?
There was a problem hiding this comment.
Basically this whole compressed concept, might be something that needs to go away.
There was a problem hiding this comment.
I can only imagine the perf gains are now gone right? We can just add it to the original hashing function?
Basically this whole compressed concept, might be something that needs to go away.
Yeah, I didn't think that it matters but apparently it has a non-negligible effect. Surprisingly, it is not the hash function itself that is slow. In profiler I can see that the fact that centralised versions are being sorted and also the conversion of VersionRange to string makes the "non-compressed" path to be actually slow:
foreach (var dependency in centralPackageVersions.OrderBy(dep => dep.Name))
{
writer.WriteNameValue(name: dependency.Name, value: dependency.VersionRange.ToNormalizedString());
}
I need to try more things to come up with final conclusion on how to handle this issue without sacrificing performance.
There was a problem hiding this comment.
I think we need to keep the "compressed" flag for now. Without it (7e53a50) the overall performance of (when measuring time of NuGet.ProjectModel.DependencyGraphSpec.GetHash() is almost 100% worse.
My latest commit (48c37d8) makes it only 10% worse. I think that it is acceptable cost for fixing no-op scenario.
We could try making it much faster by avoiding calculating a hash value multiple times for the same project. For large solutions that would mean massive savings. But it is a nontrivial change to do so I would like to keep it out of scope of this PR.
There was a problem hiding this comment.
Let's keep it then.
We could try making it much faster by avoiding calculating a hash value multiple times for the same project
The trick here is that you can't guarantee the package spec hasn't been changed.
Out of scope for this PR though. Thanks for validating that.
|
@nkolev92 do the "central" versions really matter? Whatever was resolved is what actually affects the graph and outcome. If you change a central version but it doesn't change the resolved versions of actually referenced packages, then a previous restore is still accurate right? I've seen these in the assets files and wondered why they are there, maybe we can just get rid of them? |
Not if you change a central transitive dependency. |
Ah so this is the |
|
Yep |
jeffkl
left a comment
There was a problem hiding this comment.
Thanks for the contribution @marcin-krystianc!
|
Will this fix be included in the next .NET 6 SDK release? |
|
It'll be in 6.0.400 SDK, 6.3 of NuGet, 17.3 of VS. |
Bug
Fixes: NuGet/Home#11696
Regression?
Last working version: dotnet 5.0.100. It has regressed because the runtime option:
UseRandomizedStringHashAlgorithmwasn't enabled before but now it is.Description
Hash calculation for no-op restore was relying on
String.GetHashCode()method.Unfortunately, there is a security feature (https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/runtime/userandomizedstringhashalgorithm-element#remarks) that makes string hashes calculated on a per-application domain basis. Therefore same string produces a different hash each time application is being run.
It is being fixed by replacing
String.GetHashCode()with SHA512 calculation.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation