Allow BatchNodes to reuse prior computed arrays if they would generate hte same value #62169
Conversation
| sourceValuesBuilder.Builder.SequenceEqual(previousTable.Single().item, EqualityComparer<TInput>.Default)) | ||
| { | ||
| return (previousTable.Single().item, sourceInputs); | ||
| } |
There was a problem hiding this comment.
this part is the new logic that reuses from the previousTable if possible.
src/Compilers/Core/Portable/SourceGeneration/Nodes/BatchNode.cs
Outdated
Show resolved
Hide resolved
|
@jcouv @dotnet/roslyn-compiler for eyes. Thanks! |
|
@chsienki for eyes. Thanks! |
src/Compilers/Core/Portable/SourceGeneration/Nodes/BatchNode.cs
Outdated
Show resolved
Hide resolved
|
@cston have taken your suggested approach. will comment inline with what's going on. thanks! |
| sourceInputsBuilder?.Add((entry.Step!, entry.OutputIndex)); | ||
|
|
||
| if (entry.State != EntryState.Removed) | ||
| entryCount++; | ||
| } |
There was a problem hiding this comment.
first pass determiens how many entries we have. note: this will allocate an IEnumerable. If we worry about the cost of that, we could have a dedicated method on SourceTable (called something like "CountOfNonRemovedEntries").
src/Compilers/Core/Portable/SourceGeneration/GeneratorTimerExtensions.cs
Outdated
Show resolved
Hide resolved
|
|
||
| return (result, sourceInputs); | ||
|
|
||
| ImmutableArray<TInput> tryReusePreviousTableValues(int entryCount) |
There was a problem hiding this comment.
this does non-allocating pass looking to see if we can reuse the prior array. it checks the lengths are the same then checks all non-removed items against the prior array. if a match, we can reuse.
| } | ||
|
|
||
| return newTable.ToImmutableAndFree(); | ||
| ImmutableArray<TInput> computeCurrentTableValues(int entryCount) |
There was a problem hiding this comment.
finally, we do a pass where we allocate a builder. however, importantly, we know the exact amount of items we need, so the builder doesn't create a scratch array that has the chance of not being pooled if too large.
| NodeStateTable<ImmutableArray<TInput>>.Builder newTable) | ||
| { | ||
| // Do an initial pass to both get the steps, and determine how many entries we'll have. | ||
| var sourceInputsBuilder = newTable.TrackIncrementalSteps ? ArrayBuilder<(IncrementalGeneratorRunStep InputStep, int OutputIndex)>.GetInstance() : null; |
There was a problem hiding this comment.
Could we initialize this with sourceTable.Count as well?
There was a problem hiding this comment.
yes. that would be a reasonable approximation. The only reasons i didn't were that:
- it's not exactly correct. Tables have entries, and entries have steps. So it's quite possible to have way more steps tahn .Count returns.
- TrackingIncrementalSteps is done in tests, so being that efficient is not a pressing concern on my part. It doesn't show up in my benchmarks either as i'm not turning on that capability :)
ghost
added this to the 
Followup to #62100
This drops memory allocations by around 500 MB:
to
The reason for this is that we're constantly producing the exact same
GlobalAlias[]array as the result of our .Collect call in the incremental generator. The change here lets us peek back at the prior array and then reuse it if we would produce the same.Here we literally go from 5000 allocations to a single one that is shared from that point onwards. Only if the global aliases actually change would we need to perform a new allocation.