Skip to content

Conversation

@ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Jun 6, 2025

Create a specialized pool for arrays of ClassifiedSpanInternal

These arrays show up as a fairly large allocation in the typing scenario of the razor speedometer test. There are 150 MB (2.1%)of allocations of ClassifiedSpanInternal[] in the profile, but only 2.1 MB of ImmutableArray. This should be partly addressed by the earlier switch from DrainToImmutable use ToImmutableAndClear. However, this is also indicative that the 512 entry threshold for ArrayBuilderPool isn't sufficient for ClassifiedSpanInternal array counts. Roslyn has experienced this too and utilizes separate array pools for classification/tagging performance. The new pool has a threshold of 16K, much larger than the 512 entry limit. I tested this against a 60 KB razor file and was hitting about 7K entries, so this seemed like a pretty reasonable value. To ensure this doesn't add to total memory usage, the pool is limited to only 5 entries, as cconcurrent execution of classification isn't common.

image
...
image

These arrays show up as a fairly large allocation in the typing scenario of the razor speedometer test. There are 150 MB (2.1%)of allocations of ClassifiedSpanInternal[] in the profile, but only 2.1 MB of ImmutableArray<ClassifiedSpanInternal>. This should be partly addressed by the earlier switch from DrainToImmutable use ToImmutableAndClear. However, this is also indicative that the 512 entry threshold for ArrayBuilderPool isn't sufficient for ClassifiedSpanInternal array counts. Roslyn has experienced this too and utilizes separate array pools for classification/tagging performance. The new pool has a threshold of 16K, much larger than the 512 entry limit. I tested this against a 60 KB razor file and was hitting about 7K entries, so this seemed like a pretty reasonable value.
@ToddGrun ToddGrun requested a review from a team as a code owner June 6, 2025 20:16
@ToddGrun ToddGrun requested review from a team, DustinCampbell and davidwengier and removed request for a team June 6, 2025 20:16
_spans.Add(span);
}

private class Policy : IPooledObjectPolicy<ImmutableArray<ClassifiedSpanInternal>.Builder>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: sealed

Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

One small note, if this visitor shows up in more traces, is that I've often thought its used, at least in modern tooling, in cases where there are simpler and probably faster alternatives, so it might be worth reviewing the specific stacks that cause it to show up. eg, its used to answer the question "what language is this?", but if the code only cares about if its C#, I suspect using our mapping service would be cheaper, since thats a binary search over already created data.

@DustinCampbell
Copy link
Member

LGTM

One small note, if this visitor shows up in more traces, is that I've often thought its used, at least in modern tooling, in cases where there are simpler and probably faster alternatives, so it might be worth reviewing the specific stacks that cause it to show up. eg, its used to answer the question "what language is this?", but if the code only cares about if its C#, I suspect using our mapping service would be cheaper, since thats a binary search over already created data.

Agreed! Although, it's also worth noting that the data will be re-created on each keystroke.

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Jun 9, 2025

@dotnet/razor-compiler for 2nd compiler review

@ToddGrun ToddGrun merged commit 9e605b3 into main Jun 9, 2025
11 checks passed
@ToddGrun ToddGrun deleted the dev/toddgrun/SpecializedPool_ClassifiedSpanInternal branch June 9, 2025 22:03
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jun 9, 2025
@RikkiGibson RikkiGibson modified the milestones: Next, 18.0 P1 Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants