Filter trees to only those containing global-usings or attributes prior to analyzing them. #62444
Conversation
| Assert.Equal(IncrementalStepRunReason.Removed, runResult.TrackedSteps["result_ForAttribute"].Single().Outputs.Single().Reason); | ||
| Assert.False(runResult.TrackedSteps.ContainsKey("compilationUnit_ForAttribute")); | ||
| Assert.False(runResult.TrackedSteps.ContainsKey("compilationUnitAndGlobalAliases_ForAttribute")); | ||
| Assert.False(runResult.TrackedSteps.ContainsKey("result_ForAttribute")); |
There was a problem hiding this comment.
@chsienki i need help here. for some reason the incremental generator is not tracking any steps here when the list filters down to the empty set. Can you help me debug through this to find out why that is?
There was a problem hiding this comment.
I looked at this, and I see whats going on. I think I have a fix for it, but its a little involved. I think we should open an issue here, and take the fix as a separate PR.
There was a problem hiding this comment.
Tracked with #62468
| return syntaxHelper.ContainsGlobalAliases(root, cancellationToken); | ||
| }) | ||
| .Select((tree, cancellationToken) => getGlobalAliasesInCompilationUnit(syntaxHelper, tree.GetRoot(cancellationToken))) | ||
| .WithTrackingName("individualFileGlobalAliases_ForAttribute"); |
There was a problem hiding this comment.
huge savings, as most files do not have global aliases.
| // Don't bother looking in trees that don't even have attributes in them. | ||
| var root = tree.GetRoot(cancellationToken); | ||
| return ContainsAttributeList(root.Green, syntaxHelper.AttributeListKind); | ||
| }) |
There was a problem hiding this comment.
large savings when attributes are rare in a project.
| @@ -79,7 +79,6 @@ public NodeStateTable<TOutput> UpdateStateTable(DriverStateTable.Builder builder | |||
| } | |||
| } | |||
|
|
|||
| Debug.Assert(newTable.Count == totalEntryItemCount); | |||
There was a problem hiding this comment.
this assert was just not safe. in a transform we can be transforming 1:N (where N is 0, 1 or >2). So we can't make a determination about how many items teh final count is. It's till good to prime the number of items based on what we previously saw, for the 1:1 case though.
|
@jcouv ptal. tahnks! |
src/Compilers/CSharp/Portable/SourceGeneration/CSharpSyntaxHelper.cs
Outdated
Show resolved
Hide resolved
| @@ -60,5 +63,7 @@ internal abstract class AbstractSyntaxHelper : ISyntaxHelper | |||
|
|
|||
| public abstract void AddAliases(GreenNode node, ArrayBuilder<(string aliasName, string symbolName)> aliases, bool global); | |||
| public abstract void AddAliases(CompilationOptions options, ArrayBuilder<(string aliasName, string symbolName)> aliases); | |||
|
|
|||
| public abstract bool ContainsGlobalAliases(SyntaxNode root, CancellationToken cancellationToken); | |||
There was a problem hiding this comment.
Not part of your change, but wondering why we have both interface and abstract class flavors of this? Looks like the abstract class doesn't really add functionality here?
There was a problem hiding this comment.
So this is a pattern i follow when doing thigns with C# and VB. If you don't do this, then the VB side is more obnoxious as every impl has to include the interface information. the abstract approach means that both C# and VB just need to override.
There was a problem hiding this comment.
One developer's awesome feature that makes it clear when a method implements an interface method is another developer's annoying requirement to make me restate things that can be implicit ;)
src/Compilers/CSharp/Portable/SourceGeneration/CSharpSyntaxHelper.cs
Outdated
Show resolved
Hide resolved
...ilers/Core/Portable/SourceGeneration/Nodes/SyntaxValueProvider_ForAttributeWithSimpleName.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/SourceGeneration/CSharpSyntaxHelper.cs
Outdated
Show resolved
Hide resolved
ghost
added this to the 
* upstream/main: (62 commits) Prevent assert from being hit (dotnet#62366) Don't offer '??=' for pointer types (dotnet#62476) Integrate generator times into /reportAnalyzer (dotnet#61661) Switch to a callback for cleaning up resources instead of passing in an explicit IDisposable. (dotnet#62373) Filter trees to only those containing global-usings or attributes prior to analyzing them. (dotnet#62444) Update PublishData.json Complete 'file' support for `SyntaxGenerator` (dotnet#62413) Apply changes directly to text buffer (dotnet#62337) Remove LangVer check from extended nameof binding (dotnet#62339) Fixed shared project file error (dotnet#62466) Handle new error codes Use MSBuid generated property for package path Exclude BCL libraries from Roslyn vsix Bump the integration test timeouts a bit Skip the balanced switch dispatch optimization for patterns on floating-point inputs (dotnet#62322) Test helpers shouldn't escape quotes by default (dotnet#62321) Reuse prior TableEntry values in the increment NodeTable builder if possible. (dotnet#62320) Install 3.1 runtime for SBOM tool Generate VS SBOM during official build. Minor refactoring in 'required' handling for override completion (dotnet#62422) ...
This cuts down on the sizes of tables enormously, by only have the tables be the size of those much smaller sets versus the entire number of trees we have (which could be in the thousands).
This takes us from 3.8s to 1.9s (a 50%) improvement, and from 2.2GB memory:
To 0.92GB (a 58% savings):