Initial changes for globalization hybrid mode #81470

mkhamoyan · 2023-02-01T10:47:47Z

Implemented

GlobalizationNative_GetLocaleInfoStringNative

LocaleStringData

Changes for #80689

ghost · 2023-02-01T10:48:01Z

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Implemented

GlobalizationNative_GetLocaleInfoString

  NSLocale
  LocaleString_LocalizedDisplayName => localizedStringForLocaleIdentifier
  LocaleString_EnglishDisplayName => localeIdentifier / localizedStringForLocaleIdentifier?
  LocaleString_NativeDisplayName => localeIdentifier / localizedStringForLocaleIdentifier?
  LocaleString_LocalizedLanguageName => localizedStringForLanguageCode
  LocaleString_EnglishLanguageName => localizedStringForLanguageCode
  LocaleString_NativeLanguageName => localizedStringForLanguageCode
  LocaleString_EnglishCountryName => localizedStringForCountryCode
  LocaleString_NativeCountryName => localizedStringForCountryCode
  LocaleString_DecimalSeparator => decimalSeparator
  LocaleString_ThousandSeparator => groupingSeparator

GlobalizationNative_GetLocaleName

Changes for #80689

Author:	mkhamoyan
Assignees:	mkhamoyan
Labels:	`area-System.Globalization`
Milestone:	-

ghost · 2023-02-01T16:29:39Z

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

Implemented

GlobalizationNative_GetLocaleInfoString

  NSLocale
  LocaleString_LocalizedDisplayName => localizedStringForLocaleIdentifier
  LocaleString_EnglishDisplayName => localeIdentifier / localizedStringForLocaleIdentifier?
  LocaleString_NativeDisplayName => localeIdentifier / localizedStringForLocaleIdentifier?
  LocaleString_LocalizedLanguageName => localizedStringForLanguageCode
  LocaleString_EnglishLanguageName => localizedStringForLanguageCode
  LocaleString_NativeLanguageName => localizedStringForLanguageCode
  LocaleString_EnglishCountryName => localizedStringForCountryCode
  LocaleString_NativeCountryName => localizedStringForCountryCode
  LocaleString_DecimalSeparator => decimalSeparator
  LocaleString_ThousandSeparator => groupingSeparator

GlobalizationNative_GetLocaleName

Changes for #80689

Author:	mkhamoyan
Assignees:	mkhamoyan
Labels:	`area-System.Globalization`, `os-ios`
Milestone:	-

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml

docs/workflow/trimming/feature-switches.md

src/tasks/AppleAppBuilder/Templates/runtime.m

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.cs

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.iOS.cs

src/native/libs/System.Globalization.Native/pal_locale.m

jkotas · 2023-03-13T18:40:20Z

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.cs

-
-            return GlobalizationMode.UseNls ? NlsGetLocaleInfo(type) : IcuGetLocaleInfo(type, uiCultureName);
+#if TARGET_IOS
+            return GlobalizationMode.UseNls ? NlsGetLocaleInfo(type)


GlobalizationMode.UseNls is always going to be false on iOS. It is Windows-specific property. You can omit it for iOS-specific code.

jkotas · 2023-03-13T18:42:00Z

src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs

@@ -13,13 +13,15 @@ internal static partial class GlobalizationMode
        private static partial class Settings
        {
            internal static bool Invariant { get; } = AppContextConfigHelper.GetBooleanConfig("System.Globalization.Invariant", "DOTNET_SYSTEM_GLOBALIZATION_INVARIANT");
+            internal static bool Hybrid { get; } = AppContextConfigHelper.GetBooleanConfig("System.Globalization.Hybrid", "DOTNET_SYSTEM_GLOBALIZATION_HYBRID");


Should this be under iOS ifdef - since all uses are under iOS ifdef as well?

Yes, plus wasm and wasi as well.

For now I will put under ioslike platforms. For wasm we have seperate PR.

jkotas · 2023-03-13T18:51:36Z

src/native/libs/System.Globalization.Native/pal_locale.h

@@ -9,6 +9,8 @@ PALEXPORT int32_t GlobalizationNative_GetLocales(UChar *value, int32_t valueLeng

 PALEXPORT int32_t GlobalizationNative_GetLocaleName(const UChar* localeName, UChar* value, int32_t valueLength);

+PALEXPORT const char* NativeGetLocaleName(const char* localeName, int32_t valueLength);


This should still have GlobalizationNative_ prefix to follow the coding conventions.

https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/interop-guidelines.md#unix-shims: all exports should have a prefix that corresponds to the Libraries' name, e.g. "SystemNative_" or "CryptoNative_"

mkhamoyan · 2023-03-20T16:50:35Z

/azp run runtime-ioslike

azure-pipelines · 2023-03-20T16:50:44Z

Azure Pipelines successfully started running 1 pipeline(s).

mdh1418

If the expectation is that GlobalizationMode.Invariant and GlobalizationMode.Hybrid cannot both be true, do we have some measures to check that both feature flags are not both true, and throw somewhere if they are?

src/libraries/Common/src/Interop/Interop.Locale.OSX.cs

src/libraries/System.Globalization/tests/Hybrid/Hybrid.Tests.csproj

src/libraries/System.Globalization/tests/Hybrid/HybridMode.cs

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.OSX.cs

mkhamoyan · 2023-03-21T11:48:51Z

If the expectation is that GlobalizationMode.Invariant and GlobalizationMode.Hybrid cannot both be true, do we have some measures to check that both feature flags are not both true, and throw somewhere if they are?

For now there are checks in CultureData.OSX.cs like Debug.Assert(!GlobalizationMode.Invariant);
Once more functionalities will be added in HybridMode we can add more checks or add more measures.

steveisok · 2023-03-21T13:25:53Z

If the expectation is that GlobalizationMode.Invariant and GlobalizationMode.Hybrid cannot both be true, do we have some measures to check that both feature flags are not both true, and throw somewhere if they are?

For now there are checks in CultureData.OSX.cs like Debug.Assert(!GlobalizationMode.Invariant); Once more functionalities will be added in HybridMode we can add more checks or add more measures.

If invariant mode is on, the runtime will not load ICU at all. There should not be a scenario where if invariant mode is true and hybrid is true that they somehow collide.

mkhamoyan · 2023-03-21T14:15:05Z

/azp run runtime-ioslike

azure-pipelines · 2023-03-21T14:15:19Z

Azure Pipelines successfully started running 1 pipeline(s).

steveisok · 2023-03-21T15:01:53Z

@mdh1418 @akoeplinger Please give this another pass when you have a moment.

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.OSX.cs

mdh1418

Overall looking good to me. Just some things we should do before merging
Double check ILLink trimming ILLink.Substitutions.Shared.xml is behaving as anticipated in the different feature switch scenarios. Or if someone more familiar with that signs off on it
The comment about _sWindowsName.

mkhamoyan · 2023-03-24T14:11:34Z

/azp run runtime-ioslike

azure-pipelines · 2023-03-24T14:11:42Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-03-27T08:14:47Z

/azp run runtime-ioslike

azure-pipelines · 2023-03-27T08:14:59Z

Azure Pipelines successfully started running 1 pipeline(s).

mdh1418 · 2023-03-27T18:35:55Z

docs/workflow/trimming/feature-switches.md

@@ -13,6 +13,7 @@ configurations but their defaults might vary as any SDK can set the defaults dif
 | EnableUnsafeBinaryFormatterSerialization | System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization | BinaryFormatter serialization support is trimmed when set to false |
 | EventSourceSupport | System.Diagnostics.Tracing.EventSource.IsSupported | Any EventSource related code or logic is trimmed when set to false |
 | InvariantGlobalization | System.Globalization.Invariant | All globalization specific code and data is trimmed when set to true |
+| HybridGlobalization | System.Globalization.Hybrid |  Loading ICU + native implementations |


Are we still planning to add the feature switch HybridGlobalization? Reading up on https://github.com/dotnet/designs/blob/main/accepted/2020/feature-switch.md a bit more, it seems like feature switches primarily help control the size of libraries by trimming out areas we know will not be reached under certain conditions (when a particular feature switch is on). For example, when the InvariantGlobalization feature switch is on (true), the GlobalizationMode/Settings class is trimmed out.

So for HybridGlobalization, is there anything we want to/can trim out if its known that HybridGlobalization is being used? From what I am understanding, HybridGlobalization is to help reduce the reliance on ICU. When its true, it doesn't make any other code completely unreachable right now does it? Or is IcuGetLocaleInfo(..., LocalStringData ..., ...) all supposed to be unreachable now?

We need to have feature switch HybridGlobalization as without it we will not understand to load fully icu or no. In ideal case IcuGetLocaleInfo should not be used in HybridGlobalization when all functionalities will be implemented by native functions. Also for HybridGlobalization we will load smaller icu files (trimmed out the functionalities that are implemented by native functions), this will be done in upcoming PRs.

@mkhamoyan the feature switches listed here are linker feature switches, i.e. where the linker replaces/hardcodes the value of the corresponding AppContext switch so that unused code can be trimmed out.

We don't need that, we have a regular AppContext switch that is unaffected by trimming so having it listed in this doc is confusing.

Will remove it from here. Do we have doc for regular AppContext switches?

As far as I know we just have the public docs where we could add it: https://learn.microsoft.com/en-us/dotnet/core/runtime-config/globalization

Note that in our case we'd just have runtimeconfig.json and environment variable options since the MSBuild property would need wiring up in the dotnet/sdk.

mdh1418 · 2023-03-27T18:51:53Z

src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs

@@ -13,13 +13,19 @@ internal static partial class GlobalizationMode
        private static partial class Settings
        {
            internal static bool Invariant { get; } = AppContextConfigHelper.GetBooleanConfig("System.Globalization.Invariant", "DOTNET_SYSTEM_GLOBALIZATION_INVARIANT");
+#if TARGET_OSX || TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS
+            internal static bool Hybrid { get; } = AppContextConfigHelper.GetBooleanConfig("System.Globalization.Hybrid", "DOTNET_SYSTEM_GLOBALIZATION_HYBRID");


If HybridGlobalization is not going to be a feature switch, should this be using AppContextConfigHelper.GetBooleanConfig? Will the switch System.Globalization.Hybrid ever be set? I think System.Globalization.Invariant and System.Globalization.PredefinedCulturesOnly switches are probably set in the TrimmingTests

runtime/src/libraries/System.Runtime/tests/TrimmingTests/System.Runtime.TrimmingTests.proj

Lines 16 to 19 in 9b38f2a

<DisabledFeatureSwitches>System.Globalization.Invariant</DisabledFeatureSwitches>

</TestConsoleAppSourceFiles>

<TestConsoleAppSourceFiles Include="InvariantGlobalizationTrue.cs">

<EnabledFeatureSwitches>System.Globalization.Invariant;System.Globalization.PredefinedCulturesOnly</EnabledFeatureSwitches>

.

Once more functionalities will be implemented this will be used and we can have tests in TrimmingTests for hybrid mode.

mkhamoyan · 2023-03-28T08:07:29Z

Test failures are not related to the PR.

Initial changes for GetLocaleInfoString

f8e72d8

dotnet-issue-labeler bot added the area-System.Globalization label Feb 1, 2023

ghost assigned mkhamoyan Feb 1, 2023

Remove CompareInfo changes

a82743d

mkhamoyan requested review from steveisok and akoeplinger February 1, 2023 11:03

Updated more cases

954ed0f

tarekgh added the os-ios Apple iOS label Feb 1, 2023

mkhamoyan added 12 commits February 8, 2023 10:51

updated hybrid mode property

1362d45

update

cb23688

working version of NativeGetLocaleInfoString

af32324

update

83f4e7b

Refactored and added more cases

4ac3e6b

Removed comments

60e090a

updated

26ce9a9

Merge branch 'main' into ios_hybrid_globalization

a10f09d

native functions only for apple platforms

dc347f5

Native functions only for apple

09d599e

build native functions only for ios

bacc227

Include ios file only for ios

e844a40

mdh1418 reviewed Mar 2, 2023

View reviewed changes

This was referenced Mar 2, 2023

Tracking issue for CI build timeouts #76454

Closed

IOException running NuGet-Migrations during tests in dotnet CLI first run #80619

Closed

jkotas reviewed Mar 13, 2023

View reviewed changes

mkhamoyan added 2 commits March 15, 2023 16:07

Merge branch 'main' into ios_hybrid_globalization

f7f730a

refactored

46c9e68

Remove logging

ad4c98b

mdh1418 reviewed Mar 20, 2023

View reviewed changes

Removed not used code lines

3eb5cae

build-analysis bot mentioned this pull request Mar 21, 2023

Arm32: System.Threading.Tasks.Dataflow.Tests failing with NRE #80857

Closed

mkhamoyan marked this pull request as ready for review March 21, 2023 14:10

mkhamoyan requested a review from marek-safar as a code owner March 21, 2023 14:10

mdh1418 reviewed Mar 21, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.OSX.cs Show resolved Hide resolved

mdh1418 approved these changes Mar 21, 2023

View reviewed changes

build-analysis bot mentioned this pull request Mar 21, 2023

[wasm] SimpleSourceChangeRebuildTest.SimpleStringChangeInSource - Long running test error, cannot access file used by another process #83752

Closed

Remove from linker hybrid mode

27424bf

mdh1418 reviewed Mar 27, 2023

View reviewed changes

mkhamoyan changed the title ~~Initial changes for GetLocaleInfoString~~ Initial changes for globalization hybrid mode Mar 28, 2023

mkhamoyan merged commit f561a05 into main Mar 28, 2023

jkotas deleted the ios_hybrid_globalization branch March 28, 2023 10:32

mkhamoyan mentioned this pull request Mar 29, 2023

Remove HybridGlobalization info from linker feature switches #84073

Merged

mkhamoyan mentioned this pull request Apr 6, 2023

[iOS][globalization][icu] Tracking issue for HybridGlobalization: Native API + ICU #80689

Closed

23 tasks

ghost locked as resolved and limited conversation to collaborators Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial changes for globalization hybrid mode #81470

Initial changes for globalization hybrid mode #81470

mkhamoyan commented Feb 1, 2023 •

edited by eiriktsarpalis

Loading

ghost commented Feb 1, 2023

ghost commented Feb 1, 2023

jkotas Mar 13, 2023

jkotas Mar 13, 2023

steveisok Mar 13, 2023

mkhamoyan Mar 16, 2023

jkotas Mar 13, 2023

mkhamoyan commented Mar 20, 2023

azure-pipelines bot commented Mar 20, 2023

mdh1418 left a comment

mkhamoyan commented Mar 21, 2023

steveisok commented Mar 21, 2023

mkhamoyan commented Mar 21, 2023

azure-pipelines bot commented Mar 21, 2023

steveisok commented Mar 21, 2023

mdh1418 left a comment

mkhamoyan commented Mar 24, 2023

azure-pipelines bot commented Mar 24, 2023

mkhamoyan commented Mar 27, 2023

azure-pipelines bot commented Mar 27, 2023

mdh1418 Mar 27, 2023

mkhamoyan Mar 28, 2023

akoeplinger Mar 28, 2023 •

edited

Loading

mkhamoyan Mar 29, 2023

akoeplinger Mar 29, 2023 •

edited

Loading

mdh1418 Mar 27, 2023

mkhamoyan Mar 28, 2023

mkhamoyan commented Mar 28, 2023

		@@ -9,6 +9,8 @@ PALEXPORT int32_t GlobalizationNative_GetLocales(UChar *value, int32_t valueLeng

		PALEXPORT int32_t GlobalizationNative_GetLocaleName(const UChar* localeName, UChar* value, int32_t valueLength);

		PALEXPORT const char* NativeGetLocaleName(const char* localeName, int32_t valueLength);

	<DisabledFeatureSwitches>System.Globalization.Invariant</DisabledFeatureSwitches>
	</TestConsoleAppSourceFiles>
	<TestConsoleAppSourceFiles Include="InvariantGlobalizationTrue.cs">
	<EnabledFeatureSwitches>System.Globalization.Invariant;System.Globalization.PredefinedCulturesOnly</EnabledFeatureSwitches>

Initial changes for globalization hybrid mode #81470

Initial changes for globalization hybrid mode #81470

Conversation

mkhamoyan commented Feb 1, 2023 • edited by eiriktsarpalis Loading

ghost commented Feb 1, 2023

ghost commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhamoyan commented Mar 20, 2023

azure-pipelines bot commented Mar 20, 2023

mdh1418 left a comment

Choose a reason for hiding this comment

mkhamoyan commented Mar 21, 2023

steveisok commented Mar 21, 2023

mkhamoyan commented Mar 21, 2023

azure-pipelines bot commented Mar 21, 2023

steveisok commented Mar 21, 2023

mdh1418 left a comment

Choose a reason for hiding this comment

mkhamoyan commented Mar 24, 2023

azure-pipelines bot commented Mar 24, 2023

mkhamoyan commented Mar 27, 2023

azure-pipelines bot commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoeplinger Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoeplinger Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhamoyan commented Mar 28, 2023

mkhamoyan commented Feb 1, 2023 •

edited by eiriktsarpalis

Loading

akoeplinger Mar 28, 2023 •

edited

Loading

akoeplinger Mar 29, 2023 •

edited

Loading