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

Provide parse options to the language service more quickly #60855

Merged

Conversation

jasonmalinowski
Copy link
Member

@jasonmalinowski jasonmalinowski commented Apr 20, 2022

When we load a project in Visual Studio, the project system first does an evaluation pass of the project, and hands the resulting list of items to the language service. It then does an execution pass executing CoreCompile passing SkipCompilerExecution=true and ProvideCommandLineArgs=true, that resulting command line string is where we get our compiler switches. The execution pass is much slower than the evaluation pass, so there's a window of time where we have a list of files, but not any options yet.

Because there's a gap, that means there's a time where we are parsing source files with the default parse options. We'll then have to reparse them a second time which isn't great. It also means any cache lookups we do won't have the right options either, so the cache lookups might miss.

To help this, we'll have a property for the evaluation pass which is an "approximation" of the options that would come out of CoreCompile, but only the ones that matter for parsing. It's acceptable for this to be imperfect: once the execution pass is complete we'll use those options instead, so any behaviors here that don't match the real command line generation will only be temporary, and probably won't be any worse than having no options at all.

This is the targets side of #60014; the project system changes were already made and have been merged.

@JoeRobich

This comment was marked as outdated.

When we load a project in Visual Studio, the project system first does
an evaluation pass of the project, and hands the resulting list of
<Compile> items to the language service. It then does an execution pass
executing CoreCompile passing SkipCompilerExecution=true and
ProvideCommandLineArgs=true, that resulting command line string is where
we get our compiler switches. The execution pass is much slower than the
evaluation pass, so there's a window of time where we have a list of
files, but not any options yet.

Because there's a gap, that means there's a time where we are parsing
source files with the default parse options. We'll then have to reparse
them a second time which isn't great. It also means any cache lookups we
do won't have the right options either, so the cache lookups might miss.

To help this, we'll have a property for the evaluation pass which is an
"approximation" of the options that would come out of CoreCompile, but
only the ones that matter for parsing. It's acceptable for this to be
imperfect: once the execution pass is complete we'll use those options
instead, so any behaviors here that don't match the real command line
generation will only be temporary, and probably won't be any worse than
having no options at all.
We originally obsoleted this in favor of the other overload that we
added; we've since found good reasons to use the original API as well
in dotnet/project-system#8085, so we should
just remove the [Obsolete] attribute.
@jasonmalinowski jasonmalinowski force-pushed the provide-parse-options-immediately branch from a6e8375 to 29ed5ea Compare June 27, 2022 22:59
@jasonmalinowski jasonmalinowski marked this pull request as ready for review June 27, 2022 23:01
@jasonmalinowski jasonmalinowski requested review from a team as code owners June 27, 2022 23:01

<PropertyGroup>
<CommandLineArgsForDesignTimeEvaluation>-langversion:$(LangVersion)</CommandLineArgsForDesignTimeEvaluation>
<CommandLineArgsForDesignTimeEvaluation Condition="'$(DefineConstants)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -define:$(DefineConstants)</CommandLineArgsForDesignTimeEvaluation>
Copy link
Member Author

Choose a reason for hiding this comment

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

One issue that was identified here was the SDK computes some of its constants during execution, which means this optimization doesn't always work as ideally as we'd like. There's other problems with that and we'll investigate further in what it'd take to do. We have some recently added optimizations around avoiding reparsing if there's only a preprocessor directive change and the file doesn't have any preprocessor directives; having us at least get the langversion over means we have a better chance of that optimization being able to kick in.

tl;dr; this doesn't work in all cases, but that's better than working on no cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, is there any telemetry for when "misses" occur here? something we could look at occasionally and say--oops, we added this option which affects parsing, we can see that we need to update this target.

Copy link
Member Author

@jasonmalinowski jasonmalinowski Jul 11, 2022

Choose a reason for hiding this comment

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

We do have telemetry that we added when we initially thought of doing this:

private static void TryReportCompilationThrownAway(SolutionState solutionState, ProjectId projectId)
My concern isn't the lack of the telemetry, but how we'll know to actually look at it if/when the time comes. I admit I don't have any good ideas there. I guess we could switch that code to actually report faults so it'd bubble up in our fault reporting?

Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

changes under Compilers LGTM

@jasonmalinowski
Copy link
Member Author

@dotnet/roslyn-ide, @dotnet/roslyn-compiler this needs one more review from each team.


<PropertyGroup>
<CommandLineArgsForDesignTimeEvaluation>-langversion:$(LangVersion)</CommandLineArgsForDesignTimeEvaluation>
<CommandLineArgsForDesignTimeEvaluation Condition="'$(FinalDefineConstants)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -define:$(FinalDefineConstants)</CommandLineArgsForDesignTimeEvaluation>
Copy link
Member

Choose a reason for hiding this comment

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

Why are the property names different between C# and VB?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to be a history thing. If you look at the Csc task vs. Vbc task, there's a different input that's given to them. And I see this too:

https://github.com/dotnet/sdk/blob/2693567468cbaf97c85c57e277e2a0609b317e46/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.VisualBasic.targets#L55

Copy link
Member

Choose a reason for hiding this comment

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

Sigh

Copy link
Member Author

Choose a reason for hiding this comment

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

@333fred That's the appropriate spirit when it comes to MSBuild!


<PropertyGroup>
<CommandLineArgsForDesignTimeEvaluation>-langversion:$(LangVersion)</CommandLineArgsForDesignTimeEvaluation>
<CommandLineArgsForDesignTimeEvaluation Condition="'$(FinalDefineConstants)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -define:$(FinalDefineConstants)</CommandLineArgsForDesignTimeEvaluation>
Copy link
Member

Choose a reason for hiding this comment

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

Sigh

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

Successfully merging this pull request may close these issues.

None yet

6 participants