Skip redundant sqlite3_clear_bindings and long long disambiguation on hot insert path - #445
Merged
Merged
Conversation
… hot insert path opsqlite_bind_statement always cleared bindings before binding, even though the plain execute/executeSync/reactiveExecute paths bind immediately after a fresh sqlite3_prepare_v2, where bindings are already unset. Added a should_clear_bindings flag (default true) so PreparedStatementHostObject's bind/bindSync, which reuse a persistent statement across calls, keep the clear, while the fresh-prepare paths skip it. Also dropped to_variant's long long branch: it always ended up calling sqlite3_bind_double downstream anyway (same as the double branch), so the extra cast/comparison per numeric param had no behavioral effect. Benchmarked ~1000 sequential single-row inserts against react-native-nitro-sqlite; these were part of closing an unexplained insert throughput gap.
res.setProperty(rt, "rowsAffected", ...) and friends go through the Object::setProperty(Runtime&, const char*, ...) overload, which calls String::createFromAscii on every single call -- allocating a fresh JS String just to set a property whose name never changes across calls. Cache the PropNameIDs for "rowsAffected"/"insertId"/"rows" once and reuse them, the same way column_prop_ids are already reused across rows within a single call -- except these are reused across calls too. PropNameID is scoped to the jsi::Runtime that created it, and runtime "generations" can briefly overlap during a bridgeless reload (see the comment on generation_alive in OPTypes.hpp), so a single global cache isn't safe. Keyed the cache by Runtime* instead (thread_local, no lock needed since JSI runtime access is always confined to one thread at a time). Wired into create_js_rows (the executeSync/execute hot path), create_result (executeWithHostObjects), and create_raw_result.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
sqlite3_clear_bindingscall inopsqlite_bind_statementwhen the statement was just freshly prepared (execute/executeSync/reactiveExecutepaths) — bindings are already unset on a freshsqlite3_prepare_v2, so this was pure overhead on every call.PreparedStatementHostObject::bind/bindSync, which reuse a persistent statement across multiple calls, keep the clear via a newshould_clear_bindingsflag (defaults totrue, so callers I didn't touch are unaffected).to_variant's number branch: thelong longdisambiguation always fell through tosqlite3_bind_doubledownstream anyway (same as the plaindoublebranch), so it was doing an extra cast + comparison per numeric parameter for no behavioral difference.PropNameIDs for"rowsAffected"/"insertId"/"rows"perjsi::Runtime, reused the same waycolumn_prop_idsare already reused across rows within a call — except these are reused across calls too, since they're set on everyexecute()result regardless of query.Object::setProperty(rt, const char*, ...)allocates a freshjsi::StringviaString::createFromAsciion every call, which this avoids. Keyed byRuntime*(not a single global) since runtime generations can briefly overlap during a bridgeless reload — see thegeneration_alivecomment inOPTypes.hpp.Why
Found while benchmarking op-sqlite against
react-native-nitro-sqliteon 1000 sequential single-row inserts — op-sqlite was unexpectedly slower on both sync and async inserts despite having fewer abstraction layers. These changes remove fixed per-call overhead on the bind and result-marshaling paths that scales with insert volume.Test plan
PreparedStatementHostObject.bind()reused across multipleexecute()calls with varying param values still binds correctly (this is the one path that still clears bindings)PropNameID