Store helpful info needed by incremental-generation on syntax-tree itself#62551
Store helpful info needed by incremental-generation on syntax-tree itself#62551CyrusNajmabadi merged 90 commits intodotnet:mainfrom
Conversation
…er intermediary tables.
|
@jcouv @RikkiGibson @chsienki this is ready for review. thanks! |
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM. Curious whether it is worthwhile to try and optimize further.
Do you mean overall? or just this particular area? |
| NotComputedYet, | ||
| None = 1 << 0, | ||
| ContainsGlobalAliases = 1 << 1, | ||
| ContainsAttributeList = 1 << 2, |
I meant in this specific area (checks that need to be performed on syntax trees in each incremental generation pass). |
Ah... so there is one additional check i could do, but i was wary about it. Specifically, we could pre-walk the trees (that have attribute lists in them) and see if the file passes the client's passed in 'predicate' function. Then we would never have to check trees again that we know could never match their predicate. However, i didn't do this for two reasons:
Filtering like that is really good when it's either extremely cheap, or if it's expensive, but greatly limits the set of items you need to look at. Otherwise, it's not htat helpful. That said, other generators may behave differently, so if we have some good examples that could benefit from anotehr filtering pass, we could consider this in the future. |
|
@jcouv ptal :-) |
| /// generation framework to compute and cache data once against a tree so it does not have to go back to source | ||
| /// for untouched trees when other trees in the compilation are modified. | ||
| /// </summary> | ||
| private SourceGeneratorSyntaxTreeInfo _sourceGeneratorInfo = SourceGeneratorSyntaxTreeInfo.NotComputedYet; |
There was a problem hiding this comment.
Conceptually I don't have an issue with what we're doing here. It makes sense to cache hot lookup items like this. Putting it on the SyntaxTree itself doesn't really bother me at all. In fact it seems like we could even expand on this to maintain this field across incremental parses (maybe).
My brain is mildly upset at the terminology of source generator though. It feels too specific. Yes that's the primary usage today but the attribute lookup is could be more general purpose. It would allow us to do othere optimizations in the code base.
Did you consider giving it aless specific name?
There was a problem hiding this comment.
Did you consider giving it aless specific name?
I def did, but fell back into that flip-flopping of: "this is only for source-generators today", vs "maybe other things in the future could benefit...."
What would honestly make a ton of sense would be if we had SyntaxTree cache the DeclTable entry for it, and then allow the decl table entry to have these additional bits, as well as the other bits it holds onto today. However, this would require making things common between C# and VB and would def be larger in scope.
So this was basically a way to solve this problem in a very tiny, scoped fashion. I def think tehre may be more cohesive approaches we coudl take, but they would also be much larger in scope and i figured would also take ages to get into the compiler :) I'd like to put SG work to bed (this is literally my last PR for this area), so i went with narrow and targetted.
There was a problem hiding this comment.
In fact it seems like we could even expand on this to maintain this field across incremental parses (maybe).
we def could (though it's a bit non-trivial). The general idea would be to do something similar to how we store the bits for 'ContainsDiagnostics' and 'ContainsAnnotations'. Nodes could have a bitset provided to them of interesting bits. As you build the parent nodes, they union those bits together, and you then have all that information available in the root node.
This then makes incremental fall out trivially. If we make a new AttributeList/GlobalUsing node we set that bit, and pass it upwards.
I explored this, but it was much larger in scope and it's terrifying from a perf perspective to touch the green nodes at all. We squeezed perf heavily there, and it's a space where you could change a line and easily cause +- large perf percentages. So i def didn't want to go that far when i'm really ok just storing two bits here.
* upstream/main: Bind IterationErrorVariable to a natural type for error recovery (dotnet#62601) [main] Update dependencies from dotnet/source-build-externals (dotnet#61173) Cleanup: remove VisualStudioWorkspaceImpl.TryGetContainedDocument (dotnet#62616) EnC: A couple of small fixes (dotnet#62454) Report error for ref field declared in type other than ref struct (dotnet#62615) Store helpful info needed by incremental-generation on syntax-tree itself (dotnet#62551)
…self (dotnet#62551) * Cache the hash in compilation options * Remove * Update src/Compilers/Core/Portable/InternalUtilities/Hash.cs * Update src/Compilers/Core/Portable/InternalUtilities/Hash.cs * Update src/Compilers/Core/Portable/InternalUtilities/Hash.cs * Fix api * Walk green-nodes in incremental-generator attribute-finding path * Use green nodes * REmove * Fix tests * Avoid allocating builder if not needed * Simplify * Only loop once * Do not look for load/reference directives when there can't be any * REmove use of Lazy in DeclarationTable for rarely used values. * Remove lazy * In progress * Change the data-flow path for searching for attributes to prevent fewer intermediary tables. * Remove unused type * Simplify * Update tests * Make internal * Update * Update src/Compilers/Core/Portable/SourceGeneration/Nodes/NodeStateTable.cs * Update comment * Filter large tables before processing htem. * Update tests * Sort * Fix grammar * Fix assert * Add back in * Revert * Simplify * remove * Simplify * Remove * Simplify global aliases * Prefilter to only trees that have attributes or globalaliases in them * INcrease * Only check the top level for global usings. * Remove comment * Simplify * Add docs * Revert * Make static * Extract common code * PR feedback * Use helper * update run counts * Chain compilations * Fix grammar * Don't use implicit construction * Fix * Tweak benchmark * Simplify walk * Cache syntax-tree-info on the syntax-tree itself. * no clean * Fast path detecting what sort of using construct we have. * REvert * revert * Cleanup * Simplify
Followup to #62483. Extracted out as potentially more contentious.
This stores the booleans about whether a syntax-tree contains attributes/global-usings on the SyntaxTree object itself (computed lazily of course). This avoids an intermediary CWT which itself has sizable impact (both the complexity of it, as well as the need to hash). Overall time goes down from 6.6s to 4.1 (a 38% drop). Memory usage also went down in this PR (from 2.88gb to 2.63 gb), though that's due to smarter allocation of an array we need.
CWT drop is from teh removal of hte following items: