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

Switch to using the .NET SDK properties that are defined to provide the TFM instead of using the assembly that defines System.Object #80646

Merged

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jan 14, 2023

Switch to using MSBuild properties defined by the .NET SDK or inferring the TFM from the generator version (as it ships with the TFM and is versioned identically) and using another SyntaxProvider to enable us to construct a StubEnvironment object accurately without having to query any information directly from the Compilation object. In addition to fixing #80621, this change effectively enables all of the calculation work here to be done less often in a VS session. Previously we recalculated all of the information per keystroke. Now we'll only recalculate the SkipLocalsInit check when the user provides or removes a SkipLocalsInit attribute in their code or changes their TFM.

We should backport this PR to .NET 7 to ensure good performance in VS.

Fixes #80621

…he TFM instead of using the assembly that defines System.Object
…ational version of the source generator. This will allow this change to be used by the SDK without SDK changes to expose the TFM properties to the source generators (as we don't ship Microsoft.Interop.LibraryImportGenerator.targets outside of the repository today).
@ghost
Copy link

ghost commented Jan 14, 2023

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Switch to using MSBuild properties defined by the .NET SDK or inferring the TFM from the generator version (as it ships with the TFM and is versioned identically) and using another SyntaxProvider to enable us to construct a StubEnvironment object accurately without having to query any information directly from the Compilation object. In addition to fixing #80621, this change effectively enables all of the calculation work here to be done less often in a VS session. Previously we recalculated all of the information per keystroke. Now we'll only recalculate the SkipLocalsInit check when the user provides or removes a SkipLocalsInit attribute in their code or changes their TFM.

We should backport this PR to .NET 7 to ensure good performance in VS.

Author: jkoritzinsky
Assignees: -
Labels:

area-System.Runtime.InteropServices, tenet-performance, source-generator

Milestone: -

@jaredpar
Copy link
Member

@chsienki PTAL

…ed mechanism to determine TFM since it's more for correctness than perf.
Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

LGTM. An alternative approach would be to use the MetadataReferencesProvider to look at System.Runtime or appropriate to figure out the target version rather than relying on the TFM from MSBuild; that would also only change when the user actually changes the TFM, rather than per key stroke, but I think either approach is fine.

@AaronRobinsonMSFT
Copy link
Member

An alternative approach would be to use the MetadataReferencesProvider to look at System.Runtime or appropriate to figure out the target version rather than relying on the TFM from MSBuild;

@chsienki Would this mean looking at the version of the System.Runtime assembly?

@jkoritzinsky
Copy link
Member Author

I looked at using the MetadataReferencesProvider. The up-side is that it will work in more scenarios and we won't need to fall back to our "use current assembly version" case as often. However, it looks like it can be a file path or an assembly name, which makes it hard to parse and adds complexity. I think for now, this is the best solution.

@jkoritzinsky
Copy link
Member Author

The timeout is a known issue according to Build Analysis, so I'm going to merge this in.

@jkoritzinsky jkoritzinsky merged commit 729f22c into dotnet:main Jan 18, 2023
@jkoritzinsky jkoritzinsky deleted the stub-environment-no-compilation branch January 18, 2023 22:06
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
…he TFM instead of using the assembly that defines System.Object (dotnet#80646)

Fixes dotnet#80621
@dotnet dotnet locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interop Source Generator TFM check is very expensive
4 participants