Remove declaration-only-compilation from CompilationTracker by CyrusNajmabadi · Pull Request #56139 · dotnet/roslyn

CyrusNajmabadi · 2021-09-02T21:35:54Z

We used this compilation to speed up some string searches for certain features (and some public APIs we have). Turns out we don't need this given the indices we already have for features like FAR and NavTo. Given this, we can remove this compilation and simplify the compilation tracker a bunch.

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.State.cs

src/Workspaces/Core/Portable/FindSymbols/DeclaredSymbolInfo.cs

src/Workspaces/Core/Portable/Workspace/Solution/Project.cs

jasonmalinowski · 2021-09-04T00:19:57Z

src/Workspaces/Core/Portable/Workspace/Solution/Project.cs

+            if (!this.SupportsCompilation)
+                return false;
+
+            var tasks = this.Documents.Select(async d =>


Source generated documents?

nope! i want to make sure we have a way to do source-gen'ed docs in a way that features can say if they need low-latency or not. We don't want to slow down things like 'add using' behind arbitrary 3rd party code. let's chat about a plan on that for monday.

Agreed, we probably want some flag here or something, but needs to be done carefully.

src/Workspaces/Core/Portable/FindSymbols/Declarations/DeclarationFinder.cs

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs

...le/LanguageServices/DeclaredSymbolFactoryService/AbstractDeclaredSymbolInfoFactoryService.cs

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.State.cs

CyrusNajmabadi · 2021-09-08T17:18:45Z

@jasonmalinowski this is ready for review.

jasonmalinowski

, and I love the state renames to things that explain a lot more their current needs.

jasonmalinowski · 2021-09-08T21:08:21Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs

            /// produce in progress snapshots that can be accessed from other threads.
            /// </summary>
-            private Task<CompilationInfo> BuildCompilationInfoAsync(
+            private async Task<CompilationInfo> BuildCompilationInfoAsync(


Did we need the async here?

(we had gotten away with not needing the extra state machine before, not sure what forced it now)

jasonmalinowski · 2021-09-08T21:15:15Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs

-                    WriteState(new FullDeclarationState(compilation, TextDocumentStates<SourceGeneratedDocumentState>.Empty, generatorDriver: null, generatedDocumentsAreFinal: false), solutionServices);
+                    WriteState(new AllSyntaxTreesParsedState(compilation, generatedDocuments, generatorDriver, generatedDocumentsAreFinal), solutionServices);


Hmm, good catch to make sure we don't lose these -- I can't tell if we had a bug here before (where in some obscure cases we might end up losing prior generated documents), or whether we couldn't have gotten here before once we had generated everything.

jasonmalinowski · 2021-09-08T21:16:51Z

src/Workspaces/Core/Portable/Workspace/Solution/Project.cs

+            if (!this.SupportsCompilation)
+                return false;
+
+            var tasks = this.Documents.Select(async d =>


Agreed, we probably want some flag here or something, but needs to be done carefully.

jasonmalinowski · 2021-09-08T21:18:13Z

src/Workspaces/Core/Portable/FindSymbols/Declarations/DeclarationFinder.cs

-                // full compilation if we don't have that.
+                // Do a quick syntactic check first using our cheaply built indices.  That will help us avoid creating
+                // a compilation here if it's not necessary.  In the case of an exact name search we can call a special 
+                // overload that quickly uses the direct bloom-filter identifier maps in the index.  If it's nto an 


Suggested change

// overload that quickly uses the direct bloom-filter identifier maps in the index. If it's nto an

// overload that quickly uses the direct bloom-filter identifier maps in the index. If it's not an

Remove declaration-only-compilation from CompilationTracker

8f9575f

CyrusNajmabadi requested a review from jasonmalinowski September 2, 2021 21:35

CyrusNajmabadi requested a review from a team as a code owner September 2, 2021 21:35

ghost added the Area-IDE label Sep 2, 2021

CyrusNajmabadi added 6 commits September 2, 2021 16:32

Merge remote-tracking branch 'upstream/main' into removeDeclCompilatino

968b67e

Add namespaces to syntax index.

0bce6b6

Share code

4f88c29

Simplify

40fa454

Comment

7185a7a

Fix assert

a5f6b31

jasonmalinowski reviewed Sep 3, 2021

View reviewed changes

jasonmalinowski reviewed Sep 4, 2021

View reviewed changes

...le/LanguageServices/DeclaredSymbolFactoryService/AbstractDeclaredSymbolInfoFactoryService.cs Show resolved Hide resolved

Add docs

c0a7943

CyrusNajmabadi force-pushed the removeDeclCompilatino branch from 0906b28 to c0a7943 Compare September 4, 2021 01:26

CyrusNajmabadi added 9 commits September 3, 2021 18:27

rename and doc

f07ec59

Add docs

2ebc4c8

Throw for case we don't handle

cfbb8e9

Move back to project.

3e56b53

Add quick check

97d82a6

Merge remote-tracking branch 'upstream/main' into removeDeclCompilatino

f18c75e

Don't drop generated docs

1100412

Merge remote-tracking branch 'upstream/main' into removeDeclCompilatino

f77cb60

Restore code

c40969f

CyrusNajmabadi commented Sep 7, 2021

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs Show resolved Hide resolved

CyrusNajmabadi added 3 commits September 7, 2021 13:01

Add back in a state for no cmopilatins.

06d971a

Formatting

20dd104

Formatting

3186809

CyrusNajmabadi commented Sep 7, 2021

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.State.cs Outdated Show resolved Hide resolved

Remove unused method

644c9f4

CyrusNajmabadi added 3 commits September 7, 2021 13:15

simplify

adcba1f

use array builder

5260302

Remove unused usings

73ae278

CyrusNajmabadi mentioned this pull request Sep 7, 2021

Merge all source generator state in CompilationTracker into a helper type. #56229

Merged

CyrusNajmabadi requested a review from jasonmalinowski September 7, 2021 22:19

jasonmalinowski approved these changes Sep 8, 2021

View reviewed changes

CyrusNajmabadi merged commit e0909cf into dotnet:main Sep 8, 2021

ghost added this to the Next milestone Sep 8, 2021

CyrusNajmabadi deleted the removeDeclCompilatino branch September 8, 2021 23:38

Cosifne modified the milestones: Next, 17.0.P5 Sep 27, 2021

		WriteState(new FullDeclarationState(compilation, TextDocumentStates<SourceGeneratedDocumentState>.Empty, generatorDriver: null, generatedDocumentsAreFinal: false), solutionServices);
		WriteState(new AllSyntaxTreesParsedState(compilation, generatedDocuments, generatorDriver, generatedDocumentsAreFinal), solutionServices);

	// overload that quickly uses the direct bloom-filter identifier maps in the index. If it's nto an
	// overload that quickly uses the direct bloom-filter identifier maps in the index. If it's not an

Conversation

CyrusNajmabadi commented Sep 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Sep 8, 2021

Uh oh!

jasonmalinowski left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants