Repository navigation
fix: preserve known cardinality when restoring stored sets - #2
Merged
Merged
Conversation
RCmerci
marked this pull request as ready for review
October 1, 2026 11:41
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
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.
Restoring a stored set currently discards its known cardinality. A later
countfolds the entire tree, even when the caller has already persisted the exact count for that root snapshot.Add a backwards-compatible
?count:intargument torestoreand seed the existing immutable count cache. Existing callers that omit it retain lazy traversal-based counting. Reject negative counts without reading storage. The documented contract requires the caller to supply the exact cardinality of the addressed snapshot; the API does not scan nodes to verify a hint.The existing successful add/remove paths already update a known count; duplicate inserts and absent removals preserve it. Tests cover those paths, empty sets, invalid counts, persistence of the original set, and the old no-count API. Melange and js_of_ocaml smoke tests exercise zero-read repeated counts too.
Validation on macOS arm64, OCaml 5.5.0, Dune 3.24.2, Node 22.21.1:
dune runtest --force --display=verbose: native, Melange/Node, js_of_ocaml/Node, Node GC and Chrome headless memory actions all passed.A companion DataScript change passes the counts from its stored root before replaying the tail and pins this exact PSS commit. In the combined before/after probe, counts remain 4096/4096/2048 while both the first and second count change from 333 index-node visits / 10240 leaf values to zero visits. DataScript snapshot depth scanning is a separate cost and remains unchanged.
Local experiments use synthetic memory storage, not a personal graph. No heap or wall-time speedup is claimed.
Companion DataScript PR: logseq/datascript-ocaml#23 . It pins this PR head exactly; suggested merge order is PSS then DataScript.