Avoid intermediary allocation in Incremental Driver#62089
Avoid intermediary allocation in Incremental Driver#62089CyrusNajmabadi merged 2 commits intodotnet:mainfrom
Conversation
| internal Builder(NodeStateTable<T> previous, string? name, bool stepTrackingEnabled) | ||
| { | ||
| _states = ArrayBuilder<TableEntry>.GetInstance(); | ||
| _states = ArrayBuilder<TableEntry>.GetInstance(previous._states.Length); |
There was a problem hiding this comment.
helps avoid resizes. useful since hte prior and current state lengths are often similar
There was a problem hiding this comment.
note: this pooled array is still highly problematic. because it routinely is >128 elements, we never actually pool it. so we end up churning GB of arrays in incremental scenarios.
There was a problem hiding this comment.
Would creating a dedicated pool for generators that deals in large arrays only be a potential help here?
There was a problem hiding this comment.
@jaredpar very much so. but i'd like to do in another PR :)
| } | ||
|
|
||
| entries = _states[^1].ToImmutableArray(); | ||
| entry = _states[^1]; |
There was a problem hiding this comment.
this is the alloc we're avoiding. tableentry is already immutable, so we can return that directly.
| } | ||
|
|
||
| private readonly struct TableEntry | ||
| internal readonly struct TableEntry |
There was a problem hiding this comment.
don't love this piece. any ideas?
There was a problem hiding this comment.
Maybe make a new type next to it CachedEntryEnumerable?
There was a problem hiding this comment.
tried making this work, but it still had a problem. i can't see any way to make the accessibility work where CachedEntryEnumerable references TableEntry, but doesn't fall afoul of accessibility rules. e.g.:
e.g. even this doesn't work:
public readonly struct CachedEntryEnumerable
{
private readonly TableEntry _entry;
internal CachedEntryEnumerable(TableEntry entry)
{
_entry = entry;
}making the constructor private doesn't work either as that then can't be invoked.
There was a problem hiding this comment.
for now, i'm ok exposing this. it's purely immutable data, so it should be very safe. It might make sense to make this a top level type though at this point instead of an internal nested type.
is there a documented workflow you're using to test this? |
The workflow is pretty trivial (and is in IncrementalSourceGeneratorBenchmarks). Basically, the benchmark opens Roslyn.sln, opens a file (in my case AbstractCaseCorrectionService.cs), then it continually makes edits to it, getting the incremental compilation result (using the generator driver to actually run the generator pipeline). I have a trivial generator which is just calling: var input = ctx.SyntaxProvider.ForAttributeWithMetadataName(
"System.Text.Json.Serialization.JsonSerializableAttribute",
(n, _) => n is ClassDeclarationSyntax,
(ctx, _) => 0);In other words, it is saying: run this logic when we hit a class with the |
|
@RikkiGibson @jcouv for additional reviews plz :) |
This saves 600 MB of allocs in my editing testing.