Specify builder capacities in incremental generation to avoid wasted scratch arrays.#62285
Conversation
| ? _previous._states | ||
| : _states.ToImmutable(); | ||
|
|
||
| _states.Free(); |
There was a problem hiding this comment.
this is an interesting pitfall. I originally wrote things this way so there was just a single .Free call. However, that actually was bad here as ToImmutable cannot optimize by using MoveToImmutable under the covers (like ToImmutableAndFree does).
| var count = 0; | ||
| foreach (var inputEntry in _states) | ||
| count += inputEntry.Count; | ||
| return count; |
There was a problem hiding this comment.
| var count = 0; | |
| foreach (var inputEntry in _states) | |
| count += inputEntry.Count; | |
| return count; | |
| return _states.Aggregate(0, (count, i) => count + i.Count); |
There was a problem hiding this comment.
Or add a Sum to our ImmutableArrayExtensions that takes a functor.
There was a problem hiding this comment.
i'm totally inconsistent here. i do Linq+StaticLambdas in some places, but foreaches in others :)
|
|
||
| var builder = graphState.CreateTableBuilder(previousTable, _name, _comparer); | ||
| var totalEntryItemCount = input1Table.GetTotalEntryItemCount(); | ||
| var builder = graphState.CreateTableBuilder(previousTable, _name, _comparer, totalEntryItemCount); |
There was a problem hiding this comment.
sorry, I don't yet have a strong grasp of this area. But was wondering if there's an analogous change to also make for calls to this method in 'BatchNode.cs' line 119 and 'SourceOutputNode.cs' line 51.
There was a problem hiding this comment.
BatchNode already does something similar. I'll look at sourcenode!
There was a problem hiding this comment.
ok, looking into this, BatchNode doesn't need this. It's effectively making an ImmutableArray<T>[] (an array of an array). The outer array it creates will effectively always be one element. So we're always going to pool that array builder just fine.
THe important thing for BatchNode is that hte inner ImmutableArray<T> is not creating waste. The work for that was already done in #62169
There was a problem hiding this comment.
SourceOutputNode looks like it could benefit from this optimization. However, i haven't seen that guy show up in traces (yet), so i haven't invested there. I will keep a look out if it comes up!
…ures/semi-auto-props * upstream/main: (887 commits) Ensure elastic trivia for reusable syntax in field generator (#62346) Fix typos in the incremental generators doc (#62343) Theme The "Generate Overrides" Dialog (#62244) Walk green-nodes in incremental-generator attribute-finding path (#62295) Cache the hash in compilation options (#62289) Respect dotnet_style_namespace_match_folder (#62310) Remove unreachable condition Specify builder capacities in incremental generation to avoid wasted scratch arrays. (#62285) Skip the test (#62287) Revert "Revert "Add Move Static Member To Existing Type (#61519)"" (#62284) Highlight the search term in the options page (#61301) Synch handlers with fix (#62209) Disable integration tests Fix Set capacity of builder to avoid expensive garbage. Add public APIs for opened and closed event handling for non-source documents Handle possible null symbols in `getAttributeTarget` (#62137) Perform a lookahead rather than a parsing attempt in order to determine if current token starts a conversion operator declaration. (#62240) Fix a race in CachingDictionary. (#62248) Simplify ...
Shaves 250MB of memory from incremental scenario:
to