Nullability analysis perf investigation during razor semantic classification

[maryamariyan]

Background and Motivation

There is a perf hit during razor semantic classification. We notice that computing classifications for IDE components becomes faster when using the "nullable analysis never" code flow.

Each time we open a razor document for example, we go through computing semantic classification. Screenshots below show that, skipping nullability analysis computes semantic classifications consistently faster than current default.

Perf numbers

Nullable analysis has been bad with perf, we have investigated resolving this for classification. With that we noticed that:

• AddSemanticClassifications in the caller list drops from 4% to ~0%
• an average of 372ms reduced to 132ms when we try to compute semantic classification on a large razor file.

Goal

Sample below illustrates an example where full analysis is needed:

string x = "A";
if (x == null)
{
    x.ToString(); // warn
}

But we found not all cases which need a semantic model require full nullable analysis to be computed for a component.

For example, when testing classification in Roslyn+LSP. This often involves calling into the semantic model over large pieces of the document, specifically through these compiler APIs:

• GetSymbolInfo
• GetTypeInfo

In this scenario, we're seeing a lot of time spent in nullability analysis, but the assumption is that the eventual semantic classification that gets returned would be the same regardless of the nullability analysis being enabled or not.

Potentially the work shown in the call stack below could be thrown away if this shorter path becomes available.

Proposed API

(Based on the prototype in PR: https://github.com/dotnet/roslyn/pull/71036)

namespace Microsoft.CodeAnalysis
{
    public abstract class SemanticModel
    {
+        public virtual bool DisableNullableAnalysis { get; }
    }

    public class Document : TextDocument
    {
        public bool TryGetSemanticModel([NotNullWhen(returnValue: true)] out SemanticModel? semanticModel) { }
+        public bool TryGetNullableDisabledSemanticModel([NotNullWhen(returnValue: true)] out SemanticModel? semanticModel) { }
        public async Task<SemanticModel?> GetSemanticModelAsync(CancellationToken cancellationToken = default) { }
+        public async Task<SemanticModel?> GetNullableDisabledSemanticModelAsync(CancellationToken cancellationToken = default) { }
    }
}

namespace Microsoft.CodeAnalysis.CSharp
{
    public sealed partial class Compilation
    {
-        public SemanticModel GetSemanticModel(SyntaxTree syntaxTree, bool ignoreAccessibility = false) { }
+        public SemanticModel GetSemanticModel(SyntaxTree syntaxTree, bool ignoreAccessibility) { }
+        public SemanticModel GetSemanticModel(SyntaxTree syntaxTree, bool ignoreAccessibility = false, bool disableNullableAnalysis = false)
         { }
    }

    public sealed partial class CSharpCompilation : Compilation
    {
-        public new SemanticModel GetSemanticModel(SyntaxTree syntaxTree, bool ignoreAccessibility)
+        public new SemanticModel GetSemanticModel(SyntaxTree syntaxTree, bool ignoreAccessibility, bool disableNullableAnalysis)
    }
}

Usage Examples

Something like

compilation.GetSemanticModel(syntaxTree, disableNullableAnalysis: true);

or

if (!document.TryGetNullableDisabledSemanticModel(out var semanticModel))
{
    var semanticModel = await document.GetNullableDisabledSemanticModelAsync(cancellationToken).ConfigureAwait(false);
}

Alternative Designs

• Rather than passing down two flags: , bool ignoreAccessibility = false, bool disableNullableAnalysis = false to GetSemanticModel APIs, we could introduce SemanticModelOptions to use as argument for GetSemanticModel:

namespace Microsoft.CodeAnalysis.CSharp
{
    public sealed partial class Compilation
    {
         public SemanticModel GetSemanticModel(SyntaxTree syntaxTree, bool ignoreAccessibility = false) { }
+        public SemanticModel GetSemanticModel(SyntaxTree syntaxTree, SemanticModelOptions semanticModelOptions) { }
         { }
    }

    public sealed partial class CSharpCompilation : Compilation
    {
         public new SemanticModel GetSemanticModel(SyntaxTree syntaxTree, bool ignoreAccessibility)
+        public new SemanticModel GetSemanticModel(SyntaxTree syntaxTree, SemanticModelOptions semanticModelOptions)
    }
}

Risks

• We are pursuing an API that doesn't do sharing of bound nodes between the original and nullable disabled semantic model caches. (It is possible that we are not gaining perf benefits by not sharing this but we are okay with this approach at the moment).

No risk.

Further discussions:

• Can/should flow analysis be in any disabled for "subsections" of methods? (already discussed in meeting, answer is NO)
• Does this API accommodate the hypothetical completion use case?

Razor experiment:

(click to view)

To validate our assumptions, we disabled nullability analysis (by adding `run-nullable-analysis=never` to the Features PropertyGroup of the document's csproj) and retested. When nullability is enabled, we collected traces and saw the average cost for compiler would be much higher for the above APIs when the feature is enabled. We're hoping this experiment motivates potential perf improvements in this area. Two possible routes would be: - Compiler just invests in making nullability performance better. There would be a direct win for the IDE for any changes there. - If it's hard to squeeze out performance there, perhaps we could consider a way for clients to call in, asking to bypass nullability analysis (in effect, going back to the prior system Roslyn had, where it could bind tiny pieces of the file without having to do a full pass over the entire method). We are open to multiple approaches, would be good to collaborate on this, especially to help validate results and provide useful data to help drive this process. ### When nullability analysis is disabled Please note the duration recorded in the screenshots below: - Each duration triggered upon reopening of a large razor document. - Computes timing for C# semantic classification - Average duration difference of 587 ms vs. 387 ms between when nullability analysis disabled/enabled [a] `run-nullable-analysis=always` **(DEFAULT BEHAVIOR TODAY)**  [c] `run-nullable-analysis=never` 

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[Youssef1313]

Very closely related to https://github.com/dotnet/roslyn/issues/68947.

I have always wanted a way to skip nullability analysis when doing semantic model calls, but in my case it's to improve source generators performance.

[333fred]

API Review

• How complex will it be to implement with any amount of sharing?
  • Could it just be a wrapper around an internal version of the proposed APIs?
• Today, we have an invariant that semantic models are independent. Should this be that relationship, or is closer to the speculative semantic model relationship?
  • It would mean that the nullable-disabled semantic model could affect the caches of the nullable enabled one. This could be undesirable from an API perspective?
• Consumption side, having different results based on order of operations; ie, don't maybe give nullable info from the disabled version
  • Likely to result in IDE bugs that will be hard to track down.
• Compiler team needs to sit down and work through the feasibility of sharing, and whether we're ok with such a relationship, before we move forward with the API.

[maryamariyan]

The most recent perf investigations for this API is summarized in PR https://github.com/dotnet/roslyn/pull/71036

Based on that we should be able to get answers to questions above listed in https://github.com/dotnet/roslyn/issues/70609#issuecomment-1815536958 and mark this issue back to ready for review.

/cc @RikkiGibson

[333fred]

API Review

• If we don't intend to do sharing, the API design should probably not imply that sharing can occur, which is why it's structured as it is.
• Do we have data demonstrating that we will benefit from sharing?
  • Should we instead investigate doing sharing across all semantic models?
    • May be less brittle in the long term?
• Can we lean on ExperimentalAttribute so we can do some A/B testing and see larger trends?
• We prefer the SemanticModelOptions approach to avoid a continuously-evolving list of flags

Approved in experiment: We'll approve the alternative version of this proposal in experiment for now, and revisit the final design when we have real-world A/B data.