Make JSStringToSTLString 23x faster #26955

radex · 2019-10-22T12:15:50Z

Summary

In my app I have a case where I need to pass a very large string (45MB) between JS and native. This is obviously suboptimal, but… this is where I'm at.

The main bottleneck to doing this turned out to be jsi's JSStringToSTLString(), which was extremely slow. In my case, 4.7s to execute. After this change, 204ms.

I don't really know C++, so I'm not sure this code is 100% correct and safe, and I bet it could be done even better by avoiding the extra memory allocation (would shave off another 70ms).

Changelog

[General] [Changed] - Make JSStringToSTLString 23x faster

Test Plan

radex · 2019-10-22T12:21:47Z

Post scriptum: I can't even divide right

motiz88

Nice! I wonder what was slower - the zero-allocation or the linear scan to get the string length. 😄 Left a small comment but LGTM otherwise. I'll also loop in some folks with more context about this code.

motiz88 · 2019-10-22T15:55:32Z

ReactCommon/jsi/JSCRuntime.cpp

-  std::vector<char> buffer(maxBytes);
-  JSStringGetUTF8CString(str, buffer.data(), maxBytes);
-  return std::string(buffer.data());
+  char* buffer = new char[maxBytes];


Let's use std::make_unique<char[]> instead of raw new[] and delete[] - it has the same effect (allocating without zero-initialising) but is also exception-safe.

That's very strange. I don't really understand why the previous implementation is slower.
When I read the changed code without looking at the previous version of it, I thought "oh, using std::vector is more ideomatic here and should be as fast as a plain buffer".

I think the most efficient way to implement this function is to use something like folly::small_vector (or simply use the same approch manually) trying to avoid heap allocation for small strings.

Oh my, look at this:
https://en.cppreference.com/w/cpp/container/vector/vector
and then look at this 3-4) Linear in count. That's probably because we need to set to 0 every single element.

So, I would suggest rewriting this to:

std::vector<char> buffer; buffer.reserve(maxBytes);

Awesome work, @radex! 😍

@shergin AFAICT it's not legal to use vector::data after reserve, and actually resizing the vector runs into the same needless zero-initialisation issue. So I think there's no way to do this with vector without paying this cost.

@motiz88 is right, I am wrong. The unique_ptr looks like the best option.

(As a huge a huge fan of SSO, I would suggest to implement it here as well (e.g. having if the size is smaller than 21 or whatever, use stack-allocated buffer), but that's out of the scope of this PR.)

radex · 2019-10-24T08:58:44Z

Thanks @shergin & @motiz88 for pushing me to try to learn a little bit of C++ ;)

I hope I didn't mess anything up here -- I changed raw pointers to unique_ptr, and also added stack allocated buffer for small strings

motiz88

Love this. I'd structure the code a little differently so there's less repetition and therefore fewer places for future bugs to creep in:

std::array<char, 20> stackBuffer;
std::unique_ptr<char[]> heapBuffer;
char* buffer;
if (maxBytes <= stackBuffer.size()) {
  buffer = stackBuffer.data();
} else {
  heapBuffer = std::make_unique<char[]>(maxBytes);
  buffer = heapBuffer.get();
}
size_t actualBytes = JSStringGetUTF8CString(str, buffer, maxBytes);
return std::string(buffer, actualBytes - 1);

matthargett · 2019-10-24T20:50:09Z

I like @motiz88 's last suggestion. My only suggestion on top of that would be to extract the magic "20" number to an explaining static const variable (or whatever the FB C++ standards prefer that's similar in intent).

shergin · 2020-01-27T03:11:03Z

@motiz88 Let's land this (your improved version)?

motiz88 · 2020-01-27T09:20:57Z

@shergin Sure, I'll push my changes in a bit.

facebook-github-bot

@motiz88 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2020-01-28T18:30:05Z

This pull request was successfully merged by @radex in 733532e.

^{When will my fix make it into a release? | Upcoming Releases}

radex · 2020-01-28T19:01:37Z

woohoo! Thanks @motiz88

Summary: [A recent change to JSStringToSTLString](#26955) causes a crash when the function is invoked with invalid UTF-16 data. The old behaviour, restored here, was to truncate the string before the first invalid character. Here's how [the original code](https://github.com/facebook/react-native/blob/aee88b6843cea63d6aa0b5879ad6ef9da4701846/ReactCommon/jsi/JSCRuntime.cpp#L287) handled this case: ``` std::string JSStringToSTLString(JSStringRef str) { size_t maxBytes = JSStringGetMaximumUTF8CStringSize(str); // ^ maxBytes >= 1 regardless of str's contents std::vector<char> buffer(maxBytes); // ^ vector is zero initialised JSStringGetUTF8CString(str, buffer.data(), maxBytes); // ^ writes '\0' at the first invalid character and returns early (see JSC source code) return std::string(buffer.data()); // ^ copies the string up to the first '\0' } ``` See the JSC implementations of [`JSStringGetUTF8CString`](https://opensource.apple.com/source/JavaScriptCore/JavaScriptCore-7600.8.7/API/JSStringRef.cpp.auto.html) and [`convertUTF16ToUTF8`](https://opensource.apple.com/source/WTF/WTF-7600.7.2/wtf/unicode/UTF8.cpp.auto.html). Based on the fact that `JSStringGetUTF8CString` *always* null-terminates the buffer - even when it bails out of converting an invalid string - here we're able to both 1. keep the fast path (not zero-initialising, not scanning for the null terminator) for the common case when the data is valid and JSStringGetUTF8CString returns a nonzero length; and 2. return the truncated string when JSStringGetUTF8CString returns an error code of 0, by scanning for the null terminator. Changelog: [General] [Fixed] - Fix crash when passing invalid UTF-16 data from JSC into native code Differential Revision: D19902751 fbshipit-source-id: 06bace2719800e921ec115ad6a29251eafd473f6

Summary: In my app I have a case where I need to pass a very large string (45MB) between JS and native. This is obviously suboptimal, but… this is where I'm at. The main bottleneck to doing this turned out to be `jsi`'s `JSStringToSTLString()`, which was extremely slow. In my case, 4.7s to execute. After this change, 204ms. I don't really know C++, so I'm not sure this code is 100% correct and safe, and I bet it could be done even better by avoiding the extra memory allocation (would shave off another 70ms). ## Changelog [General] [Changed] - Make JSStringToSTLString 23x faster Pull Request resolved: facebook#26955 Reviewed By: shergin Differential Revision: D19578728 Pulled By: motiz88 fbshipit-source-id: 2fbce83166953ce928f0a6aa36eed710bfe05383

Summary: [A recent change to JSStringToSTLString](facebook#26955) causes a crash when the function is invoked with invalid UTF-16 data. The old behaviour, restored here, was to truncate the string before the first invalid character. Here's how [the original code](https://github.com/facebook/react-native/blob/aee88b6843cea63d6aa0b5879ad6ef9da4701846/ReactCommon/jsi/JSCRuntime.cpp#L287) handled this case: ``` std::string JSStringToSTLString(JSStringRef str) { size_t maxBytes = JSStringGetMaximumUTF8CStringSize(str); // ^ maxBytes >= 1 regardless of str's contents std::vector<char> buffer(maxBytes); // ^ vector is zero initialised JSStringGetUTF8CString(str, buffer.data(), maxBytes); // ^ writes '\0' at the first invalid character and returns early (see JSC source code) return std::string(buffer.data()); // ^ copies the string up to the first '\0' } ``` See the JSC implementations of [`JSStringGetUTF8CString`](https://opensource.apple.com/source/JavaScriptCore/JavaScriptCore-7600.8.7/API/JSStringRef.cpp.auto.html) and [`convertUTF16ToUTF8`](https://opensource.apple.com/source/WTF/WTF-7600.7.2/wtf/unicode/UTF8.cpp.auto.html). Based on the fact that `JSStringGetUTF8CString` *always* null-terminates the buffer - even when it bails out of converting an invalid string - here we're able to both 1. keep the fast path (not zero-initialising, not scanning for the null terminator) for the common case when the data is valid and JSStringGetUTF8CString returns a nonzero length; and 2. return the truncated string when JSStringGetUTF8CString returns an error code of 0, by scanning for the null terminator. Changelog: [General] [Fixed] - Fix crash when passing invalid UTF-16 data from JSC into native code Differential Revision: D19902751 fbshipit-source-id: 06bace2719800e921ec115ad6a29251eafd473f6

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 22, 2019

kelset requested a review from ericlewis October 22, 2019 12:19

radex changed the title ~~Make JSStringToSTLString 44x faster~~ Make JSStringToSTLString 23x faster Oct 22, 2019

radex mentioned this pull request Oct 22, 2019

Fast login - iOS Nozbe/WatermelonDB#537

Merged

motiz88 requested changes Oct 22, 2019

View reviewed changes

motiz88 reviewed Oct 24, 2019

View reviewed changes

radex and others added 3 commits January 27, 2020 13:34

Update JSCRuntime.cpp

c197db7

JSStringToSTLString - use std::unique_ptr, small string optimization

719e60c

Restructure JSStringToSTLString and add comments

1e922cf

motiz88 force-pushed the patch-1 branch from 35e3b6c to 1e922cf Compare January 27, 2020 13:50

facebook-github-bot reviewed Jan 27, 2020

View reviewed changes

facebook-github-bot closed this in 733532e Jan 28, 2020

react-native-bot added the Merged This PR has been merged. label Jan 28, 2020

radex deleted the patch-1 branch January 28, 2020 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make JSStringToSTLString 23x faster #26955

Make JSStringToSTLString 23x faster #26955

radex commented Oct 22, 2019 •

edited by motiz88

Loading

radex commented Oct 22, 2019

motiz88 left a comment •

edited

Loading

motiz88 Oct 22, 2019

shergin Oct 22, 2019

shergin Oct 22, 2019 •

edited

Loading

motiz88 Oct 22, 2019

shergin Oct 22, 2019 •

edited

Loading

shergin Oct 22, 2019

radex commented Oct 24, 2019

motiz88 left a comment

matthargett commented Oct 24, 2019

shergin commented Jan 27, 2020

motiz88 commented Jan 27, 2020

facebook-github-bot left a comment

react-native-bot commented Jan 28, 2020

radex commented Jan 28, 2020

Make JSStringToSTLString 23x faster #26955

Make JSStringToSTLString 23x faster #26955

Conversation

radex commented Oct 22, 2019 • edited by motiz88 Loading

Summary

Changelog

Test Plan

radex commented Oct 22, 2019

motiz88 left a comment • edited Loading

Choose a reason for hiding this comment

motiz88 Oct 22, 2019

Choose a reason for hiding this comment

shergin Oct 22, 2019

Choose a reason for hiding this comment

shergin Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

motiz88 Oct 22, 2019

Choose a reason for hiding this comment

shergin Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

shergin Oct 22, 2019

Choose a reason for hiding this comment

radex commented Oct 24, 2019

motiz88 left a comment

Choose a reason for hiding this comment

matthargett commented Oct 24, 2019

shergin commented Jan 27, 2020

motiz88 commented Jan 27, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Jan 28, 2020

radex commented Jan 28, 2020

radex commented Oct 22, 2019 •

edited by motiz88

Loading

motiz88 left a comment •

edited

Loading

shergin Oct 22, 2019 •

edited

Loading

shergin Oct 22, 2019 •

edited

Loading