Remove double array allocation in SemanticTokens #74271
Conversation
This code was calling ArrayBuilder.ToArray, which would always allocate a new array as a result. Additionally, the arraybuilder itself would end up being large enough that it wouldn't end up going back into the pool. Thus, two large array were allocated on many calls into this method. There really is no need for this method to use ArrayBuilder, as the code that ends up owning the resultant array doesn't attempt to put it back into a pool. This shows up as about 0.5% of allocation in devenv in the customer profile I'm looking at, half of which should be removed by this change.
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
|
There's an issue here, didn't notice that currentClassifiedSpanIndex was modified inside the loop, outside of normal loop incrementing |
Switched back to a pool, as the array size isn't known before the loop. Instead of ArrayBuilder, which doesn't pool array bigger than 128 elements, used a separate ObjectPool<List> |
|
@dotnet/roslyn-ide -- ptal |
| @@ -243,23 +245,31 @@ private static void ConvertMultiLineToSingleLineSpans(SourceText text, Segmented | |||
|
|
|||
| var tokenTypeMap = SemanticTokensSchema.GetSchema(supportsVisualStudioExtensions).TokenTypeMap; | |||
|
|
|||
| using var _ = ArrayBuilder<int>.GetInstance(5 * classifiedSpans.Count, out var data); | |||
| for (var currentClassifiedSpanIndex = 0; currentClassifiedSpanIndex < classifiedSpans.Count; currentClassifiedSpanIndex++) | |||
| var data = s_tokenListPool.Allocate(); | |||
There was a problem hiding this comment.
if you use .GetPooledObject you can use a using.
| } | ||
| finally | ||
| { | ||
| data.Clear(); |
There was a problem hiding this comment.
i am fine not clearing an list of ints.
There was a problem hiding this comment.
Sure, went ahead and switched to GetPooledObject. Clearing will now occur upon retrieval from the pool.
This code was calling ArrayBuilder.ToArray, which would always allocate a new array as a result. Additionally, the arraybuilder itself would end up being large enough that it wouldn't end up going back into the pool. Thus, two large array were allocated on many calls into this method.
There really is no need for this method to use ArrayBuilder, as the code that ends up owning the resultant array doesn't attempt to put it back into a pool.
This shows up as about 0.5% of allocation in devenv in the customer profile I'm looking at, half of which should be removed by this change.