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 support interning #84695

Closed

Conversation

ilonatommy
Copy link
Member

Suggested in comment. For performance reasons it pays off to exchange char*+lenght to string when passing data from managed code to JS.

@ghost
Copy link

ghost commented Apr 12, 2023

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

Issue Details

Suggested in comment. For performance reasons it pays off to exchange char*+lenght to string when passing data from managed code to JS.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-System.Globalization

Milestone: -

{
cmpResult = Interop.JsGlobalization.CompareString(out exceptionMessage, cultureName, pString1, string1.Length, pString2, string2.Length, options);
}
int cmpResult = Interop.JsGlobalization.CompareString(out exceptionMessage, cultureName, span1.ToString(), span2.ToString(), options);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this going to allocate temporary C# strings in order to pass them to JS? This will break interning too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how to do it differently, if we want to operate on strings we need to at least allocate a local string in the function. We cannot change the fact that arguments enter in a form of ReadOnlySpan<char> and it looks like span, passing the C#/JS border is treated the same way as char*.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we have speed vs (icu data) size tradeoff here. If they choose hybrid, they would get size, but not speed.
Seems OK to me if the perf is around %200.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the past mesurements on normalization function it was more than that, around 500%. It is a good idea to start measuring how HG does in comparison to ICU4C.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how to do it differently, if we want to operate on strings we need to at least allocate a local string in the function. We cannot change the fact that arguments enter in a form of ReadOnlySpan<char> and it looks like span, passing the C#/JS border is treated the same way as char*.

In that case for this the way you did it previously (passing pointers) is the only reasonable way, making a temp C#/MonoString is not going to help.

@ilonatommy
Copy link
Member Author

Closing after investigating all possible solutions. See - discussion above

@ilonatommy ilonatommy closed this Apr 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 14, 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.

3 participants