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

Consider setting InvariantGlobalization in api template #47029

Closed
eerhardt opened this issue Mar 3, 2023 · 15 comments · Fixed by #47066
Closed

Consider setting InvariantGlobalization in api template #47029

eerhardt opened this issue Mar 3, 2023 · 15 comments · Fixed by #47066
Assignees
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels

Comments

@eerhardt
Copy link
Member

eerhardt commented Mar 3, 2023

API server applications often don't need to consider culture behavior. The data flowing in and out of the application is created and consumed by other applications (not humans). This data is typically in a culture independent format because it is consumed by other applications.

We should consider adding <InvariantGlobalization>true</InvariantGlobalization> to the dotnet new api template. This can easily be removed by the app developer, if their API app needs culture aware behavior. Alternatively, if we really thought the decision should be directly made by the user, we can make it an optional checkbox in the template dialog in VS.

Enabling InvariantGlobalization brings with it some nice performance benefits.

On Windows x64, the BasicMinimalApi using EnableRequestDelegateGenerator=true, the app size goes from:

main: 9.70 MB (10,179,072 bytes)
InvariantGlobalization: 9.48 MB (9,945,600 bytes)

It also appears to help with startup times (linux-x64):

main: Start Time (ms) | 40
InvariantGlobalization: Start Time (ms) | 37

And memory consumption (linux-x64):

main: Working Set (MB) | 47
InvariantGlobalization: Working Set (MB) | 43

cc @DamianEdwards @davidfowl @mangod9 @tarekgh

@richlander
Copy link
Member

If we make this choice, we need to provide guidance on which container image we recommend.

@mkArtakMSFT mkArtakMSFT added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Mar 3, 2023
@DamianEdwards
Copy link
Member

I'm supportive of this, especially doing so initially in the preview cycle so that we can get further feedback. Having the property directly in the project file makes it fairly discoverable too. We should ensure that the MSBuild core schema file includes an entry for this property if it doesn't already, and additionally we should look at getting the VS project property pages UI updated to support this setting this property VS UI too.

@MichalStrehovsky
Copy link
Member

Is there no way to make this pay-for-play automatically? I'm just wondering why we're ending up loading ICU on the startup path. If we're not doing any culture specific things, I would not expect ICU to get loaded at all (and affect startup time/working set).

I would even expect it to be trimmed away, but that one might be just wishful thinking, given I see a KoreanLunisolarCalendar, and various other calendar types even in a fully trimmed hello world app).

@jkotas
Copy link
Member

jkotas commented Mar 6, 2023

There are two metrics to consider for pay-for-play:

Binary footprint pay-for-play

Unfortunately, basic runtime services depend on current user culture. For example, formatting of numbers in exception messages depends on the current thread culture that is initialized to default user culture by default that brings dependency on ICU. It is not feasible to break this dependency without behavior change.

InvariantGlobalization setting introduces such behavior change by disabling everything that has to do with cultures. It may be possible to come up with settings that are a subset of what InvariantGlobalization does, for example a setting that changes the default thread culture to invariant culture. The downside is that it would make the globalization space even more complicated.

I think it makes sense to set InvariantGlobalization in the api template for good footprint and omit ICU in the recommended container base image.

Startup time pay-for-play

You have made a good point that we should not need to load ICU for api startup. It is very easy to dependent on current user culture by mistake. I see that the ICU gets loaded by a culture-specific string.Compare call in event source manifest parsers that looks like a bug. We may want to do some onion peeling to see whether fixing bugs like can move us far enough.

@JamesNK
Copy link
Member

JamesNK commented Mar 7, 2023

I think it's weird to disable this on one template. If it should be disabled on this template, why not on all templates?

The size and startup improvements are pretty minor. I think this kind of setting is better to document on a page that discusses AOT + reducing app deployment size. People who really care, and don't need gloablization, can update their project to use it.

@tarekgh
Copy link
Member

tarekgh commented Mar 7, 2023

This template is for server apps which most likely not need globalization support. I will be concerned if we turn off globalization by default on other templates.

@JamesNK
Copy link
Member

JamesNK commented Mar 7, 2023

Got it. By all templates I was thinking about all server templates

@richlander
Copy link
Member

richlander commented Mar 7, 2023

The size differences are not minor when you factor in container-based deployment.

Our Debian and Ubuntu images contain ICU and our Alpine images do not. Ubuntu Chiseled images also do not.

As both Jan and Tarek suggest, changes to globalization are observable. We do our best to help developers achieve wins where it makes sense. This whole space has a lot of gotchas.

Server templates isn't really a specific enough category, right?

@DamianEdwards
Copy link
Member

We're only talking about disabling it in this one template, specifically the new "api" template. We could also consider disabling it in the existing "webapi" and "worker" templates for the same reasons outlined already (i.e. they're typically not serving content to human readers).

@captainsafia
Copy link
Member

Triage: We'll also want to make sure that the InvariantGlobalization property is documented.

We'll also want to consider enabling this for gRPC templates.

@ghost
Copy link

ghost commented Mar 7, 2023

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@jkotas
Copy link
Member

jkotas commented Mar 7, 2023

We'll also want to make sure that the InvariantGlobalization property is documented.

https://learn.microsoft.com/en-us/dotnet/core/project-sdk/msbuild-props#invariantglobalization is the existing documentation for this property.

@JamesNK
Copy link
Member

JamesNK commented Mar 7, 2023

We'll also want to make sure that the InvariantGlobalization property is documented.

Context: Documenting in msbuild schema so it has VS intellisense and tooltip

JaynieBai pushed a commit to dotnet/msbuild that referenced this issue Mar 9, 2023
Related to dotnet/aspnetcore#47029.

Document the InvariantGlobalization property since we intend to set it in templates.
eerhardt added a commit to eerhardt/aspnetcore that referenced this issue Mar 9, 2023
API server applications do not typically need culture aware behavior. A better default is to use invariant globalization mode for API apps.

This allows for the globalization code to be trimmed away, making the final executable size smaller. It also reduces startup and memory consumption because ICU isn't loaded.

This also allows the apps to run with consistent behavior on container images that don't include ICU, making the container image smaller as well.

Fix dotnet#47029
eerhardt added a commit that referenced this issue Mar 10, 2023
…#47066)

* Set InvariantGlobalization in api, webapi, grpc, and worker templates

API server applications do not typically need culture aware behavior. A better default is to use invariant globalization mode for API apps.

This allows for the globalization code to be trimmed away, making the final executable size smaller. It also reduces startup and memory consumption because ICU isn't loaded.

This also allows the apps to run with consistent behavior on container images that don't include ICU, making the container image smaller as well.

Fix #47029

* Add template tests to ensure InvariantGlobalization is set.
@mangod9
Copy link
Member

mangod9 commented Mar 10, 2023

Nice! Assume after this change there is measurable improvement in startup for both NativeAOT and coreclr?

@eerhardt
Copy link
Member Author

I didn't test coreclr. But you can see the changes for NativeAOT above in the original post.

I've also merged aspnet/Benchmarks#1803 which updates the benchmark apps, so next week we will have official numbers for coreclr and NativeAOT.

jkotas added a commit to jkotas/runtime that referenced this issue Mar 13, 2023
These string comparisons are clearly not meant to be culture-specific. They were one of the reasons for loading ICU in Console.WriteLine("Hello world"); app.

Context dotnet/aspnetcore#47029 (comment)
jkotas added a commit to dotnet/runtime that referenced this issue Mar 13, 2023
* Fix culture-specific string comparisons in EventSource

These string comparisons are clearly not meant to be culture-specific. They were one of the reasons for loading ICU in Console.WriteLine("Hello world"); app.

Context dotnet/aspnetcore#47029 (comment)

* Update src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Apr 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants