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

[release/8.0-rc1] Use Roslyn interceptors feature in binder gen #90835

Merged
merged 6 commits into from
Aug 19, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 18, 2023

Backport of #90340 to release/8.0-rc1

/cc @layomia

Background

Uses the interceptors compiler feature, over the current approach of generating code in the global namespace. This is recommended by the Roslyn team. The request delegate generator has already adopted the approach and we'd like to have consistency. This would also provide good feedback for the compiler team.

Customer Impact (assuming generator is enabled, otherwise none)

If publishing with AOT in a web SDK project, the user should not perceive any change. Otherwise, the user will have to add <Features>$(Features);InterceptorsPreview</Features> to their project. Failing to do so would result in a compiler error if the there are binding calls that trigger generation. The error message will direct the user on how to fix it.

Testing

Automated and manual tests provide coverage of the modified code-paths.

Risk

Moderate but isolated to the modified off-by-default generator. This is a feature-level change, so it's possible that we might have a regression, but we'll continue validating the scenarios for RC 2 and address any customer feedback.

@ghost
Copy link

ghost commented Aug 18, 2023

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

Issue Details

Backport of #90340 to release/8.0-rc1

/cc @layomia

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@carlossanlop carlossanlop requested review from eiriktsarpalis, ericstj and tarekgh and removed request for eiriktsarpalis August 18, 2023 22:23
@dotnet dotnet deleted a comment from carlossanlop Aug 18, 2023
@layomia
Copy link
Contributor

layomia commented Aug 18, 2023

@carlossanlop apologies about your comment; just filled out the template.

@layomia
Copy link
Contributor

layomia commented Aug 18, 2023

I might need to manually adjust the PR pending any issues with adopting the new VS code analysis version.

@carlossanlop
Copy link
Member

@carlossanlop apologies about your comment

😭 What did I do to hurt you.

@lewing lewing added the Servicing-consider Issue for next servicing release review label Aug 18, 2023
@layomia layomia added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 19, 2023
@layomia layomia force-pushed the backport/pr-90340-to-release/8.0-rc1 branch from 74d22e3 to 4596e82 Compare August 19, 2023 01:32
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 19, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 19, 2023
@carlossanlop
Copy link
Member

Chatted with @layomia, this is ready to merge.

@carlossanlop carlossanlop merged commit 034d27f into release/8.0-rc1 Aug 19, 2023
175 checks passed
@carlossanlop carlossanlop deleted the backport/pr-90340-to-release/8.0-rc1 branch August 19, 2023 16:13
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.7.0-3.23314.3" PrivateAssets="all" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="$(MicrosoftCodeAnalysisVersion_LatestVS)" PrivateAssets="all" />
<!-- if MicrosoftCodeAnalysisVersion_LatestVS is still at 4.5.0 (eg: not source build) then update to a newer version. Remove this when we are able to update the value of MicrosoftCodeAnalysisVersion_LatestVS -->
<PackageReference Update="Microsoft.CodeAnalysis.CSharp.Workspaces" Condition="'$(MicrosoftCodeAnalysisVersion_LatestVS)' == '4.5.0'" Version="4.7.0-3.23314.3" />
Copy link
Member

Choose a reason for hiding this comment

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

This isn't going to work for source-build. This value is going to be the current version source-built during the product source-build, therefore this is going to trigger a downgrade and introduce a prebuilt.

Copy link
Member

Choose a reason for hiding this comment

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

My mistake in interpreting what is going to happen here. SB is going to set MicrosoftCodeAnalysisVersion_LatestVS to the current version therefore the conditional setting of the PackageReference to 4.7.0... won't happen.

Copy link
Member

Choose a reason for hiding this comment

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

I think this will work for source-build.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this feature remains in the release, we could port #91006 to RC2 or 1, as needed.

@@ -73,7 +69,11 @@ private sealed record CompilationData

public CompilationData(CSharpCompilation compilation)
{
LanguageVersionIsSupported = compilation.LanguageVersion >= LanguageVersion.CSharp11;
// We don't have a CSharp21 value available yet. Polyfill the value here for forward compat, rather than use the LangugeVersion.Preview enum value.
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean CSharp12?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, will fix if this feature stays in the release.

@@ -127,12 +134,11 @@ void EmitObjectInit(string objExpression, InitializationKind initKind)
}
else if (typeKind is StringParsableTypeKind.Enum)
{
parsedValueExpr = $"ParseEnum<{type.MinimalDisplayString}>({stringValueToParse_Expr}, () => {sectionPathExpr})";
parsedValueExpr = $"ParseEnum<{type.DisplayString}>({stringValueToParse_Expr}, () => {sectionPathExpr})";
Copy link
Member

Choose a reason for hiding this comment

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

When emitting lambdas in generated code consider using fully specified signatures: makes the code less subject to interference and reduces build / IDE analysis time.

}
else
{
string helperMethodDisplayString = GetHelperMethodDisplayString(type.ParseMethodName);
parsedValueExpr = $"{helperMethodDisplayString}({stringValueToParse_Expr}, () => {sectionPathExpr})";
parsedValueExpr = $"{type.ParseMethodName}({stringValueToParse_Expr}, () => {sectionPathExpr})";
Copy link
Member

Choose a reason for hiding this comment

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

When emitting lambdas in generated code consider using fully specified signatures: makes the code less subject to interference and reduces build / IDE analysis time.

}
}

internal sealed record OverloadInterceptorInfo : IEnumerable<KeyValuePair<TypeSpec, List<InterceptorLocationInfo>>>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure that record is buying you anything here. The Dictionary<> type has reference equality semantics and every instance news up a new instance. This means even logically equivalent instances are still going not be .Equals

resolver?.NormalizePath(tree.FilePath, baseFilePath: null) ?? tree.FilePath;
}

internal sealed record ConfigurationBinderInterceptorInfo
Copy link
Member

Choose a reason for hiding this comment

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

The record usage isn't giving you anything here because the fields are not value equatable. Yes they are record types but their implementation uses ref equality.

@@ -1,21 +1,25 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;

namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
{
internal sealed record SourceGenerationSpec
Copy link
Member

Choose a reason for hiding this comment

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

The record usage isn't buying you anything here because all of the fields have ref equality semantics and a new instance is created every time.

@@ -147,7 +149,7 @@ private static bool IsValidRootConfigType(ITypeSymbol? type)
{
// List<string> is used in generated code as a temp holder for formatting
// an error for config properties that don't map to object properties.
_sourceGenSpec.TypeNamespaces.Add("System.Collections.Generic");
_sourceGenSpec.Namespaces.Add("System.Collections.Generic");
Copy link
Member

Choose a reason for hiding this comment

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

The record type InvocationDiagnosticInfo at the start of this file also has broken equality semantics. Can't comment on it directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I have pending 9.0 work to fix this #89587.

@@ -42,9 +41,6 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
context.RegisterSourceOutput(inputData, (spc, source) => Execute(source.Item1, source.Item2, spc));
Copy link
Member

Choose a reason for hiding this comment

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

The equals implementation on BinderInvocation uses ref equality, not value equality. That means the CreateSyntaxProvider call above is never cached. This is an issue for IDE performance.

Copy link
Contributor

@layomia layomia Aug 23, 2023

Choose a reason for hiding this comment

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

I'd say this fact pre-dates this PR, so I'll address outside of it, for RC2/GA if necessary - #83534 (comment).

We've made related perf fixes, with metrics indicating that IDE perf is at par with other generators #90746 #89266 wrt average time spent in the generator, testing with the Roslyn repo as a "large" solution. So I believe we're fine for GA.

Further constraining candidate invocations in the initial shallow check for valid calls to intercept might be impactful & we could pull it into the release if necessary - #90687.

Here's the effort to make the generator fully incremental #89587. I don't think we should try to squeeze it into this release unless it's the only effective way to mitigate user feedback that invalidates these measurements.

Copy link
Member

Choose a reason for hiding this comment

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

with metrics indicating that IDE perf is at par with other generators

Do these benchmarks measure incremental performance?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the most important aspect is to exercise the parsing codepaths multiple times as a result of multiple edits, then yes.

I enabled the generator in all the projects in the Roslyn solution and made repeated edits in a few of them. The perf numbers reflect the aggregate of this activity. The generator parses all invocations but doesn't emit anything (these projects don't do binding). I consider this a sanity-check stress test (i.e in a large solution) that the enabled generator isn't generally detrimental to IDE perf. A more probable instance of this scenario is an AOT'd Web SDK app (where both RDG and config binding generators are enabled by default) that doesn't do any config binding.

The test doesn't cover the current behavior of re-emitting logic in projects that actually perform binding operations. In common cases there are only a few invocations and target config types, and I estimate that emitter logic is far less expensive than the parser's. I'll take measurements to see if we need any improvement for this scenario in RC 2.

Ultimately we've described workarounds for IDE perf issues here #83534 (comment). We can could document them in release notes if needed.

Copy link
Member

Choose a reason for hiding this comment

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

Is this generator alwasy on or only conditionally on?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's conditionally on -- on by default in NativeAOT web sdk apps, otherwise off.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants