Use TemporaryArray<T> instead of ArrayBuilder<T>

[sharwell]

This pull requests converts a few uses of ArrayBuilder<T> to TemporaryArray<T> where it is reasonably likely to be beneficial. It is primarily intended for reviewing the overall usability of the new type.

[CyrusNajmabadi]

so, tbh, block structure is theplace i would not expect to see wins. in other words, we're almost always going to be +4 elemetns rights?

[sharwell]

I'm guessing it's going to be perf-neutral for this case, but I like it because it's hit often and helps exercise the non-copyable analyzer we're relying on.

[CyrusNajmabadi]

sgtm.

[CyrusNajmabadi]

so i'm not opposed to this. but this feels like it would not actually be an perf improvement. the reason being that near all files will have +4 elements in them. as such, we'll always spill to the builder, so we'll only be adding overhead here.

That said, i'm ok with this due to consistency if we just want to use this almost anywhere, accepting that in some cases we'll have a little extra perf hit. One thing that i would think could be interesting (outside of the scope of this pr) is if we ended up having thread-local pools for our underlying builders. That way we would never have a single problem colliding when we returned the builders.

[sharwell]

if we ended up having thread-local pools

Once we switch to .NET Core, I plan to rewrite the pool to have processor core-specific first elements hinted by Thread.GetCurrentProcessorId. This will have performance similar to thread local storage but without the corresponding working set increase. TemporaryArray<T> would still be better for small storage since the pool efficiency is reduced as the number of active leases increases.

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jaredpar]

Where can we get a concise description of where TemporaryArray<T> is believed to be better than our traditional builder?

[CyrusNajmabadi]

Where can we get a concise description of where TemporaryArray is believed to be better than our traditional builder?

@jaredpar it should always be better when the array will have to store 4 or less items. This is for two reasons:

1. the values are just stored on the stack, and no part of the TempArray will hit the heap.
2. there is no contention/garbage possibility around pooling. this is because nothing is pulled (or returned) to a pool until you hit 5 elements. This avoids those costs, as well as the (real) garbage cost that arises with pooling when concurrent returns happen and one returned item it overwritten by another returned item.

You can practically think about it as if this gives you a small stackalloc'ed scratch area for lots of small-collection scenarios. Only once you go past that scratch area do you have the same perf costs that you'd have today with normal ArrayBuilder.

[sharwell]

@jaredpar The ArrayBuilder<T> pooling can be really bad on highly multi-core machines. In addition to avoiding data races between cores in the object pool (which improves performance), this change reduces the miss rate for the object pools which improves performance even in cases where TemporaryArray<T> is not used. My guess is TemporaryArray<T> is better for cases where a reasonable percentage of calls have at most 4 elements in the array.

This change does not address the fact that a new builder mutable value type could be created for known fixed-size allocations, which would be ideal for cases following the pattern ImmutableArray.CreateBuilder<T>(int) followed by builder.MoveToImmutable().

[333fred]

What is this update bringing in?

[sharwell]

dotnet/roslyn-analyzers#4377

[333fred]

Compiler changes LGTM (commit 5)