Skip to content

C#: Parallelize restore logic of missing packages #14243

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

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

tamasvajk
Copy link
Contributor

@tamasvajk tamasvajk commented Sep 18, 2023

This PR

  • parallelizes the implementation of DependencyManager.DownloadMissingPackages,
  • adds the managed thread ID to all the extractor log messages, and
  • merges two implementations of the Process starting logic.

Commit-by-commit review is suggested.

@github-actions github-actions bot added the C# label Sep 18, 2023
@tamasvajk tamasvajk force-pushed the parallelize-restore branch 2 times, most recently from b04c661 to 294b0f5 Compare September 18, 2023 11:30
@tamasvajk
Copy link
Contributor Author

tamasvajk commented Sep 20, 2023

Sample log messages when running work on multiple threads:

[014] Running dotnet restore --no-dependencies "/var/folders/hh/stwc86wd1pb8hzqj5vqj4ky80000gn/T/GitHub/packages/06957661922ef355" ...
[016] Running dotnet restore --no-dependencies "/var/folders/hh/stwc86wd1pb8hzqj5vqj4ky80000gn/T/GitHub/packages/58abd53305b3ceeb" ...
[025] Running dotnet restore --no-dependencies "/var/folders/hh/stwc86wd1pb8hzqj5vqj4ky80000gn/T/GitHub/packages/294e3ff335eb900b" ...
[016]   Determining projects to restore...
[014]   Determining projects to restore...
[025]   Determining projects to restore...
[016]   Restored /var/folders/hh/stwc86wd1pb8hzqj5vqj4ky80000gn/T/GitHub/packages/58abd53305b3ceeb/58abd53305b3ceeb.csproj (in 2.64 sec).
[014]   Restored /var/folders/hh/stwc86wd1pb8hzqj5vqj4ky80000gn/T/GitHub/packages/06957661922ef355/06957661922ef355.csproj (in 2.67 sec).
[025]   Restored /var/folders/hh/stwc86wd1pb8hzqj5vqj4ky80000gn/T/GitHub/packages/294e3ff335eb900b/294e3ff335eb900b.csproj (in 3.34 sec).

csharp.log now contains process and thread IDs:

[15961] [001] [INFO] EXTRACTION STARTING at 09/19/2023 15:25:17
[15961] [001] [INFO]   Extractor: .../codeql/csharp/tools/osx64/Semmle.Extraction.CSharp.Standalone.dll
[15961] [001] [INFO]   Extractor version: codeql-cli/v2.14.3 (c78cd73edfddf4faae8a3167e4b585ee21f2de53)
[15961] [001] [INFO]   Current working directory: ...
[15961] [001] [INFO]   Models constructed in 00:00:01.4667715

@tamasvajk tamasvajk marked this pull request as ready for review September 20, 2023 11:45
@tamasvajk tamasvajk requested a review from a team as a code owner September 20, 2023 11:45
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

This is much nicer! Well done!

})
.ToList();
foreach (var line in stdoutLines)
Parallel.ForEach(projects, new ParallelOptions { MaxDegreeOfParallelism = options.Threads }, project =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: Did you measure the performance impact of using this "Parallel" library instead of PLINQ?

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case, this is much nicer :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't. But I'm under the impression that they do the same.

var threadId = $"[{Environment.CurrentManagedThreadId:D3}]";
void onOut(string s)
{
Console.Out.WriteLine($"{threadId} {s}");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could be inlined as lambdas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it could be. (I think in general local functions perform better than lambdas. But not in this case, because we're passing the local function as an Action delegate.)

@tamasvajk tamasvajk merged commit 011391b into github:main Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants