[browser][non-icu] `HybridGlobalization` indexing #84471

ilonatommy · 2023-04-07T09:54:10Z

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

Old, icu-based private API: GlobalizationNative_IndexOf, GlobalizationNative_LastIndexOf

New, non-icu private API: IndexOf.

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

CompareInfo.IndexOf
CompareInfo.LastIndexOf
String.IndexOf
String.LastIndexOf

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

cc @SamMonoRT

ghost · 2023-04-07T09:54:23Z

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_IndexOf, GlobalizationNative_LastIndexOf

New, non-icu private API: IndexOf.

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

CompareInfo.IndexOf
CompareInfo.LastIndexOf
String.IndexOf
String.LastIndexOf

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:	-

kg · 2023-04-07T13:51:14Z

Something I should have noticed in previous reviews: In HybridGlobalization right now you pass strings as ptr+length pairs. This means that any time an interned string is passed across the boundary to JS, it has to be decoded to a new JS string that gets thrown away.

If you were to pass MonoString ** (i.e. ref str from C#) instead and use the existing string decoding API in the JS bindings, that API supports interning. This could be significant performance-wise for things like a needle-in-haystack search if the needle is interned, because you would be able to search for i.e. the literal "hello" from a C# source file (literals are interned) a thousand times while only decoding it once. It is a common pattern for either the needle or haystack to be interned. You can access this functionality safely via the conv_string_root API

runtime/src/mono/wasm/runtime/strings.ts

Line 112 in 50c1214

export function conv_string_root(root: WasmRoot<MonoString>): string | null {

paired with an external root.

For cases where you are indexing into the char* buffer one element at a time and never making a temporary JS string, it's fine to not use this. It seems like it would probably be worth applying this to IndexOf/LastIndexOf and comparisons, but case conversion doesn't seem likely to need interning.

ilonatommy · 2023-04-11T08:27:34Z

Something I should have noticed in previous reviews: In HybridGlobalization right now you pass strings as ptr+length pairs. This means that any time an interned string is passed across the boundary to JS, it has to be decoded to a new JS string that gets thrown away.

If you were to pass MonoString ** (i.e. ref str from C#) instead and use the existing string decoding API in the JS bindings, that API supports interning. This could be significant performance-wise for things like a needle-in-haystack search if the needle is interned, because you would be able to search for i.e. the literal "hello" from a C# source file (literals are interned) a thousand times while only decoding it once. It is a common pattern for either the needle or haystack to be interned. You can access this functionality safely via the conv_string_root API

runtime/src/mono/wasm/runtime/strings.ts

Line 112 in 50c1214

export function conv_string_root(root: WasmRoot<MonoString>): string | null {

paired with an external root.
For cases where you are indexing into the char* buffer one element at a time and never making a temporary JS string, it's fine to not use this. It seems like it would probably be worth applying this to IndexOf/LastIndexOf and comparisons, but case conversion doesn't seem likely to need interning.

Thank you, I did not know about interning. The original motivation for using pointers was optimizing memory usage - to avoid creating new "return" strings on JS side when we can update the string allocated by the managed code. I would like to ask @pavelsavara for opinion here because he hinted me to the current approach. Changing the way HybridGlobalization handles strings is not a lot of work and we can do it at any point.

pavelsavara · 2023-04-12T11:52:24Z

I don't remember the details of that conversation anymore. Looking at the PR now, I wonder why we are using icall instead of the new generated interop. With the later, you would get the string interning without any effort and trouble with GC roots.

ilonatommy · 2023-04-12T13:35:41Z

I don't remember the details of that conversation anymore. Looking at the PR now, I wonder why we are using icall instead of the new generated interop. With the later, you would get the string interning without any effort and trouble with GC roots.

That is true but using new Interop inside of System.Private.CoreLib requires referencing System.Runtime.InteropServices.JavaScript and we cannot reference any assembly from CoreLib. That was why new Interop was not used.

pavelsavara · 2023-04-12T13:59:04Z

requires referencing System.Runtime.InteropServices.JavaScript ...

I see. In that case Kate's suggesting is better.

mkhamoyan · 2023-04-12T14:09:15Z

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.WebAssembly.cs

+        /// <returns></returns>
+        // ToDo: clean up with IndexOfOrdinalHelper from .Icu
+        private unsafe int IndexOfOrdinalIgnoreCaseHelperJS(out string  exceptionMessage, ReadOnlySpan<char> source, ReadOnlySpan<char> target, CompareOptions options, int* matchLengthPtr, bool fromBeginning)
+        {


Wouldn't it better to move this function https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs#L95 to CompareInfo.cs and call from ICU and WebAssembly? From first look implementations look identical.

It differs with one goto: InteropCall. I tried to merge these two implementations: icu + wasm by passing the delegate to the InteropCall but unfortunately delegates do not support unsafe parameters (pointers), so the cleanup failed. Generally all CompareInfo should get refactored because it's a huge class and I was thinking about doing it in follow-up, when collations on HybridGlobalization would be already working.

Also I have some concerns about refactoring these functions. Originally, ICU has 2 very similar functions that differ only with lines

if (char.IsAsciiLetterLower(valueChar)) valueChar = (char)(valueChar - 0x20); if (char.IsAsciiLetterLower(targetChar)) targetChar = (char)(targetChar - 0x20);

but they have this comment at the top:

/// Duplicate of IndexOfOrdinalHelper that also handles ignore case. Can't converge both methods /// as the JIT wouldn't be able to optimize the ignoreCase path away.

An attempt to merge Wasm + Icu functions is similar to the situation described. @ViktorHofer, you are the author of the comment, could you help us? Can you think about an effective way of refactoring e.g. IndexOfOrdinalIgnoreCaseHelper and IndexOfOrdinalIgnoreCaseHelperJS into one function, when they differ only with the contents of goto InteropCall instruction?

mkhamoyan · 2023-04-12T14:10:52Z

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.WebAssembly.cs

+            }
+        }
+
+        private unsafe int IndexOfOrdinalHelperJS(out string exceptionMessage, ReadOnlySpan<char> source, ReadOnlySpan<char> target, CompareOptions options, int* matchLengthPtr, bool fromBeginning)


Same for this. Looks like most of the logic can be reused.

ilonatommy · 2023-04-12T15:26:28Z

I see. In that case Kate's suggesting is better.

Planning to first fix it for main (#84695), then here. Converting to draft.

kg · 2023-04-22T21:45:58Z

Minor note (not important): The locale.split("-")[0] === "ja" in compare_strings probably creates gc pressure and could be hurting performance. I wouldn't worry about it much unless split and/or GCs are eating time in profiles though.

You could probably replace it with locale.startsWith("ja-") I think?

ilonatommy · 2023-04-24T07:22:37Z

You could probably replace it with locale.startsWith("ja-") I think?

You are right, startsWith is correct in this context. We are using ~0.1% on Split currently (in comparison, not indexing, this implementation of indexing is very slow and needs to be re-designed). I don't see any impact on the length of compare_string execution when I change the function to startsWith, though.

pavelsavara · 2023-04-24T09:21:12Z

src/mono/wasm/runtime/net6-legacy/hybrid-globalization.ts

+export function segment_string_locale_sensitive(string: string, locale: string | undefined) : Intl.SegmentData[]
+{
+    const segmenter = new Intl.Segmenter(locale, { granularity: "grapheme" });
+    return Array.from(segmenter.segment(string));


Return the enumerator instead. It would be allocating less objects on average, when we find earlier match.

Here: https://github.com/ilonatommy/runtime/tree/hg-indexing-faster. This PR actually should be already closed, not drafted. Sorry if it confused you.

Indexer is stll slow, thoug. For 64kB it's ~2100ms, which is unacceptable. That's why the branch is still WIP and not posted as a PR.

Added support for hybrid globalization indexing.

9aa15b1

ilonatommy added arch-wasm WebAssembly architecture area-System.Globalization labels Apr 7, 2023

ilonatommy requested a review from mkhamoyan April 7, 2023 09:54

ilonatommy requested review from lewing and pavelsavara as code owners April 7, 2023 09:54

ilonatommy self-assigned this Apr 7, 2023

ilonatommy requested a review from kg as a code owner April 7, 2023 09:54

Missing change: Hybrid only for WASM.

4cc51f4

ilonatommy and others added 5 commits April 11, 2023 12:48

Merge branch 'main' into hg-indexing

0c10f2d

Merge branch 'main' into hg-indexing

044df40

Rename after merge.

1a64690

Missing wasi removal.

5b08c5c

Missing rename.

3d4ab8a

build-analysis bot mentioned this pull request Apr 12, 2023

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

Closed

mkhamoyan reviewed Apr 12, 2023

View reviewed changes

ilonatommy mentioned this pull request Apr 12, 2023

[browser][non-icu] HybridGlobalization support interning #84695

Closed

ilonatommy marked this pull request as draft April 12, 2023 15:26

Merge branch 'main' into hg-indexing

18bf1c3

This was referenced Apr 17, 2023

[mono][wasm] System.Numerics.Vectors.Tests assert equal failure #84775

Closed

WasmTestOnBrowser-System.Text.Json.Tests.WorkItemExecution timing out #84434

Closed

Merge branch 'main' into hg-indexing

f4043e3

This was referenced Apr 18, 2023

Tracking issue for CI build timeouts #76454

Closed

nativeaot/SmokeTests/DwarfDump failing on linux-x64 Debug #84979

Closed

Perf improvement, still slow (~5s for 64kB).

3ca12f9

pavelsavara reviewed Apr 24, 2023

View reviewed changes

ilonatommy closed this Apr 24, 2023

ghost locked as resolved and limited conversation to collaborators May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[browser][non-icu] `HybridGlobalization` indexing #84471

[browser][non-icu] `HybridGlobalization` indexing #84471

ilonatommy commented Apr 7, 2023

ghost commented Apr 7, 2023

kg commented Apr 7, 2023 •

edited

Loading

ilonatommy commented Apr 11, 2023

pavelsavara commented Apr 12, 2023

ilonatommy commented Apr 12, 2023 •

edited

Loading

pavelsavara commented Apr 12, 2023

mkhamoyan Apr 12, 2023

ilonatommy Apr 12, 2023

ilonatommy Apr 13, 2023

mkhamoyan Apr 12, 2023

ilonatommy commented Apr 12, 2023

kg commented Apr 22, 2023 •

edited

Loading

ilonatommy commented Apr 24, 2023

pavelsavara Apr 24, 2023 •

edited

Loading

ilonatommy Apr 24, 2023 •

edited

Loading

ilonatommy Apr 24, 2023

[browser][non-icu] HybridGlobalization indexing #84471

[browser][non-icu] HybridGlobalization indexing #84471

Conversation

ilonatommy commented Apr 7, 2023

ghost commented Apr 7, 2023

kg commented Apr 7, 2023 • edited Loading

ilonatommy commented Apr 11, 2023

pavelsavara commented Apr 12, 2023

ilonatommy commented Apr 12, 2023 • edited Loading

pavelsavara commented Apr 12, 2023

mkhamoyan Apr 12, 2023

Choose a reason for hiding this comment

ilonatommy Apr 12, 2023

Choose a reason for hiding this comment

ilonatommy Apr 13, 2023

Choose a reason for hiding this comment

mkhamoyan Apr 12, 2023

Choose a reason for hiding this comment

ilonatommy commented Apr 12, 2023

kg commented Apr 22, 2023 • edited Loading

ilonatommy commented Apr 24, 2023

pavelsavara Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

ilonatommy Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

ilonatommy Apr 24, 2023

Choose a reason for hiding this comment

[browser][non-icu] `HybridGlobalization` indexing #84471

[browser][non-icu] `HybridGlobalization` indexing #84471

kg commented Apr 7, 2023 •

edited

Loading

ilonatommy commented Apr 12, 2023 •

edited

Loading

kg commented Apr 22, 2023 •

edited

Loading

pavelsavara Apr 24, 2023 •

edited

Loading

ilonatommy Apr 24, 2023 •

edited

Loading