dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.56k stars 4.54k forks source link

Options Validation source generator runs primary code generation phase on keypress #93313

Open ericstj opened 9 months ago

ericstj commented 9 months ago

Description

See more detail in https://github.com/dotnet/runtime/issues/92914.

Reproduction Steps

Create a project that uses Options Validation source generator. Observe it's execution pattern - either in the debugger or through ETW.

Expected behavior

Changes unrelated to the options validation code and it's type closure should not trigger regeneration of the options source.

Actual behavior

Every change causes the entire pipeline to rerun.

Regression?

No

Known Workarounds

We haven't had reports of the performance here being a blocker, but that could be due to lack of use. The amount of work done on keypress will depend on whether or not the generator has work to do. If it has a lot of work to do, then it will be doing that work on every change.   Disable the options generator from design-time builds (this will result in errors where the generator is used, which are design time only errors). Workaround:

  <Target Name="_disableOptionsGeneratorInDesignTime" BeforeTargets="ResolveOffByDefaultAnalyzers">
    <ItemGroup Condition="'$(DesignTimeBuild)' == 'true' OR '$(BuildingProject)' != 'true'">
      <OffByDefaultAnalyzer Include="Microsoft.Extensions.Options.SourceGeneration.dll"
                            IsEnabled="$(EnableOptionsGenerator)"/>
    </ItemGroup>
  </Target>

Configuration

No response

Other information

No response

ghost commented 9 months ago

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

Issue Details
### Description See more detail in https://github.com/dotnet/runtime/issues/92914. ### Reproduction Steps Create a project that uses Options Validation source generator. Observe it's execution pattern - either in the debugger or through ETW. ### Expected behavior Changes unrelated to the options validation code and it's type closure should not trigger regeneration of the options source. ### Actual behavior Every change causes the entire pipeline to rerun. ### Regression? No ### Known Workarounds We haven't had reports of the performance here being a blocker, but that could be due to lack of use. The amount of work done on keypress will depend on whether or not the generator has work to do. If it has a lot of work to do, then it will be doing that work on every change.   Disable the options generator from design-time builds (this will result in errors where the generator is used, which are design time only errors). Workaround: ```xml ``` ### Configuration _No response_ ### Other information _No response_
Author: ericstj
Assignees: -
Labels: `untriaged`, `area-Extensions-Options`, `source-generator`
Milestone: -
ericstj commented 8 months ago

It looks like VS noticed the impact of this generator as well, causing 0.21% regression on a typing benchmark for razor pages. When fixing this we should be comparing performance to other well-behaved incremental generators (RegEx, Interop, Json) to ensure we raise enough of the data model to the incremental pipeline so that we don't end up calling the expensive part of our pipeline of every change.

sharwell commented 8 months ago

@ericstj PR #93427 will address (eliminate) the performance overhead for cases where the source generator is not being used. I'm watching this issue as tracking ongoing work to improve the performance specifically in cases where the generator is being used (i.e. produces at least one output).

ericstj commented 8 months ago

Yes, we need to improve that situation. FWIW we have the same class of bug in logging https://github.com/dotnet/runtime/issues/93309 since 6.0 which also needs to be fixed.

As mentioned above the plan is to fix in 9.0 (with possibility of backport) - @sharwell do you know of a reason to prioritize a fix for this sooner than that?

sharwell commented 8 months ago

No, the urgent fix was already submitted.

ericstj commented 8 months ago

Thank you for confirming @sharwell. We will get to this in 9.0 I just wanted to make sure we were on the same page.