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

Improve Perf for Azure Pipelines build #450

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

Nirmal4G
Copy link
Contributor

@Nirmal4G Nirmal4G commented Sep 20, 2022

Changes

  • Use 'windows-latest' image.
  • Narrow Test Results path pattern.
  • Do Restore separately from Build step.
  • Don't do previous steps like restore/build.
  • Use 'BuildConfiguration' for 'Release' build.
  • Remove tools that are already present in the image.
  • Use Multi-Level lookup to use already installed runtime(s).

This reduces build times by 20%-40% approx.
If we can cache .NET SDKs and NuGet packages,
we could further reduce build times by another 30%!

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • Tested code with current supported SDKs
  • Contains NO breaking changes
  • Code follows all style conventions

Other information

Rebase or Squash merge the PR and set its message to title and description.

@Nirmal4G
Copy link
Contributor Author

Azure Pipeline references:

- Use 'windows-latest' image.
- Narrow Test Results path pattern.
- Do Restore separately from Build step.
- Don't do previous steps like restore/build.
- Use 'Build.Configuration' for 'Release' build.

- Remove tools that are already present in the image.
- Use Multi-Level lookup to use installed runtime(s).

This reduces the build time by 20% approx.
If .NET SDKs and NuGet packages are cached,
the build time further reduces by another 20%!
- Cache everything under '$(Agent.ToolsDirectory)/dotnet'
  directory to speed up the build. Check for changes in
  the .NET SDK Version to invalidate and rebuild
  the cache during subsequent builds.
@Nirmal4G Nirmal4G force-pushed the feature/improve-azp-perf branch 2 times, most recently from fc8a1ba to e91c609 Compare September 21, 2022 10:59
azure-pipelines.yml Outdated Show resolved Hide resolved
@Nirmal4G Nirmal4G force-pushed the feature/improve-azp-perf branch from e91c609 to 9b1ebda Compare September 21, 2022 14:16
@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Sep 21, 2022

Almost done. Roslyn compiler supports file_header_template. We could use that instead of Update-Headers.ps1 script. Remove the invocation from the pipeline.

See the changes here: f520a84. If you are okay with that, I'll either open a new PR or add it here.

@Nirmal4G Nirmal4G requested a review from Sergio0694 September 22, 2022 07:07
azure-pipelines.yml Outdated Show resolved Hide resolved
@Nirmal4G
Copy link
Contributor Author

There's a couple different bugs in Roslyn 4.3, one is that issue with S.C.I you linked, and another is one that causes ForAttributeWithMetadataName to crash at runtime...We'd still need the .NET 7 SDK anyway because I'll be adding a .NET 7 target to the HighPerformance shortly...

Both of them seem to be compiler issues and fix was already provided the next day. I have fixed them in my fork and it also doesn't need .NET 7 SDK. By "shortly" if you mean in next few days then this patch might not be worth merging. But if you see the changes, you might know whether you need it or not.

See the changes here: 39ea152. If you're okay with the changes, we can also drop the "Cache .NET SDKs and Tools" commit.

@Sergio0694
Copy link
Member

"fix was already provided the next day."

We can't rely on that unfortunately. Even if the latest VS 17.3.x build is fixed, that still means that anyone using a previous version (including in CIs) would get random crashes from the generators without a clear explanation as to why. It's just safer to skip to 4.4 directly, which is what I've done. People on 17.3 would still just use the previous generator anyway, which still works, it's just a bit less efficient. 17.4 goes GA in November which is when I plan to release 8.1, so that's fine 🙂

@Nirmal4G Nirmal4G force-pushed the feature/improve-azp-perf branch from 3e016f1 to 9b1ebda Compare September 22, 2022 13:47
@Nirmal4G Nirmal4G requested a review from Sergio0694 September 22, 2022 14:07
@Sergio0694
Copy link
Member

Sergio0694 commented Sep 22, 2022

@Nirmal4G I love that change, but we need files without the header to also produce an error, is that the case?

NOTE: that should be in a separate PR either way.

@Nirmal4G
Copy link
Contributor Author

we need files without the header to also produce an error, is that the case?

Yes, it does produce error and also suggest a fix for both header less and non-matching scenarios.

@Sergio0694
Copy link
Member

Awesome, then yeah let's do that too (in another PR) 😄

While at it, can we also normalize the whitespace of all files in the solution, given the header will also have them? All files should just be using \n. Right now I'm not sure there's a line terminator set, and I've seen plenty of files using CLRF instead.

@Nirmal4G
Copy link
Contributor Author

While at it, can we also normalize the whitespace of all files in the solution, given the header will also have them?

All files are already normalized. I already did this in my previous PRs in Windows repo and recently updated Git Files in here #304.

All files should just be using \n. Right now, I'm not sure there's a line terminator set, and I've seen plenty of files using CLRF instead.

Git always stores text files with LF (\n) terminator. Git by default checks out and normalizes the line-endings to match each platform's native line-ending (Mac: CR, Unix/Linux: LF, Windows: CRLF).

@Nirmal4G Nirmal4G marked this pull request as ready for review September 22, 2022 15:55
azure-pipelines.yml Show resolved Hide resolved
azure-pipelines.yml Show resolved Hide resolved
azure-pipelines.yml Show resolved Hide resolved
@Nirmal4G Nirmal4G requested a review from Sergio0694 September 26, 2022 16:41
azure-pipelines.yml Show resolved Hide resolved
Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

:shipit:

@Sergio0694 Sergio0694 merged commit bdf9138 into CommunityToolkit:main Sep 28, 2022
@Nirmal4G Nirmal4G deleted the feature/improve-azp-perf branch September 28, 2022 16:25
@Sergio0694
Copy link
Member

@Nirmal4G Getting this CI failure in https://dev.azure.com/dotnet/CommunityToolkit/_build/results?buildId=79569&view=logs&j=4a47a688-b50e-58f6-4d2c-6d1b829fc8e2&t=2bf27cc2-dcab-5f34-f3ae-ecb203c09e81:

C:\hostedtoolcache\windows\dotnet\sdk\7.0.100-rc.1.22431.12\NuGet.targets(396,5): error MSB3202: The project file "D:\a\1\s\tests\CommunityToolkit.Mvvm.Roslyn431.UnitTests\CommunityToolkit.Mvvm.Roslyn431.UnitTests.csproj" was not found. [D:\a\1\s\dotnet Community Toolkit.sln]

Is this a regression caused by this PR?

Branch is https://github.com/CommunityToolkit/dotnet/tree/dev/roslyn-431.

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Oct 4, 2022

@Sergio0694 The error says Project file not found. I don't think it is related to this PR. Maybe you forgot to commit this CommunityToolkit.Mvvm.Roslyn431.UnitTests renamed file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants