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

[browser] Revert to full NativeName by interop with JS #99956

Merged
merged 18 commits into from Mar 26, 2024

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Mar 19, 2024

Fixes #44739, #45951.

Reason for this PR:

For size savings we removed lang_tree and region_tree from ICU WASM filters. We were returning a short form of NativeName and DisplayName for ~200 supported locales in the browser, e.g. en (US). After experimenting with HybridGlobalization we proved that we can get the missing data from JS to patch the regression caused by the size reduction. Now, NativeName and DisplayName will be fetched from WebAPI Intl.DisplayNames. Other Globalization APIs (other LocaleStringData types) continue to be fetched from ICU.

Logic change

Without this PR:

We ask ICU about NativeDisplayName but it does not have this info and returns empty data. In

nativeDisplayName = NativeLanguageName + " (" + NativeCountryName + ")";
we construct the backup response on the managed side.
Similar mechanism is used for LocalizedDisplayName.

new CultureInfo("zh-Hans").EnglishName -> "zh-Hans"

With this PR:

For all LocaleStringData types but NativeDisplayName, EnglishDisplayName and LocalizedDisplayName we continue fetching the data from ICU. In these three cases we call to JS.

Fixed APIs:
CultureInfo.NativeName, CultureInfo.EnglishName, CultureInfo.DisplayName, RegionInfo.NativeName, RegionInfo.EnglishName, RegionInfo.DisplayName.
They depend on ECMAScript's Intl API, so some responses might differ from ICU responses, e.g. new CultureInfo("zh-Hans").EnglishName -> "Simplified Chinese", not "Chinese (Simplified)".

Result:

Thread.CurrentThread.CurrentCulture = new CultureInfo("zh-Hans-CN");
var native = Thread.CurrentThread.CurrentCulture.NativeName;
Console.WriteLine(native); // Simplified Chinese (China)'

Performance impact

The CultureInfo constructor call interops with JS / ICU API, the next calls to English/Native properties rely on cached value. Measuring performance for NativeName and DisplayName:

Without this PR:

  |                     String, NativeName |     0.3859us |
  |                    String, DisplayName |     0.3859us |

With this PR:

  |                     String, NativeName |     0.5079us |
  |                    String, DisplayName |     0.5039us |

Multithreaded runtime still uses the truncated data version, fetching data from JS fails there see: #98483

System.Globalization.Tests.CompareInfoTests.GetCompareInfo(name: "zh-Hans")
[21:06:32] info: System.TypeInitializationException : Exception of type 'System.TypeInitializationException' was thrown.
[21:06:32] info:    at System.Object.InvokeStub_CompareInfoTests..ctor(Object , Object , IntPtr* )
[21:06:32] info:    at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

 and DisplayName for browser.
@ilonatommy ilonatommy added arch-wasm WebAssembly architecture area-System.Globalization os-browser Browser variant of arch-wasm labels Mar 19, 2024
@ilonatommy ilonatommy self-assigned this Mar 19, 2024
@ilonatommy ilonatommy changed the title Fix NativeName [browser] Revert to full NativeName by interop with JS Mar 19, 2024
Copy link
Member

@mkhamoyan mkhamoyan left a comment

Choose a reason for hiding this comment

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

These test cases also need to be updated for browser (there can be even more)

if (PlatformDetection.IsNotUsingLimitedCultures || PlatformDetection.IsAndroid || PlatformDetection.IsHybridGlobalizationOnApplePlatform)
{
yield return new object[] { "GB", "United Kingdom" };
yield return new object[] { "SE", "Sverige" };
yield return new object[] { "FR", "France" };
}
else
{
// Browser's ICU doesn't contain RegionInfo.NativeName
yield return new object[] { "GB", "GB" };
yield return new object[] { "SE", "SE" };
yield return new object[] { "FR", "FR" };
}

if (PlatformDetection.IsNotUsingLimitedCultures || PlatformDetection.IsAndroid || PlatformDetection.IsHybridGlobalizationOnApplePlatform)
{
yield return new object[] { "en-US", "English (United States)" };
yield return new object[] { "en-CA", "English (Canada)" };
yield return new object[] { "en-GB", "English (United Kingdom)" };
}
else
{
// Mobile / Browser ICU doesn't contain CultureInfo.NativeName
yield return new object[] { "en-US", "en (US)" };
yield return new object[] { "en-CA", "en (CA)" };
}

…CultureData.Browser.cs

Co-authored-by: Meri Khamoyan <96171496+mkhamoyan@users.noreply.github.com>
@ilonatommy
Copy link
Member Author

ilonatommy commented Mar 20, 2024

These test cases also need to be updated for browser (there can be even more)

Awesome, thank you. I think we can simplify even the condition. Currently tests are checking for PlatformDetection.IsNotUsingLimitedCultures || PlatformDetection.IsAndroid || PlatformDetection.IsHybridGlobalizationOnApplePlatform to expect the long-form names.

IsNotUsingLimitedCultures == !IsMobile == !(IsBrowser || IsWasi || IsAppleMobile || IsAndroid)
IsAppleMobile == IsMacCatalyst || IsiOS || IstvOS
IsHybridGlobalizationOnApplePlatform == IsHybrid && ( IsAppleMobile )
Then, in this PR we can simplify the condition to !IsWasi || IsHybridGlobalizationOnApplePlatform to expect long-form names.

@mkhamoyan
Copy link
Member

Awesome, thank you. I think we can simplify even the condition. Currently tests are checking for PlatformDetection.IsNotUsingLimitedCultures || PlatformDetection.IsAndroid || PlatformDetection.IsHybridGlobalizationOnApplePlatform to expect the long-form names.

IsNotUsingLimitedCultures == !IsMobile == !(IsBrowser || IsWasi || IsAppleMobile || IsAndroid) IsAppleMobile == IsMacCatalyst || IsiOS || IstvOS IsHybridGlobalizationOnApplePlatform == IsHybrid && ( IsAppleMobile ) Then, in this PR we can simplify the condition to !IsWasi || IsHybridGlobalizationOnApplePlatform to expect long-form names.

Yes, that sounds great. I guess we can modify IsNotUsingLimitedCultures and IsUsingLimitedCultures as in several places we are checking for PlatformDetection.IsNotUsingLimitedCultures || PlatformDetection.IsAndroid || PlatformDetection.IsHybridGlobalizationOnApplePlatform

Copy link
Member

@matouskozak matouskozak left a comment

Choose a reason for hiding this comment

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

LGTM!

@ilonatommy ilonatommy requested a review from maraf March 26, 2024 07:53
@ilonatommy ilonatommy merged commit 0c30a1b into dotnet:main Mar 26, 2024
159 of 161 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Globalization os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NativeName on CurrentCulture is shorten on Blazor WebAssembly .NET 5.0
5 participants