Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make cache queries parallel #6468

Merged
merged 9 commits into from
Jun 1, 2021
Merged

Conversation

cdmihai
Copy link
Contributor

@cdmihai cdmihai commented May 20, 2021

Can review the commits individually. Ignore whitespace helps.

Context

The project cache was being queried serially. Oops.
This is because the monolithic BuildManager._syncLock was being held during the cache query, thus serializing all access.

Changes Made

Implements the 2nd option from the design doc: https://gist.github.com/cdmihai/0955cb217b2cbd66e18c89b20bf68319#2-reuse-how-the-buildmanager-offloads-parallel-work-to-build-nodes

  • Reverted code of ExecuteSubmission to what it was before project cache plugins.
  • Changed ExecuteSubmission to either issue a cache request or a build request (until now it only issues build requests)
  • The cache request is sent to the ProjectCacheService which submits it to the thread pool. This achieves parallelization of cache requests
  • Each cache request, on its own thread:
    • evaluates the project if necessary
    • does the cache query
  • When a cache request finishes in the ProjectCacheService it is posted back on the BuildManager work queue thread and is handled by either skipping the build or doing a real build.

Design time builds were a pain to get right this time. Previously design time builds were easy to deal with because the BuildManager detected them early enough. Now they get detected later in the project cache service. The build manager detects this and shuts the cache service off when design time builds are detected.

Testing

Added a parallel stress test. This should be a good test to both ensure the cache is queried in parallel and to stress test the concurrency in the engine.

Risk assessment

This should make the non project cache logic less risky than it was before, since I took the project cache logic out of BuildManager and moved it to the ProjectCacheService.

@cdmihai cdmihai force-pushed the parallel_cache_queries branch from b0a3a7a to 6588839 Compare May 25, 2021 00:54
@cdmihai cdmihai force-pushed the parallel_cache_queries branch from 6588839 to ab2c282 Compare May 27, 2021 21:14
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, a few questions.

@cdmihai
Copy link
Contributor Author

cdmihai commented Jun 1, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cdmihai cdmihai added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jun 1, 2021
@Forgind Forgind merged commit 2be2ece into dotnet:vs16.11 Jun 1, 2021
rainersigwald pushed a commit that referenced this pull request Sep 8, 2022
)

In #6468, the two callers of the method and the method itself were moved to `ProjectCacheService`. However, the original method remained behind as dead code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants