perf(sorted_set): compute union size from split drops instead of recounting#3829
Draft
bobzhang wants to merge 1 commit into
Draft
perf(sorted_set): compute union size from split drops instead of recounting#3829bobzhang wants to merge 1 commit into
bobzhang wants to merge 1 commit into
Conversation
…unting Resolves the TODO in union: after merging, the result size was recomputed by a full each() traversal of the merged tree. An element common to both sets is dropped exactly once, at the `comp == 0` branch of split, so split now reports its drop count (0 or 1) as a third tuple component and union accumulates them: size = |self| + |src| - dups. This removes the O(n+m) recount pass. The split whitebox tests now also pin the drop count for present, absent, and empty-tree splits, and new union tests assert exact sizes for overlapping, disjoint, and identical operands, cross-checked against an element count. Partially addresses #3824 (the defensive copy_tree of both operands is inherent to the mutable node design and stays). Reviewed by Codex CLI (codex-cli 0.144.1): "Approved; no findings. Inductively, split creates disjoint partitions; each src node can be dropped only at its unique matching self pivot... Thus dups equals the intersection size exactly." Signed-off-by: Codex CLI <codex@openai.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Coverage Report for CI Build 5186Coverage remained the same at 91.268%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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
2 participants
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.
Partially addresses #3824 — resolves the
TODO: optimize this. Avoid counting the size of the setinunion.Before
After building the merged tree,
unionrecomputed the result size with a fulleach()traversal (an extra O(n+m) pass over the freshly built tree).After
An element common to both sets is dropped exactly once, at the
comp == 0branch ofsplit.splitnow returns its drop count (0 or 1) as a third tuple component,unionaccumulates them, and the size is arithmetic:|self| + |src| − dups. The recount pass is gone;splitstays pure (noRefcell).Tests: the
splitwhitebox tests now also pin drop counts for present/absent/empty splits, and new union tests assert exact sizes for overlapping, disjoint, and identical operands, cross-checked against an actual element count.Not addressed here (per the issue): the defensive
copy_treeof both operands, which is inherent to the mutable node design — rotations mutate in place, so subtree sharing would corrupt the sources.Review
Reviewed by Codex CLI (codex-cli 0.144.1): "Approved; no findings. Inductively,
splitcreates disjoint<, equal, and>partitions; eachsrcnode follows one recursion branch and can be dropped only at its unique matchingselfpivot. Allselfvalues are rejoined, whilejoin*and rotations preserve every value. Thusdups = |self ∩ src|exactly."Signed-off-by: Codex CLI codex@openai.com
Validation
moon checkclean,moon fmtapplied, no.mbtichangesmoon test: 6716 passed, 0 failed (sorted_set 57/57)🤖 Generated with Claude Code