Cache the ApiData values we produce for an AdditionalText file. #6556

CyrusNajmabadi · 2023-03-29T23:26:37Z

Typing in roslyn shows this to be the highest allocation hitter. This is because diagnostics will end up calling into this analyzer, which then rereads teh SourceText for these additional files, over and over again.

For example, this was typing just for just a few seconds:

CyrusNajmabadi · 2023-03-29T23:27:17Z

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs

@@ -147,83 +169,110 @@ static void RegisterImplActions(CompilationStartAnalysisContext compilationConte
            }
        }

-        private static ApiData ReadApiData(List<(string path, SourceText sourceText)> data, bool isShippedApi)
+        private static ApiData ReadApiData(string path, SourceText sourceText, bool isShippedApi)


this now computes teh ApiData for a single file. handling of multiple files is taken care of by the caller.

CyrusNajmabadi · 2023-03-30T00:12:17Z

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.Impl.cs

@@ -20,21 +20,24 @@ namespace Microsoft.CodeAnalysis.PublicApiAnalyzers
 {
    public partial class DeclarePublicApiAnalyzer : DiagnosticAnalyzer
    {
+        private sealed record AdditionalFileInfo(string Path, SourceText SourceText, bool IsShippedApi);


common data shared among all ApiLine instances.

CyrusNajmabadi · 2023-03-30T00:18:00Z

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.Impl.cs

-        private sealed class ApiLine
+        private sealed record AdditionalFileInfo(string Path, SourceText SourceText, bool IsShippedApi);
+
+        private readonly record struct ApiLine


made into a simple struct, which also has a pointer to the common data stored across all lines from teh same file.

codecov · 2023-03-30T01:10:05Z

Codecov Report

Merging #6556 (ba8f956) into main (b84dbd4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6556   +/-   ##
=======================================
  Coverage   96.43%   96.43%           
=======================================
  Files        1372     1372           
  Lines      320278   320264   -14     
  Branches    10295    10293    -2     
=======================================
- Hits       308848   308839    -9     
+ Misses       8975     8971    -4     
+ Partials     2455     2454    -1

ToddGrun · 2023-03-30T01:40:33Z

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.Impl.cs

-            }
+            public SourceText SourceText => FileInfo.SourceText;
+            public string Path => FileInfo.Path;
+            public bool IsShippedApi => FileInfo?.IsShippedApi is true;


FileInfo?

why null check?

It's a struct, and used in an fashion where it can be in the default state.

So should the other ones be checked then too?

ToddGrun · 2023-03-30T01:47:03Z

        void CheckAndRegisterImplementation(bool isPublic)

could this be made static if you passed in the compilationContext?

Refers to: src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs:115 in a86ecec. [](commit_id = a86ecec, deletion_comment = False)

ToddGrun · 2023-03-30T01:50:16Z

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs

-                        maxNullableRank = Math.Max(rank, maxNullableRank);
-                        continue;
-                    }
+                lineNumber++;


lineNumber++;

I assume it's intentional that this doesn't count ws only / empty lines, but it seems odd.

yup yup. im preserving semantics here

Maybe @sharwell can verify it's the intended behavior, it definitely doesn't match what the variable name indicates to me.

my preference is to not change this. this was the behavior from before. If we change it, we risk breaking things for some customer who does have blank lines and then the nullable directive :)

ToddGrun · 2023-03-30T01:51:47Z

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs

-            foreach (var (path, sourceText) in data)
-            {
-                int rank = -1;
+            // current line we're on.  Note: we ignore whitespace lines when computin gthis.


computin gthis

nit: typo

ToddGrun · 2023-03-30T02:09:34Z

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs

        {
-            if (!TryGetApiText(analyzerOptions.AdditionalFiles, isPublic, cancellationToken, out var shippedText, out var unshippedText))
+            using var allShippedData = ArrayBuilder<ApiData>.GetInstance();


using var allShippedData

This threw me for a loop until I saw this was coming from an implementation in this repo. I assume we don't want a similar disposable pattern for the one in Roslyn?

The Roslyn one is disposable as well, just a slightly different pattern (due to compiler team concerns)

Weird, I'm not seeing the Roslyn ones as IDisposable from code inspection, but I'm probably missing it

we have a struct disposable wrapper around ours. look for using var _ = ArrayBuilder... in roslyn.sln :)

mavasani · 2023-03-30T02:25:32Z

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs

+        /// Cache from additional text instance to the api data we have read out for that specific file.  We only store
+        /// data for additional texts that explicitly match the public/internal api file names we expect.
+        /// </summary>
+        private static readonly ConditionalWeakTable<AdditionalText, ApiData> s_additionalTextToApiData = new();


Please don’t use a CWT here. We already have an API on AnalysisContext to allow caching per-file data across compilations: https://github.com/dotnet/roslyn/blob/3a7a7407ea3c831630ecf2754092c33df3a6e452/src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalysisContext.cs#L234

Internally, we also use a CWT, so the API already provides a simple way to achieve caching:
Note

Example test analyzer for this API: https://github.com/dotnet/roslyn/blob/3a7a7407ea3c831630ecf2754092c33df3a6e452/src/Compilers/Test/Core/Diagnostics/CommonDiagnosticAnalyzers.cs#L1605

Sure. I can do this.

So i don't seem to be able to use that helper. it is keyed off of SourceText. But avoiding the source-text is the point here. I need to keep off of AdditionalFile. Thoughts @mavasani ?

But avoiding the source-text is the point here

I am not sure I understand. SourceText is already strongly held by the AdditionalText: https://github.com/dotnet/roslyn/blob/3a7a7407ea3c831630ecf2754092c33df3a6e452/src/Compilers/Core/Portable/AdditionalTextFile.cs#L49-L53

Isn't the key point here avoiding repeated compute of the data associated with each AdditionalText file?

Ok. Went deep down the rabit hole of trying to make this keyed off sourcetext. It's painful. Effectively, the data we compute off the source-text holds onto data outside of the source text (specifically, things like the path and other information about the original additionaltext). I went down the path of trying to extract this out (e.g. RawApiData, ApiData, RawApiLine, ApiLine) and have the map point from the SourceText to the raw data. But then there was so much wrapping of this i needed to do to make working with things palatable (and not have additional copies/allocatins happening).

If we make it so that hte context objects allow for storing arbitrary K/V and not just Text/Value or Tree/Value, then this will be trivial to do.

I discussed this with manish, and we decided that would take too much time. So we're going to accept that this is a CWT. However, i'm making it non-static so it has the same lifetime as the analyzer (And whatever owns it).

mavasani

LGTM.

I'd like a signoff from @ToddGrun before merge.

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs

ToddGrun · 2023-03-30T14:25:57Z

LGTM. I am curious why we reach this code so often (I believe it's multiple times per keystroke). I thought Cyrus mentioned that we had a cache that should hit before this point.

CyrusNajmabadi · 2023-03-30T14:45:51Z

@mavasani it would def be good to see if there's something causing caching of diagnostic info to not work properly. Or if the diagnostic subsystem itself is calling this stuff multiple times over different codepaths.

CyrusNajmabadi added 3 commits March 29, 2023 16:05

In progress

42d760e

in progrss

1bb8e66

Finish

4412ee7

CyrusNajmabadi requested a review from a team as a code owner March 29, 2023 23:26

github-actions bot added the Community label Mar 29, 2023

CyrusNajmabadi commented Mar 29, 2023

View reviewed changes

CyrusNajmabadi added 2 commits March 29, 2023 16:31

Pooling

830e5de

Simplify

92d39d2

CyrusNajmabadi requested review from mavasani and ToddGrun March 29, 2023 23:36

Share data

6cae209

CyrusNajmabadi requested a review from sharwell March 30, 2023 00:11

CyrusNajmabadi commented Mar 30, 2023

View reviewed changes

Make into struct

7a29344

CyrusNajmabadi commented Mar 30, 2023

View reviewed changes

CyrusNajmabadi added 10 commits March 29, 2023 17:22

Records

254e7f1

inline

55f177c

REstore

7e20263

REstore

ff71b5f

REstore

87dfd85

REstore

914c18b

Simplify

7704c79

Simplify

c3b9928

Simplufy

7882c3d

doc

a86ecec

ToddGrun reviewed Mar 30, 2023

View reviewed changes

mavasani reviewed Mar 30, 2023

View reviewed changes

CyrusNajmabadi added 2 commits March 29, 2023 19:52

Switch approach

0aa693f

Make non static

b9eaa3f

CyrusNajmabadi requested a review from mavasani March 30, 2023 03:44

mavasani approved these changes Mar 30, 2023

View reviewed changes

CyrusNajmabadi requested a review from ToddGrun March 30, 2023 05:46

CyrusNajmabadi commented Mar 30, 2023

View reviewed changes

src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs Outdated Show resolved Hide resolved

Update src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs

ba8f956

ToddGrun approved these changes Mar 30, 2023

View reviewed changes

CyrusNajmabadi merged commit 9a0d72e into main Mar 30, 2023

CyrusNajmabadi deleted the readTextLess branch March 30, 2023 14:46

github-actions bot added this to the vNext milestone Mar 30, 2023

Cache the ApiData values we produce for an AdditionalText file. #6556

Cache the ApiData values we produce for an AdditionalText file. #6556

Uh oh!

Conversation

CyrusNajmabadi commented Mar 29, 2023

Uh oh!

CyrusNajmabadi Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddGrun commented Mar 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ToddGrun commented Mar 30, 2023

Uh oh!

CyrusNajmabadi commented Mar 30, 2023

Uh oh!

Uh oh!

CyrusNajmabadi Mar 29, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading