-
Notifications
You must be signed in to change notification settings - Fork 506
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
Update AnalyzersExtensions and SettingsHelper to use cached JsonValue objects where possible #3642
Update AnalyzersExtensions and SettingsHelper to use cached JsonValue objects where possible #3642
Conversation
… objects where possible DotNetAnalyzers#3634
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3642 +/- ##
========================================
Coverage 93.26% 93.26%
========================================
Files 1079 1079
Lines 113737 113859 +122
Branches 4027 4030 +3
========================================
+ Hits 106072 106186 +114
- Misses 6630 6636 +6
- Partials 1035 1037 +2 |
Seems to also be a partial fix for the SA1121 slowness reported in #2975. |
Do you have time to review this, @sharwell? I would also like to cache complete StyleCopSettings objects, since merging the json content with the analyzer options takes considerable time as well, but I am holding that off until this one is merged to make it a bit easier to review. |
@@ -23,72 +24,19 @@ internal static class AnalyzerExtensions | |||
/// </summary> | |||
/// <param name="context">The analysis context.</param> | |||
/// <param name="action">Action to be executed at completion of parsing of a document.</param> | |||
public static void RegisterSyntaxTreeAction(this AnalysisContext context, Action<SyntaxTreeAnalysisContext, StyleCopSettings> action) |
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.
📝 So this is a bit of a strange situation. The AnalysisContext.TryGetValue<TValue>
method is effectively static - it doesn't access any state of AnalysisContext
. In theory, you could do something like this:
delegate bool TryGetStyleCopSettingsFileJson(SourceText text, SourceTextValueProvider<Lazy<JsonValue>> valueProvider, out Lazy<JsonValue> value);
// This delegate can be captured and used in the callback
TryGetStyleCopSettingsFileJson tryGetValue = context.TryGetValue<Lazy<JsonValue>>;
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.
For cases where more than one callback is registered, it does seem like it would be more efficient to use an explicit start callback and only compute it once.
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.
The big problem right now is re-creating the settings object over and over. Holding on to these two comments for later, when most of the work is done.
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.
For cases where more than one callback is registered, it does seem like it would be more efficient to use an explicit start callback and only compute it once.
Handled in #3648
|
||
private static Lazy<JsonValue> ParseJson(SourceText text) | ||
{ | ||
return new Lazy<JsonValue>(() => JsonReader.Parse(text.ToString())); |
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.
💡 Would be easy to use SourceTextReader
to avoid the call to ToString()
here
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.
Absolutely. Will do.
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.
Handled in #3648
var settingsFile = GetSettingsFile(context.Options, GetJsonValue, cancellationToken); | ||
return GetSettings(context.Options, tree, settingsFile, failureBehavior); | ||
|
||
Lazy<JsonValue> GetJsonValue(SourceText settingsText) |
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.
💡 It would be better to make this a static
lambda in the call to GetSettingsFile
so it can cache the delegate
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.
You lost me :-) This local function captures the context. I hadn't heard of static lambdas before, but it seems the context makes it impossible for it to be static. I am probably misunderstanding you (or static lambdas). Could you elaborate, @sharwell?
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.
Suppose you need to capture a local variable, like this:
int specialValue = 5;
InvokeHelper(() => specialValue);
// helper
int InvokeHelper(Func<int> calculateValue)
=> -calculateValue();
You can avoid the delegate allocation by making a generic state argument, and then making the lambda static
:
int specialValue = 5;
InvokeHelper(static specialValue => specialValue, specialValue);
// helper
int InvokeHelper<TArg>(Func<TArg, int> calculateValue, TArg arg)
=> -calculateValue(arg);
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.
Ah, got it! So if I change GetSettingsFile to accept a generic state/context parameter the way you suggest, would there be any difference between a static lambda and making the existing local function static? I just feel that a local function is better for readability.
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.
I'm not sure. We'd need to look at the IL output from the compiler to verify.
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.
I am not fluent in IL, but it looks like a branch around the allocation if it has already been performed. Similar code for both static cases.
https://sharplab.io/#v2:EYLgtghglgdgNAFxFANnAJiA1AHwAIBMAjALABQeAzAASHUDC1A3udW7TXgCzUBiA9vwAUsBNQAeASmqt2LMu0XUA+tQC81ACLDxcWkQBs1AJ7qAfCckBuWWwC+tmQvZVaPAEIQATiJhipTorySuyqGtpCutQAMtaOivEuhtSiMb5ixtJqFsY2zvaOjq4AKlr8ADzFZkKlAG4QKACuAKZ6eACslXpV1BAAxghQ/DCSjsEhtADsvQNDMEL1Tc1x+dQOZHZAA=
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.
Handled in #3648
{ | ||
return GetSettingsFile(context.Options, GetJsonValue, cancellationToken); | ||
|
||
Lazy<JsonValue> GetJsonValue(SourceText settingsText) |
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.
💡 It would be better to make this a static
lambda in the call to GetSettingsFile
so it can cache the delegate
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.
Handled in #3648
@bjornhellander The suggestions above can be implemented as part of the follow-up work |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [StyleCop.Analyzers](https://github.com/DotNetAnalyzers/StyleCopAnalyzers) | nuget | patch | `1.2.0-beta.435` -> `1.2.0-beta.507` | --- ### Release Notes <details> <summary>DotNetAnalyzers/StyleCopAnalyzers</summary> ### [`v1.2.0-beta.507`](https://github.com/DotNetAnalyzers/StyleCopAnalyzers/releases/tag/1.2.0-beta.507) [Compare Source](https://github.com/DotNetAnalyzers/StyleCopAnalyzers/compare/1.2.0-beta.435...1.2.0-beta.507) #### What's Changed - Update to StyleCop.Analyzers 1.2.0-beta.435 by [@​sharwell](https://github.com/sharwell) in [DotNetAnalyzers/StyleCopAnalyzers#3499 - Add c# 11 test project to opencover-report.ps1 by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3506 - Use GetText instead of ToFullString by [@​sharwell](https://github.com/sharwell) in [DotNetAnalyzers/StyleCopAnalyzers#3514 - Keep tracked nodes in a list by [@​sharwell](https://github.com/sharwell) in [DotNetAnalyzers/StyleCopAnalyzers#3525 - Remove unnecessary nullable directives by [@​sharwell](https://github.com/sharwell) in [DotNetAnalyzers/StyleCopAnalyzers#3530 - Remove hard-coded language versions in test projects for c# 8, 9 and 10 by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3528 - Update SA1515 to not let one range of trivia affect another by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3529 - Mentioned VS 2022 by [@​twojnarowski](https://github.com/twojnarowski) in [DotNetAnalyzers/StyleCopAnalyzers#3549 - Remove byte order mark from schema file by [@​martincostello](https://github.com/martincostello) in [DotNetAnalyzers/StyleCopAnalyzers#3562 - Update SA1012 to expect no space between a property pattern's opening brace and an enclosing list pattern's opening bracket by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3511 - Update Microsoft.CodeAnalysis.CSharp.Workspaces to version 4.4.0 for the c# 11 test project by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3580 - Update SA1008 to handle positional patterns inside property patterns by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3579 - Update SA1000 to trigger after keywords is, or, and, not by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3585 - Update SA1000.md by [@​Youssef1313](https://github.com/Youssef1313) in [DotNetAnalyzers/StyleCopAnalyzers#3563 - Update SA1313 to also allow incorrect names in explicitly implemented methods from interfaces by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3569 - Update SA1023 to not trigger first in line, inside a foreach without braces by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3543 - Update SA1400 to recognize access modifier "file" by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3590 - Update SA1206 to recognize modifier "file" by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3591 - Update SA1000 to handle checked operator declarations correctly by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3505 - Update SA1402 to handle records and record structs by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3570 - Bump Newtonsoft.Json from 12.0.3 to 13.0.2 in /StyleCop.Analyzers/StyleCop.Analyzers.Status.Generator by [@​dependabot](https://github.com/dependabot) in [DotNetAnalyzers/StyleCopAnalyzers#3584 - Update to the latest version of the testing library by [@​sharwell](https://github.com/sharwell) in [DotNetAnalyzers/StyleCopAnalyzers#3601 - Update so that SA1600 tests will be run with the expected language version in test projects for c# 8 and above by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3614 - Update reading of file_header_template and stylecop.documentation.copyrightText to allow multiple lines by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3617 - Update SA1015 to require trailing space after an explicit generic return type in a lambda expression by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3625 - Update to Microsoft.CodeAnalysis.Analyzers 3.3.5-beta1.23205.2 by [@​sharwell](https://github.com/sharwell) in [DotNetAnalyzers/StyleCopAnalyzers#3628 - Update SA1206 to handle c# 11 modifier "required" by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3535 - Preparations for SettingsHelper optimizations by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3635 - Correct SA1515 to not fire on the second line of a file header by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3633 - Update AnalyzersExtensions and SettingsHelper to use cached JsonValue objects where possible by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3642 - Update SA1010 to not trigger on list patterns by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3507 - Update NamingSettings and DocumentationSettings to keep one Regex instance instead of calling Regex.IsMatch by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3639 - Use ResxSourceGenerator for resource generation by [@​sharwell](https://github.com/sharwell) in [DotNetAnalyzers/StyleCopAnalyzers#3343 - Make XmlCommentHelper faster by [@​ninedan](https://github.com/ninedan) in [DotNetAnalyzers/StyleCopAnalyzers#3651 - Update RenameToUpperCaseCodeFixProvider to not offer a code fix if the identifier only consists of underscores by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3637 - Don't emit SA1414 for interface implementations by [@​CollinAlpert](https://github.com/CollinAlpert) in [DotNetAnalyzers/StyleCopAnalyzers#3644 - Support file-scoped namespaces in SA1516 by [@​JakubLinhart](https://github.com/JakubLinhart) in [DotNetAnalyzers/StyleCopAnalyzers#3513 - Update SA1137 to also consider init accessors by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3669 - Update SA1500 to also consider init accessors by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3670 - Update SA1513 to not trigger before an init accessor by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3666 - Update SA1212 to also trigger for an init accessor before a getter by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3661 - Update SA1513 codefix to use the existing newline character sequence by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3607 - Correct code fix for SA1130 when delegate expression is part of a cast expression by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3516 - Update so that c# 7 tests will be run with the expected language version in test projects for c# 8 and above by [@​bjornhellander](https://github.com/bjornhellander) in [DotNetAnalyzers/StyleCopAnalyzers#3616 - SA1629 should allow full-sentence links instead of forcing the period to glow white by [@​jnm2](https://github.com/jnm2) in [DotNetAnalyzers/StyleCopAnalyzers#3371 #### New Contributors - [@​twojnarowski](https://github.com/twojnarowski) made their first contribution in [DotNetAnalyzers/StyleCopAnalyzers#3549 - [@​ninedan](https://github.com/ninedan) made their first contribution in [DotNetAnalyzers/StyleCopAnalyzers#3651 - [@​CollinAlpert](https://github.com/CollinAlpert) made their first contribution in [DotNetAnalyzers/StyleCopAnalyzers#3644 - [@​JakubLinhart](https://github.com/JakubLinhart) made their first contribution in [DotNetAnalyzers/StyleCopAnalyzers#3513 **Full Changelog**: DotNetAnalyzers/StyleCopAnalyzers@1.2.0-beta.435...1.2.0-beta.507 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 4am" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/ThorstenSauter/NoPlan). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMzEuMCIsInVwZGF0ZWRJblZlciI6IjM1LjEzMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Thorsten Sauter <Thorsten.Sauter@gmail.com>
Partial fix for #3634
The primary change is to use the SourceTextValueProvider cache in CompilationStartAnalysisContext to reduce the number of times the settings file is parsed. Code fix providers and some tests still use overloads of GetStyleCopSettings which does not use the cache.