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.

Sign up for GitHub

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

Jump to bottom

[browser][non-icu] HybridGlobalization SortKey #84621

Merged
merged 9 commits into from
Apr 18, 2023
Merged

[browser][non-icu] HybridGlobalization SortKey #84621

merged 9 commits into from
Apr 18, 2023
+139 −71

Conversation

ilonatommy
Copy link
Member

Implements a chunk of web-api based globalization. Is a part of HybridGlobalization feature and contributes to #79989.

Old, icu-based private API: GlobalizationNative_GetSortKey

New, non-icu private API: none, Web API does not expose SortKey. We throw PlatformNotSupportedException.

Affected public API (see: tests in CompareInfoTests.cs):

  • CompareInfo.GetSortKey
  • CompareInfo.GetSortKeyLength
  • CompareInfo.GetHashCode

All changes in behavior are listed in docs\design\features\hybrid-globalization.md

This PR also refactors CompareInfo tests - it moves common properties/fields to CompareInfoTestsBase.

cc @SamMonoRT

@ilonatommy ilonatommy requested review from kg, tarekgh and mkhamoyan April 11, 2023 11:30
@ilonatommy ilonatommy self-assigned this Apr 11, 2023
@ghost
Copy link

ghost commented Apr 11, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Implements a chunk of web-api based globalization. Is a part of HybridGlobalization feature and contributes to #79989.

Old, icu-based private API: GlobalizationNative_GetSortKey

New, non-icu private API: none, Web API does not expose SortKey. We throw PlatformNotSupportedException.

Affected public API (see: tests in CompareInfoTests.cs):

  • CompareInfo.GetSortKey
  • CompareInfo.GetSortKeyLength
  • CompareInfo.GetHashCode

All changes in behavior are listed in docs\design\features\hybrid-globalization.md

This PR also refactors CompareInfo tests - it moves common properties/fields to CompareInfoTestsBase.

cc @SamMonoRT

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-System.Globalization
Milestone: -

@ghost
Copy link

ghost commented Apr 11, 2023

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

Issue Details

Implements a chunk of web-api based globalization. Is a part of HybridGlobalization feature and contributes to #79989.

Old, icu-based private API: GlobalizationNative_GetSortKey

New, non-icu private API: none, Web API does not expose SortKey. We throw PlatformNotSupportedException.

Affected public API (see: tests in CompareInfoTests.cs):

  • CompareInfo.GetSortKey
  • CompareInfo.GetSortKeyLength
  • CompareInfo.GetHashCode

All changes in behavior are listed in docs\design\features\hybrid-globalization.md

This PR also refactors CompareInfo tests - it moves common properties/fields to CompareInfoTestsBase.

cc @SamMonoRT

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-System.Globalization
Milestone: -

@pavelsavara
Copy link
Member

What are the size wins ?
What are the use-cases which would be affected by this change ?

@ilonatommy
Copy link
Member Author

What are the size wins ? What are the use-cases which would be affected by this change ?

Size win:
lack of SortKey is a side effect of removing collations data from ICU. Overall size decrease on full WASM ICU file (icudt.dat) ~40% (329770 bytes -> 202330 bytes on brotli compressed).

Use-cases (only of changes from this PR, not the whole collation):

  • CompareInfo.GetSortKey
  • CompareInfo.GetSortKeyLength
  • CompareInfo.GetHashCode

@pavelsavara
Copy link
Member

(329770 bytes -> 202330 bytes on brotli compressed).

Thanks

Use-cases (only of changes from this PR, not the whole collation):

* CompareInfo.GetSortKey
* CompareInfo.GetSortKeyLength
* CompareInfo.GetHashCode

I mean what are the use-cases where those APIs are useful ?

This is what I tried https://github.com/search?q=CompareInfo.GetSortKey+lang%3AC%23+&type=code&p=3
I see mostly database oriented libraries.

Would it be better if we provide bitwise/invariant sortkey instead of PNSE ? Just an idea.

@ilonatommy
Copy link
Member Author

I mean what are the use-cases where those APIs are useful ?

This is what I tried https://github.com/search?q=CompareInfo.GetSortKey+lang%3AC%23+&type=code&p=3 I see mostly database oriented libraries.

Sorry, I did not get it. It seems you are right, mostly DBs.

Would it be better if we provide bitwise/invariant sortkey instead of PNSE ? Just an idea.

It is not a bad idea but I think if we make it a rule for all locales, it could be confusing. User that calls e.g. new CultureInfo("de-DE").CompareInfo.GetSortKey() expects locale-sensitive sort key.
If they need invariant SortKey, they can switch on InvariantGlobalization and when they need locale-sensitive, functional SortKey, they should not use HybridGlobalization.
I can think about a use-case when your solution would be perfectly useful:

  • the user needs to cut down on runtime size (HybridGlobalization on)
  • they use sort key for locale-insensitive case, e.g.: CultureInfo.InvariantCulture.CompareInfo.GetSortKey()
  • but do not want to switch on InvariantGlobalization becuse they need locale-sensitive operations somewhere else.

I will stash this idea in the tracking issue in case there is a demand on the above case.

@ilonatommy ilonatommy mentioned this pull request Apr 12, 2023
Closed
23 tasks
@ilonatommy ilonatommy requested a review from pavelsavara April 17, 2023 06:17
@build-analysis build-analysis bot mentioned this pull request Apr 17, 2023
Closed
pavelsavara
pavelsavara approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@pavelsavara pavelsavara left a comment

Choose a reason for hiding this comment

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

LGMT

What is our plan for Android/iOS where maybe Hybrid could support these methods ?

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs Outdated
@@ -1599,5 +1615,9 @@ public SortVersion Version
}

public int LCID => CultureInfo.GetCultureInfo(Name).LCID;

#if TARGET_BROWSER
private static string GetPNSEText(string funcName) => $"{funcName} is not supported when HybridGlobalization=true. Disable it to load larger ICU bundle, then use this option.";
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in resource file and localized ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I can see all exception text in PrivateCoreLib are stored like this, so you are right.

@ilonatommy
Copy link
Member Author

What is our plan for Android/iOS where maybe Hybrid could support these methods ?

Android might have an invariant equivalent, it's dedicated for LDAP sorting: https://docs.oracle.com/javase/7/docs/api/javax/naming/ldap/SortKey.html. I do not see any culture-related SortKey.
For iOS I cannot find an equivalent, @mkhamoyan do you know about any?
Even if Hybrid SortKey will be supported on both of them, then PNSE will be left only for WASM.

@runfoapp runfoapp bot mentioned this pull request Apr 17, 2023
Closed
@ilonatommy ilonatommy merged commit 907cc1c into dotnet:main Apr 18, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pavelsavara pavelsavara pavelsavara approved these changes

@kg kg Awaiting requested review from kg

@tarekgh tarekgh Awaiting requested review from tarekgh

@mkhamoyan mkhamoyan Awaiting requested review from mkhamoyan

Assignees

@ilonatommy ilonatommy

Labels
arch-wasm WebAssembly architecture area-System.Globalization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ilonatommy @pavelsavara