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.

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

Avoid main thread switch in UpToDateCheckHost #9458

Merged
merged 8 commits into from
May 20, 2024
Merged

Avoid main thread switch in UpToDateCheckHost #9458

merged 8 commits into from
May 20, 2024

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented May 2, 2024 •
edited

Replaces API usage:

  • From _vsShell.GetProperty((int)__VSSPROPID.VSSPROPID_IsInCommandLineMode, ...)
  • To vsAppId.GetProperty((int)__VSAPROPID10.VSAPROPID_IsInCommandLineMode, ...)

The latter is free-threaded, as described in: https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki/40079/Free-threaded-replacements

This requires bumping several VS packages to 17.11, in order to use __VSAPROPID10.VSAPROPID_IsInCommandLineMode.

Also, IVsAppCommandLine is free-threaded, so don't use IVsUIService for that, as given in: https://github.com/microsoft/VSSDK-Analyzers/blob/614165cfdca3245dd8c19530bd054e24cd80be32/src/Microsoft.VisualStudio.SDK.Analyzers.CodeFixes/build/AdditionalFiles/vs-threading.MembersRequiringMainThread.txt#L84

Together, these changes mean we don't need to switch to the main thread in UpToDateCheckHost 👍

Microsoft Reviewers: Open in CodeFlow

@drewnoakes drewnoakes added Tenet-Performance This issue affects the "Performance" tenet. Feature-Up-to-date Build up-to-date check that avoids shelling out to MSBuild unless necessary. Performance-Scenario-Build This issue affects build performance. labels May 2, 2024
@drewnoakes drewnoakes requested a review from a team as a code owner May 2, 2024 09:43
@drewnoakes drewnoakes added this to the 17.11 milestone May 2, 2024
@drewnoakes
Copy link
Member Author

Test is failing with:

System.TypeLoadException : Method 'ImportMetadata' in type 'Microsoft.Build.Execution.ProjectItemInstance' from assembly 'Microsoft.Build, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' does not have an implementation.

Seems like something related to the package updates here has broken mock object proxy generation. That's a tomorrow problem!

@drewnoakes
Update VS packages to 17.11
7becab9
@drewnoakes
Avoid main thread switch in UpToDateCheckHost
7e694fb 
Change:

- From `_vsShell.GetProperty((int)__VSSPROPID.VSSPROPID_IsInCommandLineMode, ...)`
- To `vsAppId.GetProperty((int)__VSAPROPID10.VSAPROPID_IsInCommandLineMode, ...)`

The latter is free-threaded, as described in: https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki/40079/Free-threaded-replacements

Also, `IVsAppCommandLine` is free-threaded, so don't use `IVsUIService` for that, as given in: https://github.com/microsoft/VSSDK-Analyzers/blob/614165cfdca3245dd8c19530bd054e24cd80be32/src/Microsoft.VisualStudio.SDK.Analyzers.CodeFixes/build/AdditionalFiles/vs-threading.MembersRequiringMainThread.txt#L84

Together, these changes mean we don't need to switch to the main thread in `UpToDateCheckHost`.
@drewnoakes drewnoakes force-pushed the futdc-avoid-main-thread-switch branch from 2c5eb96 to 7e694fb Compare May 20, 2024 01:42
@drewnoakes
Copy link
Member Author

I rebased this PR and it now fails with a similar error:

System.TypeInitializationException : The type initializer for 'Microsoft.Build.Shared.FrameworkLocationHelper' threw an exception.
---- System.MissingMethodException : Method not found: 'Boolean Microsoft.Build.Framework.NativeMethods.get_IsMono()'.

It seems like we have an unexpected version of MSBuild DLLs at runtime. I've added an explicit reference to a version of MSBuild to try and fix this. Unfortunately the problem does not reproduce locally.

@drewnoakes
Copy link
Member Author

We now explicitly reference the version of MSBuild used in these tests, which makes CI pass.

@drewnoakes
Simplify use of IVsService
f804cd6
@drewnoakes
Convert to use AsyncLazy so that concurrent requests don't double-exe…
c0b81f7 
…cute

It seems likely that we would get multiple requests to the up-to-date check as we're called per-project, and many projects can build in parallel. Better to only do this work once.
@drewnoakes
Remove cancellation support
7f0b4b2 
AsyncLazy doesn't pass a cancellation token to the func. I tried to work around this with a CTS, but that doesn't work. If one caller cancels, it'll cancel all other callers, which isn't correct. Instead, we remove cancellation from the GetService calls, as these are cheap and cancellation is unlikely. Anyone waiting on the value would be correctly cancelled still.
@drewnoakes drewnoakes mentioned this pull request May 20, 2024
Merged
@drewnoakes drewnoakes merged commit a8a739f into dotnet:main May 20, 2024
5 checks passed
@drewnoakes drewnoakes deleted the futdc-avoid-main-thread-switch branch May 20, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davkean davkean davkean approved these changes

@kvenkatrajan kvenkatrajan kvenkatrajan approved these changes

Assignees
No one assigned
Labels
Feature-Up-to-date Build up-to-date check that avoids shelling out to MSBuild unless necessary. Performance-Scenario-Build This issue affects build performance. Tenet-Performance This issue affects the "Performance" tenet.
Projects
None yet
Milestone
17.11
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@drewnoakes @davkean @kvenkatrajan