Process projects in parallel in FAR. #45074
Conversation
CyrusNajmabadi
force-pushed
the
farProjectParallel
branch
from
cda74a3 to
c983c3a
Compare
src/Features/Core/Portable/SpellCheck/AbstractSpellCheckCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
| // have to create all their dependent compilations in order to get their compilation. | ||
| // This would be very expensive and would take a lot of time before we got our first | ||
| // result. | ||
| var connectedProjects = _dependencyGraph.GetDependencySets(_cancellationToken); |
There was a problem hiding this comment.
this is no longer an issue as we can still get results uickly by processing hte compilations in parallel, thus returning results quickly for the compilations we can get quickly.
...ces/Core/Portable/FindSymbols/FindReferences/FindReferencesSearchEngine_ProjectProcessing.cs
Outdated
Show resolved
Hide resolved
…ferencesSearchEngine_ProjectProcessing.cs
| connectedProjectSet, projectToDocumentMap).ConfigureAwait(false); | ||
| } | ||
| foreach (var (project, documentMap) in projectToDocumentMap) | ||
| tasks.Add(Task.Run(() => ProcessProjectAsync(project, documentMap), _cancellationToken)); |
There was a problem hiding this comment.
Is Task.Run here just to pass cancellationToken?
There was a problem hiding this comment.
It's to ensure that the work is truly parallel and the main lol can get to the next item to add to the work queue. If there work doesn't actually ever yield, and if entirely synchronous, then not doing this could have no benefit.
There was a problem hiding this comment.
It's to ensure that the work is truly parallel and the main lol can get to the next item to add to the work queue.
I looked back here to read your answer again. But I think that's not what I was asking.
I was wondering why not: tasks.Add(ProcesssProjectAsync(...))? Is cancellationToken the only reason?
There was a problem hiding this comment.
I was wondering why not: tasks.Add(ProcesssProjectAsync(...))? Is cancellationToken the only reason?
@CyrusNajmabadi
No. It's so the work is actually parallel. Adding lots of async work doesn't make anything acutally parallel. Consider something as simple as this:
public Task ExpensiveAsync()
{
Thread.Sleep(10000);
return Task.CompletedTask;
}
if we go in a loop 10 times and attempt to add ExpensiveAsync to an array of tasks, it will take 100 seconds. By doing Task.Run, we explicitly move this work to a threadpool thread, and allow the work to operate entirely concurrently, allowing it all to finish in 10 seconds.
This is obviously an extreme case (total thread blocking, no actual asynchrony), but the general idea still holds. Putting a lot of async work into a queue doesn't make it necessary concurrent. Doing the Task.Run does ensure this.
There was a problem hiding this comment.
This is obviously an extreme case (total thread blocking, no actual asynchrony), but the general idea still holds.
Got it. Just so that I understand, it does depend on the ProcesssProjectAsync implementation to be possibly CPU-bound/Blocking, and even for IO-bound scenarios, Task.Run will ensure any "additional work" that is being done as part of it to run in parallel.
There was a problem hiding this comment.
Yes. tasks.Add(ProcesssProjectAsync(...)) is possibly concurrent. tasks.Add(Task.Run(() is definitely concurrent.
| // Now actually process the project. | ||
| await ProcessProjectAsync(project, documentMap).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private async Task ProcessProjectAsync( |
There was a problem hiding this comment.
this is the only bit of code in this file, could probably just be moved to FindReferencesSearchEngine.cs now that it's much simpler
There was a problem hiding this comment.
Agreed. can clean up in my continuing followup PRs :)
ghost
added this to the 
In Roslyn.sln:
We don't get full linear speedup because, while there are opportunities for parallel speedup, there are also several times where things become linear as several projects wait on the compilation for a dependent project.