Skip to content

C#: Avoid explicitly restoring projects in solution files. #14111

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 14, 2023
Merged

C#: Avoid explicitly restoring projects in solution files. #14111

merged 3 commits into from
Sep 14, 2023

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Aug 31, 2023 •
edited
Loading

In this PR we do the following performance optimisations to the dependency manager.

  • If solution file(s) are restored then we don't explictly restore projects that are up to date with respect to dotnet restore (after restoring the solution).
  • dotnet restore on projects are executed in parallel.

A couple of implementation details

  • We need to change the dotnet restore verbosity level to normal when restoring solutions to get all the relevant information related to restored projects.
  • The DotNet.cs file is starting to contain business logic. In C#: Re-factor Dotnet.cs to enable unit testing. #14142 we re-factor DotNet.cs to enable unit testing.

The changes have been tested on OrchardCMS/OrchardCore as it contains a solution file.
The numbers below show that we get the same accuracy for the comparison queries, but the wall clock time drops from approximately 16m to 9m (on my machine).

On main we have:

Results for OrchardCMS/OrchardCore
Query Files.ql:
Result count - both/traced/standalone: 4151/3692/20
Query Declarations.ql:
Result count - both/traced/standalone: 69128/84740/113
Result count in the matching 4151 files - both/traced/standalone: 69128/0/0
Matching 100.0%
Query Dependencies.ql:
Result count - both/traced/standalone: 491/2/229
Query CallTargets.ql:
Result count - both/traced/standalone: 87430/264261/213
Result count in the matching 4151 files - both/traced/standalone: 87430/1523/154
Matching 98.12%
Query TypeHierarchy.ql:
Result count - both/traced/standalone: 6351/4027/23
Result count in the matching 4151 files - both/traced/standalone: 6351/7/5
Matching 99.81%
Query Expressions.ql:
Result count - both/traced/standalone: 311660/1070097/1049
Result count in the matching 4151 files - both/traced/standalone: 311660/6370/870
Matching 97.73%
Query VariableAccesses.ql:
Result count - both/traced/standalone: 86270/193815/39
Result count in the matching 4151 files - both/traced/standalone: 86270/363/0
Matching 99.58%
Query TypeMentions.ql:
Result count - both/traced/standalone: 86709/131893/396
Result count in the matching 4151 files - both/traced/standalone: 86709/1439/319
Matching 98.01%

python3 diff_standalone.py 1510.23s user 299.76s system 188% cpu 15:58.18 total

On the feature branch including the unit test re-factor we have

Results for OrchardCMS/OrchardCore
Query Files.ql:
Result count - both/traced/standalone: 4151/3692/20
Query Declarations.ql:
Result count - both/traced/standalone: 69128/84740/113
Result count in the matching 4151 files - both/traced/standalone: 69128/0/0
Matching 100.0%
Query Dependencies.ql:
Result count - both/traced/standalone: 491/2/229
Query CallTargets.ql:
Result count - both/traced/standalone: 87439/264252/206
Result count in the matching 4151 files - both/traced/standalone: 87439/1514/147
Matching 98.14%
Query TypeHierarchy.ql:
Result count - both/traced/standalone: 6351/4027/23
Result count in the matching 4151 files - both/traced/standalone: 6351/7/5
Matching 99.81%
Query Expressions.ql:
Result count - both/traced/standalone: 311657/1070100/1056
Result count in the matching 4151 files - both/traced/standalone: 311657/6373/877
Matching 97.73%
Query VariableAccesses.ql:
Result count - both/traced/standalone: 86271/193814/39
Result count in the matching 4151 files - both/traced/standalone: 86271/362/0
Matching 99.58%
Query TypeMentions.ql:
Result count - both/traced/standalone: 86709/131893/396
Result count in the matching 4151 files - both/traced/standalone: 86709/1439/319
Matching 98.01%

python3 diff_standalone.py 1107.11s user 143.95s system 234% cpu 8:53.80 total

@github-actions github-actions bot added the C# label Aug 31, 2023
@michaelnebel michaelnebel force-pushed the csharp/reduceprojectrestore branch 3 times, most recently from 1d86206 to f41830f Compare September 5, 2023 08:48
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 5, 2023
View reviewed changes
@michaelnebel michaelnebel force-pushed the csharp/reduceprojectrestore branch from f41830f to e256144 Compare September 5, 2023 11:31
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Sep 5, 2023
@michaelnebel michaelnebel mentioned this pull request Sep 5, 2023
Merged
@michaelnebel michaelnebel marked this pull request as ready for review September 5, 2023 12:15
@michaelnebel michaelnebel requested a review from a team as a code owner September 5, 2023 12:15
tamasvajk
tamasvajk reviewed Sep 5, 2023
View reviewed changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

Added some questions and suggestions. Have you checked how the performance changes simply with using multiple threads for the restores?

@michaelnebel
Copy link
Contributor Author

Added some questions and suggestions. Have you checked how the performance changes simply with using multiple threads for the restores?

No, I have not - I will try that 👍

@michaelnebel
Copy link
Contributor Author

michaelnebel commented Sep 6, 2023 •
edited
Loading

Added some questions and suggestions. Have you checked how the performance changes simply with using multiple threads for the restores?

No, I have not - I will try that 👍

Result for only executing project restore in parallel can be seen below.
It is a bit better than the original solution, but it would ideal, if we can also rely projects being restored by dotnet restore on the solution.

Execution time when restoring projects in parallel (without using info from restore of solution).

Results for OrchardCMS/OrchardCore
Query Files.ql:
Result count - both/traced/standalone: 4151/3692/20
Query Declarations.ql:
Result count - both/traced/standalone: 69128/84740/113
Result count in the matching 4151 files - both/traced/standalone: 69128/0/0
Matching 100.0%
Query Dependencies.ql:
Result count - both/traced/standalone: 491/2/234
Query CallTargets.ql:
Result count - both/traced/standalone: 87451/264240/195
Result count in the matching 4151 files - both/traced/standalone: 87451/1502/136
Matching 98.16%
Query TypeHierarchy.ql:
Result count - both/traced/standalone: 6353/4025/22
Result count in the matching 4151 files - both/traced/standalone: 6353/5/4
Matching 99.86%
Query Expressions.ql:
Result count - both/traced/standalone: 311668/1070089/1045
Result count in the matching 4151 files - both/traced/standalone: 311668/6362/866
Matching 97.73%
Query VariableAccesses.ql:
Result count - both/traced/standalone: 86271/193814/39
Result count in the matching 4151 files - both/traced/standalone: 86271/362/0
Matching 99.58%
Query TypeMentions.ql:
Result count - both/traced/standalone: 86710/131892/395
Result count in the matching 4151 files - both/traced/standalone: 86710/1438/318
Matching 98.02%

python3 diff_standalone.py 2123.84s user 421.09s system 289% cpu 14:38.71 total

@michaelnebel
Copy link
Contributor Author

I re-ran the performance experiment (locally) for not using project information from .NET restore after explicitly deleting the target folder. Extraction of OrchardCMS/OrchardCore still took around 14 minutes with a fresh CLI.

@michaelnebel michaelnebel marked this pull request as draft September 11, 2023 11:23
@michaelnebel michaelnebel force-pushed the csharp/reduceprojectrestore branch from e256144 to 0127b77 Compare September 13, 2023 11:32
@michaelnebel
Copy link
Contributor Author

michaelnebel commented Sep 13, 2023 •
edited
Loading

Ran the extraction locally.
Just for the standalone extraction (from a cold cache) on OrchardCMS/OrchardCore, the extraction times are:

[build-stdout] Extraction completed in 00:02:48.1925152 (this feature branch)
[build-stdout] Extraction completed in 00:09:12.2875998 (main)

In this case we get a significant improvement.

@michaelnebel michaelnebel marked this pull request as ready for review September 13, 2023 11:55
tamasvajk
tamasvajk previously approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

LGTM

tamasvajk
tamasvajk approved these changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelnebel michaelnebel merged commit b9acf1a into github:main Sep 14, 2023
@michaelnebel michaelnebel deleted the csharp/reduceprojectrestore branch September 14, 2023 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@tamasvajk tamasvajk tamasvajk approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@michaelnebel @tamasvajk