Disable CodeStyle analyzers by default in all explicit builds of Roslyn.sln (i.e. csc/vbc invocations both inside Visual Studio and from command line prompt) by mavasani · Pull Request #43303 · dotnet/roslyn

mavasani · 2020-04-13T12:47:27Z

Workaround for #42166

This change brings down the local build time for Roslyn.sln on my machine from around 8 mins to 5 mins. I am going to file separate bugs with performance traces for slow IDE analyzers in CodeStyle NuGet package.

Workaround for dotnet#42166

eng/targets/Imports.targets

jaredpar · 2020-04-13T18:53:57Z

What is the motivation for doing this? Today the code style analyzers only run as part of our build correctness leg. They are disabled for all other legs. The motivation behind that split is exactly what you're laying out here:

Keep non-style legs fast as possible
Have a single style / build correctness leg that does all of this heavy build enforcemen

jaredpar · 2020-04-13T18:54:17Z

Basically if we do this how will code style get verified as a part of our CI process?

@sharwell

mavasani · 2020-04-13T18:55:09Z

What is the motivation for doing this?

Faster local builds for each developer.

jaredpar · 2020-04-13T18:56:41Z

The PR says

Disable CodeStyle analyzers by default in CI for Roslyn.sln

CI refers to continuous integration aka AzDO. This does not generally refer to developer inner loop.s

jaredpar · 2020-04-13T18:58:00Z

The description of the PR, the implementation and the discussion here all disagree on what this PR is doing.

mavasani · 2020-04-13T18:58:41Z

CI refers to continuous integration aka AzDO. This does not generally refer to developer inner loop.s

Fixed now, I meant to state disable by default for command line builds

jaredpar · 2020-04-13T18:59:18Z

eng/targets/Imports.targets

+
+  <!-- Workaround for https://github.com/dotnet/roslyn/issues/42166 -->
+  <!-- PERF: This property group prevents the analyzers from CodeStyle NuGet package from being executed by default in CI -->
+  <PropertyGroup Condition="'$(DesignTimeBuild)' != 'true'">


If we want this to be enabled for continuous integration only then we should use '$(ContinuousIntegration)' != 'true'. That is the standard way of conditioning data on CI.

That would also disable all these analyzers for design time builds/IDE live analysis, we do not want that.

Who is "we" here? This is definitely what a good portion of the compiler team wants for exactly what you laid out in this email. I want my inner loop fast including building from Visual Studio.

design time build == live analysis during typing. If we make your suggested change, we will not execute these analyzers even during typing and see no suggestions.

jaredpar · 2020-04-13T19:01:50Z

Fixed now, I meant to state disable by default for command line builds

Why command line builds here and not Visual Studio? The biggest source of complaints I've seen about code style build time is in Visual Studio, not the command line. In the command line if you don't care about code style then just use -skipAnaylzers. It's been there for a long time specifically for this case.

jaredpar

This is impacting how the overall build of Roslyn works and it hasn't really been discussed with the team. The biggest complaint we've had internally about build perf time is around building from Visual Studio, not from the command line.

mavasani · 2020-04-13T19:08:08Z

Why command line builds here and not Visual Studio? The biggest source of complaints I've seen about code style build time is in Visual Studio, not the command line. In the command line if you don't care about code style then just use -skipAnaylzers. It's been there for a long time specifically for this case.

There are about 70+ code style analyzers in this NuGet package, all of them reporting hidden diagnostics by default. We have only enforced 2 analyzers from this NuGet package as warnings in build (formatting and file header analyzer), but due to #42166, all of these 70+ analyzers are running on every command line build of Roslyn on every developer's machine, adding to build time overhead. Except for the explicitly enforced analyzers (current the two mentioned above), execution of rest of code style analyzers is just an unnecessary build time overhead - hidden diagnostics are never reported on command line build. This change aims at removing that overhead from command line builds.

jaredpar · 2020-04-13T19:21:10Z

@mavasani

but due to #42166, all of these 70+ analyzers are running on every command line build of Roslyn on every developer's machine, adding to build time overhead.

Who is complaining about this? This is the crux of my confusion here. For developers who build from the command line there is already a solution for exactly this: -skipAnalyzers.

This is frustrating because we have several active threads going on right now about the usability of Visual Studio given the negative performance of our analyzers. Developers who are actively blocked in their work. Yet instead of addressing that (build from VS) we're sending PRs to disable build from command line (which I haven't seen anyone complain about).

Am I missing the complaints?
Do we have a story for fixing the VS build slow down?

mavasani · 2020-04-13T19:33:09Z

Do we have a story for fixing the VS build slow down?

I think the confusion is in the wordings that I used in this PR. Sorry, let me try to clarify:

"Command line build": I meant an explicit built, both those done inside VS and from a command line prompt - basically, all execution of analyzers from csc.exe/vbc.exe.
"Design time build": This is the background build that happens in VS while you are typing, and analyzers need to execute in here to get code style suggestions for active file.

eng/targets/Imports.targets

jaredpar · 2020-04-13T22:37:56Z

eng/targets/Imports.targets

+  <!-- Workaround for https://github.com/dotnet/roslyn/issues/42166 -->
+  <!-- PERF: This property group prevents the analyzers from CodeStyle NuGet package from being executed by default in all explicit builds (i.e. csc/vbc invocations both inside Visual Studio and from command line prompt) -->
+  <PropertyGroup Condition="'$(DesignTimeBuild)' != 'true'">
+    <NoWarn>$(NoWarn);IDE0004;IDE0004WithoutSuggestion;IDE0005;IDE0007;IDE0007WithoutSuggestion;IDE0008;IDE0008WithoutSuggestion;IDE0009;IDE0009WithoutSuggestion;IDE0010;IDE0010WithoutSuggestion;IDE0011;IDE0011WithoutSuggestion;IDE0016;IDE0016WithoutSuggestion;IDE0017;IDE0017WithoutSuggestion;IDE0018;IDE0018WithoutSuggestion;IDE0019;IDE0019WithoutSuggestion;IDE0020;IDE0020WithoutSuggestion;IDE0021;IDE0022;IDE0023;IDE0024;IDE0025;IDE0026;IDE0027;IDE0028;IDE0028WithoutSuggestion;IDE0029;IDE0029WithoutSuggestion;IDE0030;IDE0030WithoutSuggestion;IDE0031;IDE0031WithoutSuggestion;IDE0032;IDE0032WithoutSuggestion;IDE0033;IDE0033WithoutSuggestion;IDE0034;IDE0034WithoutSuggestion;IDE0035;IDE0035WithoutSuggestion;IDE0036;IDE0036WithoutSuggestion;IDE0037;IDE0037WithoutSuggestion;IDE0040;IDE0040WithoutSuggestion;IDE0041;IDE0041WithoutSuggestion;IDE0042;IDE0042WithoutSuggestion;IDE0043;IDE0044;IDE0044WithoutSuggestion;IDE0045;IDE0045WithoutSuggestion;IDE0046;IDE0046WithoutSuggestion;IDE0047;IDE0048;IDE0048WithoutSuggestion;IDE0050;IDE0050WithoutSuggestion;IDE0051;IDE0052;IDE0054;IDE0054WithoutSuggestion;IDE0055;IDE0056;IDE0056WithoutSuggestion;IDE0057;IDE0057WithoutSuggestion;IDE0058;IDE0059;IDE0060;IDE0061;IDE0062;IDE0062WithoutSuggestion;IDE0063;IDE0063WithoutSuggestion;IDE0064;IDE0065;IDE0066;IDE0066WithoutSuggestion;IDE0070;IDE0070WithoutSuggestion;IDE0071;IDE0071WithoutSuggestion;IDE0072;IDE0072WithoutSuggestion;IDE0073;IDE0073WithoutSuggestion;IDE0074;IDE0074WithoutSuggestion;IDE0075;IDE0075WithoutSuggestion;IDE1005;IDE1005WithoutSuggestion;IDE1006;IDE1006WithoutSuggestion</NoWarn>


How does adding these to $(NoWarn) disable the diagnostic?

If all diagnostics reported by an analyzer are disabled (via nowarn or ruleset) then the analyzer is completely skipped for execution.

How does this interact with the editor config change which bumps them all back to warning?

We currently enforce only 2 of these as warnings in Roslyn's editorconfig (file header and formatting analyzer). Editorconfig based settings always take precedence over compilation options (nowarn and ruleset settings). So those 2 analyzers will run everywhere, and rest will only run while typing in the IDE.

mavasani · 2020-04-14T22:21:05Z

Thanks!

Disable CodeStyle analyzers by default in CI for Roslyn.sln

0b020e4

Workaround for dotnet#42166

mavasani added the Area-Infrastructure label Apr 13, 2020

mavasani requested review from jaredpar and sharwell April 13, 2020 12:47

mavasani requested a review from a team as a code owner April 13, 2020 12:47

mavasani commented Apr 13, 2020

View reviewed changes

eng/targets/Imports.targets Outdated Show resolved Hide resolved

mavasani mentioned this pull request Apr 13, 2020

[Perf] RemoveUnnecessaryCast analyzer (IDE0004) #43304

Closed

mavasani added 2 commits April 13, 2020 06:31

Explicitly enforce IDE0055 as a warning in .editorconfig

0831588

Address feedback - skip non-configurable diagnostics

a1fadec

JoeRobich approved these changes Apr 13, 2020

View reviewed changes

mavasani changed the title ~~Disable CodeStyle analyzers by default in CI for Roslyn.sln~~ Disable CodeStyle analyzers by default in command line builds for Roslyn.sln Apr 13, 2020

jaredpar reviewed Apr 13, 2020

View reviewed changes

jaredpar requested changes Apr 13, 2020

View reviewed changes

mavasani commented Apr 13, 2020

View reviewed changes

eng/targets/Imports.targets Outdated Show resolved Hide resolved

Update eng/targets/Imports.targets

0401dd3

mavasani changed the title ~~Disable CodeStyle analyzers by default in command line builds for Roslyn.sln~~ Disable CodeStyle analyzers by default in all explicit builds of Roslyn.sln (i.e. csc/vbc invocations both inside Visual Studio and from command line prompt) Apr 13, 2020

jaredpar reviewed Apr 13, 2020

View reviewed changes

jaredpar approved these changes Apr 14, 2020

View reviewed changes

mavasani merged commit f5a967c into dotnet:master Apr 14, 2020

ghost added this to the Next milestone Apr 14, 2020

mavasani deleted the DisableCodeStyleAnalyzersByDefaultInCI branch April 14, 2020 22:21

sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020

Conversation

mavasani commented Apr 13, 2020

Uh oh!

Uh oh!

jaredpar commented Apr 13, 2020

Uh oh!

jaredpar commented Apr 13, 2020

Uh oh!

mavasani commented Apr 13, 2020

Uh oh!

jaredpar commented Apr 13, 2020

Uh oh!

jaredpar commented Apr 13, 2020

Uh oh!

mavasani commented Apr 13, 2020

Uh oh!

jaredpar Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Apr 13, 2020

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

mavasani commented Apr 13, 2020

Uh oh!

jaredpar commented Apr 13, 2020

Uh oh!

mavasani commented Apr 13, 2020

Uh oh!

Uh oh!

jaredpar Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

jaredpar Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani commented Apr 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mavasani Apr 13, 2020 •

edited

Loading