Make JSCRuntime::createValue 35% faster #27016
Conversation
facebook-github-bot
added
the
CLA Signed
|
Doh, I accidentally broke Symbols. btw. @mhorowitz -- dcc40a6#diff-44cf263c203a5949e25721f41394e9aeR418 — I haven't tested yet, but I think this was broken already on iOS 13 / macOS Catalina, and community Android builds of JSC, where there is a kJSTypeSymbol: WebKit/WebKit@76b4e1b#diff-7cc5e4afb4c7092e07c92db27a5c328eR89-R90 |
ReactCommon/jsi/JSCRuntime.cpp
Outdated
| } | ||
| // case kJSTypeSymbol: | ||
| default: | ||
| // WHAT ARE YOU |
There was a problem hiding this comment.
This is neat! But do you need to check smellsLikeES6Symbol here?
There was a problem hiding this comment.
There are tests for this in https://github.com/facebook/react-native/blob/master/ReactCommon/jsi/jsi/test/testlib.cpp#L1160 but I am not sure if they actually run in CI. But you should be able to compile/run them by hand.
If we want RN to build and pass these tests against JSC libraries with and without this symbol, it may be tricky. Under Xcode, there are macros to test library versions, but I don't know if that works when using community JSC on Android.
|
|
||
| JSType type = JSValueGetType(ctx, ref); | ||
|
|
||
| if (type == /* kJSTypeSymbol */ 6) { |
There was a problem hiding this comment.
I think this should work, no?
|
@jacobp100 @mhorowitz fixed! |
|
@mhorowitz any update? I've been running this patch in production for a week now, haven't seen any issues. |
There was a problem hiding this comment.
@mhorowitz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@mhorowitz what's holding up this PR? |
|
This pull request was successfully merged by @radex in 24e0bad. When will my fix make it into a release? | Upcoming Releases |
Summary: JSC does some sort of thread safety locking on every single JSC API call, which makes them ridiculously expensive for a large number of calls (such as when passing a large array over the RN bridge). It would be great if we could lock and unlock once for an entire sequence of JSC calls… but in the meantime, the less we call JSC the better.  In my benchmark environment (Nozbe/WatermelonDB#541), the time spent in JSCRuntime::createValue went down from 1.07s to 0.69s by changing JSValueIsXXXX calls to a single JSValueGetType call. The underlying implementation does the same thing: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/API/JSValueRef.cpp#L58 ## Changelog [General] [Fixed] - Make JSCRuntime::createValue faster Pull Request resolved: facebook#27016 Reviewed By: RSNara Differential Revision: D18769047 Pulled By: mhorowitz fbshipit-source-id: 9d1ee28840303f7721e065c1b3c347e354cd7fef
Summary
JSC does some sort of thread safety locking on every single JSC API call, which makes them ridiculously expensive for a large number of calls (such as when passing a large array over the RN bridge). It would be great if we could lock and unlock once for an entire sequence of JSC calls… but in the meantime, the less we call JSC the better.
In my benchmark environment (Nozbe/WatermelonDB#541), the time spent in JSCRuntime::createValue went down from 1.07s to 0.69s by changing JSValueIsXXXX calls to a single JSValueGetType call.
The underlying implementation does the same thing: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/API/JSValueRef.cpp#L58
Changelog
[General] [Fixed] - Make JSCRuntime::createValue faster
Test Plan