Immediately filter syntax trees down to those that either have attributes or global-using-aliases.#62483
Conversation
…er intermediary tables.
| /// the number of *relevant syntax trees*. This can lead to huge memory churn keeping track of a high number of | ||
| /// trees, most of which are not going to be relevant. | ||
| /// </summary> | ||
| private static readonly ConditionalWeakTable<SyntaxTree, SyntaxTreeInfo> s_treeToInfo = new(); |
There was a problem hiding this comment.
note: as an alternative to a CWT, i could also use a normal dictionary created inside ForAttributeWithSimpleName and then captured. But i wasn't certain what the semantics were for the lifetimes of these pipelines. @chsienki what are the lifetimes? And are the pipelines executed concurrently?
...ilers/Core/Portable/SourceGeneration/Nodes/SyntaxValueProvider_ForAttributeWithSimpleName.cs
Outdated
Show resolved
Hide resolved
...ilers/Core/Portable/SourceGeneration/Nodes/SyntaxValueProvider_ForAttributeWithSimpleName.cs
Show resolved
Hide resolved
...ilers/Core/Portable/SourceGeneration/Nodes/SyntaxValueProvider_ForAttributeWithSimpleName.cs
Outdated
Show resolved
Hide resolved
...ilers/Core/Portable/SourceGeneration/Nodes/SyntaxValueProvider_ForAttributeWithSimpleName.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| private IEnumerable<int> YieldItems(OneOrMany<int> items) | ||
| private static IEnumerable<int> YieldItems(OneOrMany<int> items) |
There was a problem hiding this comment.
alternative recommendation?
There was a problem hiding this comment.
Places that use this can use GetEnumerator instead of YieldItems, no?
There was a problem hiding this comment.
unfortunately no, that returns an Enumerator/IEnumerator. The test helpers test IEnumerables. I could make OneOrMany IEnumerable, but it seemed excessive (and potentially worrying for boxing reasons) just for test purposes.
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 76)
|
@chsienki PTAL |
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM. There are 80 commits here, compared to 96 lines added, which we will need to scroll past if we need to search the history on GitHub for build-breaking issues, etc. Consider consolidating those commits somehow, e.g. via interactive rebase, or by squash merge.
| .SelectMany(static (c, _) => c.SyntaxTrees) | ||
| .SelectMany((compilation, cancellationToken) => compilation.SyntaxTrees | ||
| .Select(tree => GetTreeInfo(tree, syntaxHelper, cancellationToken)) | ||
| .Where(info => info.ContainsGlobalAliases || info.ContainsAttributeList)) |
There was a problem hiding this comment.
Did you consider adding flags to SyntaxTree instead? Wondering about what the tradeoffs would be.
There was a problem hiding this comment.
I'm definitely considering that (And talked to julien about just that). i'm going to separate into a distinct PR as the bleeding into SyntaxTree feels like something to consider outside of this. Will do next.
Yup. It's set to squash-merge. |
…utes or global-using-aliases. (dotnet#62483) * 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
Followup to #62389
Cuts down CPU time from 3.4 to 1.4seconds (58% improvement).
Cuts down memory from 1.3GB:
To 0.5GB (60% improvement):
--
At this point all memory is contained here:
Which are the SyntaxTree arrays the compilation itself points at. We can only improve on this if we change the underlying storage for a Compilation to something better on memory usage (like a ImmutableList).