Avoid running duplicate/ unnecessary auto restore #1198
Conversation
jainaashish
force-pushed
the
asja-fix4307
branch
from
9e2436a
to
ba3b9fc
Compare
| /// Returns number of nominations still pending for .net core projects. | ||
| /// </summary> | ||
| /// <returns>Number of pending nominations.</returns> | ||
| int GetExpectedProjectsNomination(); |
There was a problem hiding this comment.
How about boolean IsAllProjectsNominated() instead? Actual count is not necessary and may be confusing.
There was a problem hiding this comment.
hmm makes sense! earlier i was validating changes with previous run so used int.. but now it makes more sense to just use boolean.
| { | ||
| // check if this .Net core project is nominated or not, and accordingly increase counter. | ||
| DependencyGraphSpec projectRestoreInfo; | ||
| if (_projectSystemCache.TryGetProjectRestoreInfo(cpsProject.MSBuildProjectPath, out projectRestoreInfo) && |
There was a problem hiding this comment.
Noted in other places we tend to use GetNuGetProjectSafeName for polling the cache. What if project created and not saved?
There was a problem hiding this comment.
TryGetProjectRestoreInfo is only being used in CPSProjectReferenceProject and it always passes projectFullPath which is why I passed the same thing here to have the consistency.
| // Drains the queue | ||
| while (!_pendingRequests.Value.IsCompleted | ||
| && !token.IsCancellationRequested) | ||
| { | ||
| SolutionRestoreRequest discard; | ||
| if (!_pendingRequests.Value.TryTake(out discard, IdleTimeoutMs, token)) | ||
| { | ||
| // check if there are pending nominations | ||
| currentExpectedNominations = _solutionManager.Value.GetExpectedProjectsNomination(); |
There was a problem hiding this comment.
Consider caching the result once all of the projects are nominated.
There was a problem hiding this comment.
thought so, but then there still could be cases where new projects are being created which are still missing nominations when it tries to run an auto restore which is why I didn't cache it. And also figuring out if projects are nominated or not, is a quick check so doesn't impact perf as well.
jainaashish
force-pushed
the
asja-fix4307
branch
3 times, most recently
from
b80be5a
to
b2e846e
Compare
| var netCoreProjects = GetNuGetProjects().OfType<NetCorePackageReferenceProject>().ToList(); | ||
|
|
||
| // count for nominated projects so far | ||
| var nominatedProjects = 0; |
There was a problem hiding this comment.
you don't have to count all of them, just find at least one without restore info.
| // if we're still expecting some nominations and also haven't reached our max timeout | ||
| // then delay it for 1500 msec and try again. | ||
| retries++; | ||
| await Task.Delay(DelayAutoRestoreTime); |
There was a problem hiding this comment.
Not needed. TryTake above will do wait just fine.
jainaashish
force-pushed
the
asja-fix4307
branch
from
b2e846e
to
2e0c428
Compare
jainaashish
force-pushed
the
asja-fix4307
branch
from
2e0c428
to
0267a34
Compare
|
@alpaix As per our discussion, I've updated the PR |
This PR will delay auto restore when we're still expecting some nominations to come from project system and make sure to run a single auto restore for all the projects which will save performance as well as avoid unnecessary error/ warning messages from multiple auto restores.
Also, this will maximum delay auto restore for 20 secs, even after 20 secs we're missing some nominations then we'll continue with it and let it fail assuming there is something wrong with project system which is why they were not able to nominate. This is the fallback mechanism so that we never delay auto restore indefinitely.
And lastly, Solution restore requests are never delayed and processed immediately as soon as they arrive so that it doesn't change any behavior for explicit restore.
Fixes NuGet/Home#4307
@rrelyea @DoRonMotter @alpaix @emgarten