-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Using Nullable Disabled semantic model to compute semantic classification #71036
Using Nullable Disabled semantic model to compute semantic classification #71036
Conversation
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs
Outdated
Show resolved
Hide resolved
f028dbf
to
47ad5ce
Compare
@@ -71,8 +71,8 @@ dotnet_diagnostic.RS0006.severity = error | |||
dotnet_diagnostic.RS0012.severity = warning | |||
dotnet_diagnostic.RS0014.severity = warning | |||
dotnet_diagnostic.RS0015.severity = warning | |||
dotnet_diagnostic.RS0016.severity = error | |||
dotnet_diagnostic.RS0017.severity = error | |||
dotnet_diagnostic.RS0016.severity = none # PROTOTYPE(ndsm): re-enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the PROTOTYPE comments in this PR be addressed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's from @RikkiGibson 's commit. I would have to refer back to him to see how this needs to be updated.
Refer to commit d6262f6 in this PR
Also refer to this pipeline run:
- Seems like the roslyn-CI (Correctness Correctness_TodoCheck) task also caught this PROTOTYPE comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First step I think is to present the findings to API review. They may want revisions to names, shapes, etc. of the API but any changes should have pretty small impact on this PR.
My commits haven't yet been reviewed by compiler team so we will need 2 compiler team reviews on the PR as well. I would be fine with just pushing commits to this PR to address any feedback.
This specific prototype comment will be addressed by re-enabling the error and updating the public API files.
…thub.com/maryamariyan/roslyn into dev/maryamariyan/perf-analysis-semtokens
…-analysis-semtokens
- This commit would be sufficient if decide to not do AB/testing
- Tested locally using WorkspaceConfigurationOptionsStorage to set feature flag - Changes here may be used if we want to do A/B testing Note: I commented out `globalOptions.GetOption(DisableNullableAnalysisInClassification)` and sets to WorkspaceConfigurationOptions.DisableNullableAnalysisInClassification property to true directly in my tests.
- Tested locally using ClassificationOptions to set feature flag - Changes here may be used if we want to do A/B testing Note: I commented out `globalOptions.GetOption(DisableNullableAnalysisInClassification, language)` and set ClassificationOptions.DisableNullableAnalysisInClassification property to true directly while testing this change locally.. Use ClassificationOptions to set feature flag
…allow for AB testing TODO - [ ] Confirm if AB testing is needed. If not, then I can revert back to 2c79db4 - [ ] Otherwise, test with global options, using this commit (I only tested previous commits locally)
System.Composition.Hosting.CompositionFailedException : Exported contract type 'IWorkspaceService' is not assignable from part 'DefaultClassificationConfigurationService'.
- Removes Feature flag setup - Reverts content back to 2c79db4
...ces/Core/Portable/Classification/SyntaxClassification/AbstractSyntaxClassificationService.cs
Outdated
Show resolved
Hide resolved
...ces/Core/Portable/Classification/SyntaxClassification/AbstractSyntaxClassificationService.cs
Outdated
Show resolved
Hide resolved
done with pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ide side LGTM.
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 classificationsI have 4 cases:
ALWAYS
: when nullability analysis enabled ALWAYSDEFAULT
(Using new APIs): when nullability analysis is not set (DEFAULT) - Using new APIs to get Nullable Disabled Semantic ModelNEVER
: when nullability analysis is disabled NEVERDEFAULT
(Original Semantic Model): when nullability analysis is not set (DEFAULT) - Without new APIs, using Original Semantic Modelperfview 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)
Case # 4 (currently shipped)
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)
Case # 4 (currently shipped)