Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid allocations from calls to CultureInfo.GetCultures #8231

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

ToddGrun
Copy link
Contributor

A local profile taken during running the vsixinstaller on roslyn indicated this call was allocating quite heavily in the "devenv /updateconfiguration" process the vsixinstaller kicks off. DirectoryBasedTemplate.ParseLocFileName was allocating 8.1% of all allocations in that devenv process, nearly all of which comes from the CultureInfo.GetCultures call.

Instead of allocating all cultures and an array to hold them by calling CultureInfo.GetCultures, this code can use CultureInfo.GetCultureInfo with the requested culture name and use that result instead. Note that GetCultureInfo throws if the requested culture name isn't supported, so we need to catch the corresponding CultureNotFoundException.

image

A local profile taken during running the vsixinstaller on roslyn indicated this call was allocating quite heavily in the "devenv /updateconfiguration" process the vsixinstaller kicks off. DirectoryBasedTemplate.ParseLocFileName was allocating 8.1% of all allocations in that devenv process, nearly all of which comes from the CultureInfo.GetCultures call.

Instead of allocating all cultures and an array to hold them by calling CultureInfo.GetCultures, this code can use CultureInfo.GetCultureInfo with the requested culture name and use that result instead. Note that GetCultureInfo throws if the requested culture name isn't supported, so we need to catch the corresponding CultureNotFoundException.
@ToddGrun ToddGrun requested a review from a team as a code owner July 16, 2024 17:10
@joeloff
Copy link
Member

joeloff commented Jul 16, 2024

@ToddGrun is this something we need sooner in VS, e.g. in the current servicing builds? main is currently .NET 9 which won't ship until later this year and only in newer VS releases after 17.10.

@ToddGrun
Copy link
Contributor Author

@ToddGrun is this something we need sooner in VS, e.g. in the current servicing builds? main is currently .NET 9 which won't ship until later this year and only in newer VS releases after 17.10.

As we chatted offline, this isn't high priority, but it might be worth porting to 8.0 SDKs.

@marcpopMSFT
Copy link
Member

Triage: Approved for porting to 8.0 versions. We can trigger backports. Did you want to fill the tactics template or should we?

@marcpopMSFT marcpopMSFT merged commit 61c0a80 into dotnet:main Jul 16, 2024
10 checks passed
@marcpopMSFT
Copy link
Member

/backport to release/8.0.1xx

Copy link
Contributor

Started backporting to release/8.0.1xx: https://github.com/dotnet/templating/actions/runs/9964346280

@ToddGrun
Copy link
Contributor Author

Triage: Approved for porting to 8.0 versions. We can trigger backports. Did you want to fill the tactics template or should we?

Would appreciate some help with that. I can try to provide any information needed.

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.

4 participants