perf: guard the big-int rescue scan on a necessary condition #71
No reviewers
Labels
No labels
area:defold
area:docs
area:infra
area:protocol
area:testing
kind:bug
kind:chore
phase:breadth
phase:core
phase:hardening
phase:slice
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
yuris/spacetimedb-defold!71
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "perf/69-quote-guard"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #69.
quote_big_intsran its patterngsubunguarded at both callsites — once per frame indecode_server, once per row indecode_row. On a table with no 128-bit column every one of those scans rewrote nothing and still cost about what the parse did. Its siblingquote_wide_intshas had a necessary-condition guard for a while; this gives the big-int scan the same shape.The condition: every key the pass can rescue contains the byte run
"__. A plainfindis ~free, and absent that run the pattern cannot match. A"__inside a row's double-encoded string is a false positive — it runs the scan for nothing, which is exactly the old behaviour. There is no false negative, which is the only direction that could corrupt a u128.Measurement
Both from
dev/tools/conformance.sh, 10000 rows,STDB_BULK_RUNS=3, hand-written schema, same machine. The harness runs two engine processes per invocation; the second is quoted here.Before, on
main@a606406:After, on this branch:
quo40% → 0%, frame median 29.3ms → 20.2ms. PHASE5 (identity-keyed table) passes, which is the live exercise of the rescue path — the guard does not defeat it. HTML5 conformance also passes, withquo=0%there too.The harness needed correcting, not the guard
PHASE6 asserted
quote_calls + wide_calls == decode_calls— "exactly one quoting scan precedes every decode". The guard makes that false by design: thegsubis now reached only when the guard hits. So the invariant is re-derived rather than relaxed, and it is now strictly stronger:guard_calls + wide_calls == decode_calls— every decode is still preceded by exactly one quoting attempt;quote_calls == guard_hits— the rescuegsubruns on exactly the attempts that hit, never on a miss. A mismatch catches both failure directions: a miss leaking through to the scan (the cost this PR removes, back again), or a hit being swallowed (a rescue silently skipped, which would corrupt u128 columns).The guard is counted, not timed, for the same reason the wide-int guard is: timing it was tried first and billed ~1.8ms of pure instrumentation at 10k rows — 10% of the frame, all apparatus, since two clock reads cost more than the
findthey measure. One clock read per apply still marks the apply's start. That is whyquotereads0.0msrather than a small number.A note on the unit test
The issue's sketch asked for
assert(rawequal(out, input))to pin that the guard skips the copy. That assert cannot discriminate here and was dropped: this is Lua 5.1, which interns every string, so agsubthat matched nothing hands back an object alreadyrawequalto its input. Verified directly — the assert passes with the guard removed entirely, and it passed with a 2400-byte frame too, so it is not a short-string artefact. Publishing it would have been a test that cannot fail.What the tests assert instead is the part that would actually break if the condition were too strong: a frame with no
"__reachesjson.decodeunrewritten, a frame carrying a rescuable key is still rewritten and still decodes to exact digits, and the escaped\"__connection_id__\"form inside row data still round-trips. The performance claim is carried by thequoshare above, which is where it belongs.README
The performance section described the scan as unguarded and bounded a codec swap at 6–8x on that basis. Re-measured and rewritten rather than patched: every number in it moved, and the two claims that went stale in the unfavourable direction are called out rather than dropped.
json.decode11.9ms, quoting 0.0ms, construction 5.6ms, frame 18.0ms.decode_typednever paid the per-row big-int scan, so it got nothing from this guard while the hand-written path got everything. Nearly half a typed frame is construction.Sweep data behind those crossings: native 7000→13.2ms, 8000→14.7, 8500→16.3, 9000→18.2, 10000→18.0, 25000→53.6, 40000→90.5, 45000→98.7, 50000→112.9 (pooled medians, two processes each). Wasm 1000→21.5, 5000→46.5, 10000→92.5, 11000→102.5, 12000→106.
stylua --checkclean,7 passed, 0 failed, both conformance harnesses pass.All eight findings actioned. Three of them changed the shipped code, not just the prose — thank you, particularly for mutation-testing the guard and for catching that
apparatuswas reporting exactly double.The guard now keys on the initials, not the bare
"__runFinding 3 was the one with real reach. Keying on
"__left the frame-level scan in place for everyInitialSubscription(unescaped"__time_duration_micros__") — i.e. for every caller of the documented legacydb:subscribe, which is the ~5.6ms you measured. Taking the suggestion:Still a necessary condition — both
BIG_INT_FIELDSkeys spell out to"__connection_id__or"__uuid__— and"__tis not rescuable, so the micros envelope short-circuits too. ATransactionUpdatestill hits, correctly: its__connection_id__is a value the scan exists to rescue.Because that condition is now keyed on the initial, a future
BIG_INT_FIELDSkey starting with another letter would be silently short-circuited. Noted at the table itself, since that is where someone would add one.timed_callswas reporting doubleFinding 1, taken verbatim:
local timed_calls = (rows + 1) + quote_calls. Confirmed on the hand-written path —apparatus=1.49ms→ 0.71ms, matching the 0.74ms you measured on the typed path for the same 10001 timed calls. The comment claiming "one timed quoting scan per row" and "the exact size of the bias against the hand-written schema" is rewritten, since post-guard that bias is zero.guard_hits ~= 0addedFinding 5 is right that
quote_calls == guard_hitsis0 == 0and cannot see a guard broadened until it hits everywhere. Added alongside, mirroring the wide-int guard four lines below. Both assertions kept: the equality catches a swallowed hit (a rescue silently skipped), the zero-check catches a restored scan. Passes on both tiers.__uuid__was genuinely uncoveredFinding 4, taken verbatim. Verified it discriminates — mutating the guard to
find('"__connection_id__'):Finding 6 accepted as stated: the
PLAINassertion documents the contract rather than testing it, and the comment now says so instead of claiming it covers over-tightening. I also dropped two assertions I had duplicated from the pre-existing block 30 lines above (micros-stay-numbers, escaped-key).README: the construction row was the guard's own cost
Finding 2 was correct and the fix needed a measurement, not a rewording. Paired
main-vs-branch runs atSTDB_BULK_RUNS=5, back to back in one session:build3.85 → 8.0ms with no construction code touched, exactly as you said. Microbenchmarked the two contributors at 10000 rows on a row of this table's shape:So construction is published as
main's 3.9ms, and the table gained an explicitunbilled: the guard, and the harness's own wrapper | ~3.3msrow rather than hiding ~3.3ms inside a bucket labelled construction. The codec bound moves to ~2.4x (from the ~3x I had, which was computed off the inflated residual).Finding 7: arithmetic fixed and re-measured rather than just recomputed — wasm 1000 rows is 22.0ms pooled over six samples, so 17ms is 77% of it. The wasm column is also corrected against a fresh
STDB_BULK_RUNS=3run (decode28 → 23.5ms,build17.5 → 19.5ms, bound ~1.4x → ~1.3x).Native crossings re-swept with the final code, pooled: 8000 → 15.8ms, 8500 → 16.9, 9000 → 17.7, 45000 → 107.7. So 16.7ms at ~8500 rows and 100ms at ~42000, both stated to one significant figure now, with the observed 15-25% spread quoted beside them. The typed penalty lands at ~50% against the paired 20.9ms baseline.
Finding 8: taken — full lines below,
apparatusincluded, since that field is what exposed finding 1.Suite
7 passed, 0 failed,stylua --checkclean, and both conformance harnesses pass on the final code — native all six phases, HTML5 green withquo=0%and the newguard_hitsassertion holding on the wasm tier too.