Fix lock contention in ProjectRootElementCache.Get #6680
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ns (loading, etc) is out of a lock.
rainersigwald
approved these changes
Jul 15, 2021
rokonec
approved these changes
Jul 16, 2021
|
Experimental insertion in VS showed problems with these changes. Converting this PR to draft while I investigate and fix. |
…ojectRootElementCache not to insert an element to the cache.
…et function, and not in openLoader delegate function.
AR-May
force-pushed
the
lock-contention-in-ProjectRootElementCache-3
branch
from
57cfc33
to
a6db102
Compare
|
Ready for review now, the errors does not seem to be related to the changes after investigation. |
ladipro
reviewed
Jul 21, 2021
| @@ -63,8 +63,11 @@ private ProjectRootElement GetFromOrAddToCache(string projectFile, OpenProjectRo | |||
| ErrorUtilities.VerifyThrowInternalNull(rootElement, "projectRootElement"); | |||
| ErrorUtilities.VerifyThrow(rootElement.FullPath.Equals(key, StringComparison.OrdinalIgnoreCase), | |||
| "Got project back with incorrect path"); | |||
|
|
|||
| AddEntry(rootElement); | |||
There was a problem hiding this comment.
May I ask you to explain this statement? We're inside the GetOrAdd callback so rootElement will be added to the cache with the key of projectFile. Why are we calling another add?
There was a problem hiding this comment.
Yes, I removed AddEntry from the end of all the open functions in order to divide downloading and adding to the cache code. So, now everywhere else where I used open functions, I now need to do AddEntry exactly afterwards, so that behavior is exactly the same as before.
Here, in case of SimpleProjectRootElementCache, AddEntry does a bit more than just adding to _cache (it raises an event additionally).
There was a problem hiding this comment.
Thank you, yes, it does what it was doing before. It doesn't look right, though, to be adding inside the GetOrAdd callback. It means that the item will be added twice - by AddEntry and then attempted another add by GetOrAdd after the callback returns. It looks as though this should be a TryGetValue instead of GetOrAdd.
Just a perf nit, really, and OK to ignore.
ladipro
approved these changes
Jul 21, 2021
Forgind
added
the
merge-when-branch-open
Forgind
pushed a commit
that referenced
this pull request
Dec 23, 2021
Fixes possible race condition in ProjectRootElementCache.Get supposedly introduced in #6680. Context Passing a load function that uses ProjectRootElementCache.Get to ProjectRootElementCache.Get function could lead to failure (to verify a path in the loaded ProjectRootElement), due to a race condition. We eliminate calling ProjectRootElementCache.Get inside the load function, eliminating an unnecessary double entry to ProjectRootElementCache.Get function. Changes Made Remove double entry to ProjectRootElementCache.Get function that could cause race condition. Adjust path verification in ProjectRootElementCache and SimpleProjectRootElementCache to be more safe and to have more information. Testing Unit tests + manual testing Notes The stack trace of failure: > ##[error]C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): Error : MSB0001: Internal MSBuild Error: Got project back with incorrect path > at Microsoft.Build.Shared.ErrorUtilities.ThrowInternalError(String message, Exception innerException, Object[] args) > at Microsoft.Build.Evaluation.ProjectRootElementCache.Get(String projectFile, OpenProjectRootElement openProjectRootElement, Boolean isExplicitlyLoaded, Nullable1 preserveFormatting) > at Microsoft.Build.Evaluation.Evaluator4.ExpandAndLoadImportsFromUnescapedImportExpression(String directoryOfImportingFile, ProjectImportElement importElement, String unescapedExpression, Boolean throwOnFileNotExistsError, List1& imports) > at Microsoft.Build.Evaluation.Evaluator4.ExpandAndLoadImportsFromUnescapedImportExpressionConditioned(String directoryOfImportingFile, ProjectImportElement importElement, List1& projects, SdkResult& sdkResult, Boolean throwOnFileNotExistsError) > at Microsoft.Build.Evaluation.Evaluator4.ExpandAndLoadImports(String directoryOfImportingFile, ProjectImportElement importElement, SdkResult& sdkResult) > at Microsoft.Build.Evaluation.Evaluator4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement) > at Microsoft.Build.Evaluation.Evaluator4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport) > at Microsoft.Build.Evaluation.Evaluator4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement) > at Microsoft.Build.Evaluation.Evaluator4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport) > at Microsoft.Build.Evaluation.Evaluator4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement) > at Microsoft.Build.Evaluation.Evaluator4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport) > at Microsoft.Build.Evaluation.Evaluator4.EvaluateImportElement(String directoryOfImportingFile, ProjectImportElement importElement) > at Microsoft.Build.Evaluation.Evaluator4.PerformDepthFirstPass(ProjectRootElement currentProjectOrImport) > at Microsoft.Build.Evaluation.Evaluator4.Evaluate()
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3039
Context
There is significant amount of lock contentions in ProjectRootElementCache.Get method.
(For OrchardCore project, for example, the total time spent waiting to take a lock ~1sec.)
Reason: the code is using a single lock to control to access the cache, and also read file into XmlDocument, when it does not exist. The later one can be slow on a slow disk.
Changes Made
The ProjectRootElementCache.Get function is rewritten to exclude IO operations from the locked sections.
For this goal we also had to separate logic of loading from logic of adding the element to the cache.
Testing
Unit tests + manual testing.