JSI - iOS implementation #541
Conversation
# Conflicts: # native/ios/WatermelonDB/DatabaseBridge.swift
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: #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 Pull Request resolved: facebook#27016 Reviewed By: RSNara Differential Revision: D18769047 Pulled By: mhorowitz fbshipit-source-id: 9d1ee28840303f7721e065c1b3c347e354cd7fef
|
11 months after first toying with the idea (#426), the iOS implementation of the JSI adapter is DONE. @kokusGr Sorry about the gargantuan PR. I've split it up into smaller chunks as much as I could… but for a long time I didn't know what final shape the implementation will take so it was hard for me to submit PRs over time. Also, the Database.cpp implementation is big. In the ideal world, I would have split up the React Native-specific concerns from WatermelonDB-specific logic from SQLite-specific logic (like with the old adapter). However, this is performance-critical code and sadly, doing so is a major no-go. Transforming data between RN's representation and sqlite C representation is the biggest bottleneck, and an intermediate transformation into a common C++ STL representation would mean a huge performance (and peak memory use) hit. So sadly, we have to intertwine jsi, sqlite3, and watermelon-specific logic in the same place. @kokusGr After you review this, give it to @jsamelak to take a look at the C++ code. |
|
|
||
| if (resultPrepare != SQLITE_OK) { | ||
| sqlite3_finalize(statement); | ||
| throw dbError("Failed to prepare query statement"); |
There was a problem hiding this comment.
Yea, seems like a good idea to pass error code from sqlite3_finalize. Unless there are some implications of doing so 🤔
There was a problem hiding this comment.
If the most recent evaluation of statement S failed, then sqlite3_finalize(S) returns the appropriate error code or extended error code.
if I understand this correctly, finalize never fails - just returns, as a convenience the last state of running the statement - but we already check that.
or maybe I didn't understand your feedback?
There was a problem hiding this comment.
Sorry maybe I should write this under this comment // TODO: In serialized threading mode, those may be incorrect - probably smarter to pass result codes around? If sqlite3_finalize already returns the code we can pass it to dbError :) Unless it's different than code returned by sqlite3_extended_errcode.
There was a problem hiding this comment.
ah. yes, two things:
- currently we don't do multithreaded operations, so no need to worry about it. if we do, I think we'll have to revise all dbError calls, but i'll need to do more research
- indeed what sqlite returns is different from sqlite3_extended_errcode… they return… well… non-extended error codes :D and you use the separate function to get a more specific reason for an error.
| return cachedRecords_.find(cacheKey) != cachedRecords_.end(); | ||
| } | ||
| void Database::markAsCached(std::string cacheKey) { | ||
| // TODO: what about duplicates? |
There was a problem hiding this comment.
cachedRecords_ is of type unordered_set, and sets cannot contain duplicates.
|
|
||
| jsi::Array records(rt, 0); | ||
|
|
||
| for (size_t i = 0; true; i++) { |
There was a problem hiding this comment.
is it guaranteed that stepResult eventually will be equal to SQLITE_DONE? Otherwise you could end with infinite loop
There was a problem hiding this comment.
yep - we get SQLITE_ROW zero or more (but not infinite) number of times, followed by SQLITE_DONE — or another status for error (which will break out of the loop by throwing).
| auto &rt = getRt(); | ||
| beginTransaction(); | ||
| try { | ||
| // TODO: Maybe it's faster & easier to do it in one query? |
There was a problem hiding this comment.
You could gather IDs into list and perform delete from (...) where id IN (...IDs)
There was a problem hiding this comment.
yeah, I know, but i can't pass an array as a placeholder argument, so I'd have to concatenate strings which I wanted to avoid doing in c++. I'm gonna leave it like that for now, but i'll get back to deletion performance later and see if this matters
|
@radex you want me to merge this PR? |
|
not yet, there's a parent PR waiting |
No description provided.