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][non-icu] HybridGlobalization compare #84249

Merged
merged 10 commits into from Apr 7, 2023

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Apr 3, 2023

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

Old, icu-based private API: GlobalizationNative_CompareString

New, non-icu private API: CompareString.

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

  • CompareInfo.Compare,
  • String.Compare,
  • String.Equals.

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

Test name time ICU4C [ms] time HG [ms] increase by [times]
String, String Compare 0.0448 5.029 112
String, String Equals 0.0450 4.9587 110
String, CompareInfo Compare 0.0451 4.9238 109

see final performance here: #85098
cc @SamMonoRT

Performance:
most of the processing time is taken by mono_wasm_compare_string (~60-70%) while ~50% is used for TextDecoder.decode and only up to 20% for localeCompare.

@ilonatommy ilonatommy self-assigned this Apr 3, 2023
@ghost
Copy link

ghost commented Apr 3, 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.

Requires #84019 to be merged first (it has changes introducing HybridGlobalization mode).

Old, icu-based private API: GlobalizationNative_CompareString

New, non-icu private API: CompareStringJS.

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

  • CompareInfo.Compare,
  • String.Compare.

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

cc @SamMonoRT

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-System.Globalization

Milestone: -

@ilonatommy ilonatommy changed the title [browser][non-icu] HybridGlobalization compare [browser][non-icu] HybridGlobalization compare Apr 4, 2023
@ilonatommy ilonatommy marked this pull request as ready for review April 5, 2023 09:13
@ilonatommy
Copy link
Member Author

Failures: Blazor WasmBuildTests and AOT timeouts are not connected.

@ilonatommy ilonatommy requested a review from tarekgh April 5, 2023 09:14
hiraganaBig.localeCompare(katakanaSmall, "en-US", { sensitivity: "base" }) // 0; base: a ≠ b, a = á, a = A
```

- List of all `CompareOptions` combinations always throwing `PlatformNotSupportedException`:
Copy link
Contributor

Choose a reason for hiding this comment

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

In the future, could we use attributes or a compile time analyzer to make builds fail when the user's code attempts to use these combinations? (Obviously not in this PR)

Copy link
Member Author

@ilonatommy ilonatommy Apr 5, 2023

Choose a reason for hiding this comment

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

I think we could but I am not sure if I know how to do it. Do we have a similar mechanism somewhere already?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, maybe ask in the group chat or channel. I haven't touched any of our generators/analyzers but I know we have a couple like the JSImport/Export generator.

@ilonatommy
Copy link
Member Author

Failures not related. Merging to move forward. If there are any new ideas later, feel free to let me know and we will solve them in a follow-up, cc @pavelsavara, @radical.

@ilonatommy ilonatommy merged commit d34a2a2 into dotnet:main Apr 7, 2023
168 of 175 checks passed
@dotnet dotnet locked as resolved and limited conversation to collaborators May 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants