roslyn icon indicating copy to clipboard operation
roslyn copied to clipboard

Using Nullable Disabled semantic model to compute semantic classification

Open maryamariyan opened this issue 1 year ago • 7 comments

Summary:

Using new API (#70609) to get semantic model with nullability analysis disabled

Report

I completed my investigation, what I see is that the changes reduce the overhead on EnsureNullabilityAnalysisPerformedIfNecessary when we are computing semantic classifications

I have 4 cases:

  1. ALWAYS: when nullability analysis enabled ALWAYS
  2. DEFAULT (Using new APIs): when nullability analysis is not set (DEFAULT) - Using new APIs to get Nullable Disabled Semantic Model
  3. NEVER: when nullability analysis is disabled NEVER
  4. DEFAULT (Original Semantic Model): when nullability analysis is not set (DEFAULT) - Without new APIs, using Original Semantic Model

perfview comparison:

Summary: This confirms when we use nullable disabled semantic model for semantic classification the computation of nullability analysis drops and overall semantic classification computation becomes faster. Note that AddSemanticClassifications in the caller list drops from 4% to ~0%

Case # 2 with new APIs (Using nullable disabled semantic model) image

Case # 4 (currently shipped) image

stopwatch comparison:

Summary: This shows an average of 372ms reduced to 132ms when we try to compute semantic classification on a large razor file.

Case # 2 (Using nullable disabled semantic model) image

Case # 4 (currently shipped) image

maryamariyan avatar Nov 30 '23 20:11 maryamariyan

Done with review pass (commit 17)

AlekseyTs avatar Feb 02 '24 15:02 AlekseyTs

It looks like test/CI failures in compiler layer are meaningful. Will have to investigate.

RikkiGibson avatar Feb 15 '24 19:02 RikkiGibson

How should we document the experimental diagnostics? Perhaps we should add a markdown doc similar to https://github.com/dotnet/roslyn/blob/main/docs/Breaking%20API%20Changes.md which lists the "currently active experiments", providing context for each diagnostic ID we are using with [Experimental]?

RikkiGibson avatar Feb 16 '24 00:02 RikkiGibson

@AlekseyTs Changes under Compilers are ready for another review pass.

RikkiGibson avatar Feb 16 '24 01:02 RikkiGibson

#if DEBUG

This change looks suspicious. It looks like AnalyzeBoundNodeNullability doesn't do any validation unless we are in DEBUG, therefore it doesn't feel useful to call it for RELEASE. Therefore, it looks like, instead of modifying this code, we should bail out at the very beginning for nullable disabled semantic model in RELEASE mode. There is simply no point in doing all that work. #Closed


Refers to: src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs:1983 in 95fb86a. [](commit_id = 95fb86a0460c119492a43904864fa07fc76857c5, deletion_comment = True)

AlekseyTs avatar Feb 16 '24 15:02 AlekseyTs

Done with review pass (commit 22)

AlekseyTs avatar Feb 16 '24 15:02 AlekseyTs

Done with review pass (commit 23)

AlekseyTs avatar Feb 17 '24 13:02 AlekseyTs

done with pass.

CyrusNajmabadi avatar Feb 26 '24 20:02 CyrusNajmabadi