Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 5.6k
[browser][HybridGlobalization] Improve speed performance of IndexOf and LastIndexOf text APIs with HybridGlobalization mode - #95583
#95583Merged
Merged
[browser][HybridGlobalization] Improve speed performance of IndexOf and LastIndexOf text APIs with HybridGlobalization mode#95583
Conversation
matouskozak
commented
Dec 4, 2023 •
edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Member
ghost
commented
Dec 4, 2023
Tagging subscribers to this area: @dotnet/area-system-globalization Issue DetailsDescriptionIn this PR, we improve the time performance of This is done by picking up necessary parts from the previously used This improvement introduces a regression in raw size of ~21kB due to grapheme segmentation rules presented in src/mono/wasm/runtime/hybrid-globalization/segmentation-rules.ts. IdeasBy introducing our grapheme segmentation on the TypeScript side, we are duplicating code already present in .NET under the name
Contributes to: #94473 Note: I don't have much experience with TypeScript, so feel free to change any parts that don't obey style conventions.
|
ghost
commented
Dec 4, 2023
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsDescriptionIn this PR, we improve the time performance of This is done by picking up necessary parts from the previously used This improvement introduces a regression in raw size of ~21kB due to grapheme segmentation rules presented in src/mono/wasm/runtime/hybrid-globalization/segmentation-rules.ts. IdeasBy introducing our grapheme segmentation on the TypeScript side, we are duplicating code already present in .NET under the name
Contributes to: #94473 Note: I don't have much experience with TypeScript, so feel free to change any parts that don't obey style conventions.
|
pavelsavara
commented
Dec 4, 2023
Member
2 more ideas:
|
ilonatommy
commented
Dec 4, 2023 •
edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Member
Update: 21kB before brotli compression. After compression, 5kB (that is ~1.5% of the original |
This was referenced Dec 4, 2023
matouskozak
commented
Dec 4, 2023
MemberAuthor
That sounds good: |
matouskozak
marked this pull request as ready for review
December 5, 2023 09:14
kg
reviewed
Dec 5, 2023
Uh oh!
There was an error while loading. Please reload this page.
kg
reviewed
Dec 5, 2023
Uh oh!
There was an error while loading. Please reload this page.
kg
reviewed
Dec 5, 2023
Uh oh!
There was an error while loading. Please reload this page.
matouskozak
commented
Jan 12, 2024
MemberAuthor
/azp run runtime-wasm |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| } | ||
| IEnumerable<string> actual = Directory.EnumerateFiles(assertOptions.BinFrameworkDir, "icudt*dat"); | ||
| if (assertOptions.GlobalizationMode == GlobalizationMode.Hybrid) |
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why actual is not all files ? Like *.* ? @radical
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because are only looking for the icu related files here, and we do throw if the number of expected!=actual .
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now that we have a differently named file too, it would indeed make sense to enumerate all the files ignoring subdirectories, and AssertFilesOnDisk can skip the number of files check.
MemberAuthor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radical@pavelsavara Should I make the change as part of this PR? If I understand @radical comment correctly, it would require removing the number of files check and replacing Assert.Equal(expected, actualFileNames) with Assert.Contains(expected, actualFileNames) in the AssertFilesOnDisk function.
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up PR would be fine.
This was referenced Jan 12, 2024
pavelsavara
commented
Jan 15, 2024
Member
/azp run runtime-wasm |
|
Azure Pipelines successfully started running 1 pipeline(s). |
pavelsavara
approved these changes
Jan 15, 2024
pavelsavara
left a comment
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @ilonatommy please approve for graphemes
ilonatommy
commented
Jan 15, 2024
Member
/azp run runtime-wasm-libtests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ilonatommy
approved these changes
Jan 15, 2024
This was referenced Jan 15, 2024
matouskozak
commented
Jan 16, 2024
MemberAuthor
Failures are unrelated and tracked. Merging. |
This was referenced Jan 16, 2024
tmds pushed a commit
to tmds/runtime
that referenced
this pull request
Jan 23, 2024
…nd LastIndexOf text APIs with HybridGlobalization mode (dotnet#95583) * Re-implement the grapheme segmenter from Intl. * Load segmentation rules as static json asset
pavelsavara
commented
Feb 1, 2024
Member
I realized that in order to make this MT friendly, we need to load the JSON on all web-workers, not just in the UI thread. |
matouskozak
commented
Feb 1, 2024
MemberAuthor
What changes would be necessary to accommodate this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
In this PR, we improve the time performance of
IndexOfandLastIndexOftext APIs when running in HybridGlobalization (HG) mode.This is done by picking up necessary parts from the previously used
Intl.Segmenterclass. By picking up only essential parts for grapheme segmentation, we could reduce the overhead and improve performance from ~2362ms down to ~38ms (measured using browser-bench)This improvement introduces a regression in raw size of ~21kB due to grapheme segmentation rules presented in src/mono/browser/runtime/hybrid-globalization/segmentation-rules.json. These rules are loaded separately as static JSON asset when HG is enabled.
Ideas
By introducing grapheme segmentation algorithm on the TypeScript side, we are duplicating code already present in .NET under the name
TextSegmentationUtility.GetLengthOfFirstUtf16ExtendedGraphemeCluster. I have several propositions for how we could leverage this:JsCompareString). By core part, I mean grapheme segmentation of source and target strings and iteration over strings to find a match. With this approach, we wouldn't introduce any additional rule files and use the already present and tested grapheme segmentation algorithm. However, doing interop for every string comparison introduces additional overhead, which would slow the APIs. I tried a basic version of this approach locally and measured ~200ms forIndexOfandLastIndexOfAPIs (slower than this PR, but still significantly faster than usingIntl.Segmenter). We could choose this approach if we consider the size increase by this PR undesirable.TextSegmentationUtilityfor grapheme segmentation, but we would have to segment the entire source/target string in advance. This could significantly hinder performance for cases when the target string can be found in the first parts of the source string. Additionally, we would have to pass an array of grapheme breaks to the interop call.runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CharUnicodeInfoData.cs
Lines 1441 to 2295 in b69f79c
GetNumericGraphemeTableOffsetNoBoundsChecksfunction in Typescript. Edit based on @pavelsavara comment: Ideally, we would preprocess data from CharUnicodeInfoData.cs only once and used them in subsequent calls.cc: @ilonatommy
Contributes to: #94473
Note: I don't have much experience with TypeScript and WASM in general, so feel free to change any parts that don't obey style conventions.