Use ToImmutableAndClear when using a pooled builder with a known length#63110
Conversation
| SyntaxEditor editor, CodeActionOptionsProvider fallbackOptions, CancellationToken cancellationToken) | ||
| { | ||
| using var spansDisposer = ArrayBuilder<TextSpan>.GetInstance(diagnostics.Length, out var spans); | ||
| using var _ = ArrayBuilder<TextSpan>.GetInstance(diagnostics.Length, out var spans); |
There was a problem hiding this comment.
no point naming these disposers. our pattern in teh vast majority of other cases is just to use the _ or _1... to signifiy this should not be accessed itself.
There was a problem hiding this comment.
Is there a point to having the disposers, if the calls are changed to clear? I thought the whole idea was to save the need to call clear, but if you're now calling clear, should they just be removed?
There was a problem hiding this comment.
yes. the disposers are necessary for two reasons:
- ensuring we clean up in the event of early return (for example, cancellation).
- ensuring the builder gets returned to the pool. ToImmutableAndClear will give the resultant ImmutableArray and clear the builder. but the builder is not returned to the pool.
Trying to understand why resetting to 0 is always good here. Wouldn't that mean we almost always want to call |
Yes, it would probably always be ok. But it will incur an extra check in the cases where not needed. These are at least teh cases that it is a pure win always :) |
I think there's an argument to be made to remove ToImmutable (or rename it), and only have the ToImmutableAsClear as the primary interface here. That's the mainline case. THe case of wanting to convert to an array, but still have data in the builder seems... likely not helpful. but we also have 100x more cases of ArrayBuilder and ToImmutable, so i'm warier about such a large change. |
Understood. I thought the main reason to want to use pooled arrays is because we can leave an array that is larger in memory and not have to allocate again. Might be something I'm missing here, since if we're not seeing benefits why do we use them at all? |
|
|
ToImmutableAndClear has two benefits over ToImmutable when the ArrayBuilder was given a known length.
So this is best of all worlds. No additional intermediary arrays. No copies. And we always pool, no matter how many elements we computed while using hte scratch array.
Note: i only updated the usages where i could trivially validate that the number of elements added was equal to the number of elements requested.