Repository navigation
fix(jsoo): tail-recursive List.remove_assoc closes the add-all stack gap - #37
Merged
Merged
Conversation
add-all at size 10000 overflowed the default Node stack under js_of_ocaml: ensure_current_tx_tempid walks the tempid order list (one entry per transacted entity) with the non-tail stdlib remove_assoc, producing ~10k nested frames plus caml_compare calls. Extend the existing Datascript_types.List shadow (already used for concat_map/concat/flatten) with a tail-recursive remove_assoc so native, Melange, and jsoo all get bounded-stack behavior. The full size-10000 bench suite now completes under node --stack-size=200.
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
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
add-allatBENCH_SIZE=10000overflowed the default Node.js stack under js_of_ocaml (RangeError: Maximum call stack size exceeded, worked around withnode --stack-size=8000indocs/perf.md).Root cause. Once per
transact,ensure_current_tx_tempid(impl/transact.ml:82, called atimpl/transact.ml:1908) runsList.remove_assoc "db/current-tx" (tempid_map_order tempids). The tempid order list carries one entry per transacted entity, so at size 10000 the walk is ~10,000 deep. The OCaml stdlib'sremove_associs not tail-recursive — under jsoo that becomes ~10k JS frames plus acaml_compare/caml_compare_valframe pair per element (captured via the.js_errorstashed bycaml_wrap_exception). Measured on this machine: the crash reproduces atnode --stack-size<=500and survives near ~1000 — exactly the default-stack boundary.Fix. Extend the existing
Datascript_types.Listshadow intype/datascript_types.ml— the same mechanism already used to replace Melange's recursiveconcat_map/concat/flatten— with a tail-recursiveremove_assoc(first-occurrence removal, order preserved,=equality — identical stdlib semantics). Because everyimplmodule opensDatascript_types, native, Melange, and jsoo all get the bounded-stack version; this also covers the other data-sizedremove_assocwalk inimpl/storage.ml(pending disk entries).Measured (same machine, Node v24.9.0, OCaml 5.5.0, size 10000, warmup 200ms / sample 500ms / 5 samples, median ms):
add-alljsoo--stack-size=500)--stack-size=200)add-allnativeThe remaining ~3x jsoo/native gap matches the size-200/1000 ratios in
docs/perf.md; the doc's stale--stack-size=8000note is updated.Verification. All 34 native test executables pass (incl.
test_perf);test/js_smoke.bc.jsandtest/js_facade_runner.jsagainstjs/datascript_js.bc.jspass.Link to Devin session: https://app.devin.ai/sessions/723787857ebe49cb8f0281b369d46433
Open in Devin Desktop: https://app.devin.ai/desktop/session/723787857ebe49cb8f0281b369d46433?variant=devin
Requested by: @tiensonqin