Using Nullable Disabled semantic model to compute semantic classification #71036
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
ghost
added
the
Community
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs
Outdated
Show resolved
Hide resolved
3 tasks
maryamariyan
force-pushed
the
dev/maryamariyan/perf-analysis-semtokens
branch
from
f028dbf
to
47ad5ce
Compare
maryamariyan
changed the title
| @@ -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.
Should the PROTOTYPE comments in this PR be addressed?
There was a problem hiding this comment.
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.
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
RikkiGibson
reviewed
Feb 23, 2024
…-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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
EnsureNullabilityAnalysisPerformedIfNecessarywhen 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
AddSemanticClassificationsin 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)
