Fix performance bottleneck #4446
Conversation
ghost
added
the
|
|
|
|
||
| var allKeys = thisPackages.Keys.Concat(otherPackages.Keys) |
There was a problem hiding this comment.
The only reason I can see for the old implementation is if thisPackages or otherPackages can have a different StringComparer to OrdinalIgnoreCase.
Can this ever happen? I can't see tests for it
| .Distinct(StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| foreach (var key in allKeys) | ||
| foreach (var kv in thisPackages) |
There was a problem hiding this comment.
Thank you for your contribution.
Could you be able to include after change flame graph? We need to measure if it's improving or not.
Also, I believe there is correctness concern too.
thisPackage -> a, b,c
otherPackage -> b,c,d
Before your change result it a, b, c ,d
After your change b, c only.
You need to add another for loop iteration for otherPackages too.
We need to assert with unit test above concern.
|
@jeffkl investigated the same code a few months ago: #4256 Jeff, do you have any comments/feedback/learnings to share with this PR?
@danslocombe Unfortunately I don't have great trust in NuGet.Client's tests. I concur with @erdembayar's concern that there may be a correctness issue. As I commented in Jeff's PR, I was unable to find a test where a package is a transitive package of two different PackageReferences, each of which have different NoWarn values. I honestly have no idea if the transitive package is supposed to have both or neither of the NoWarn values 😕 |
|
Also, can you please improve the title of this PR? NuGet has an enormous surface, Visual Studio, various command line tools, many features that are not restore. This PR's title much too ambiguous as to what part of NuGet it's related to. |
Yeah the issue seems to be the Intersect doesn't perform an intersection if one of the input collections is null |
I don't follow, if one of them is null, then it returns the other one so technically it's still intersection, I believe it's expected. |
|
When I investigated the issue, I only discovered that the determination of transitive NoWarn with a complex graph could create millions of items and take forever. My attempt at a fix was here: https://github.com/NuGet/NuGet.Client/pull/4256/files which limited the transitive search to ignore already visited keys. There are a fair amount of unit tests for this class but I didn't have time to really dive into how much coverage they provide. |
|
@danslocombe |
|
@erdembayar Assigning this issue to you because it looks like you are engaging with the contributor. |
Bug
Fixes:
Regression? Last working version:
Description
Saw my nuget restores are quite slow on a mid-sized-project with the CPU pegged close to 100% on Windows.
Looking at a trace it seems like it is spending a lot of time intersecting properties from dependencies
In particular a Linq execution seems to be taking a long time.
This small pr improves the intersection hopefully bringing speedups :)
All the tests seem to be passing.
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation