Skip to content

Commit 7929837

Browse files
bobzhangclaude
andcommitted
perf(sorted_set): compute union size from split drops instead of recounting
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 found pivot of the split, so union now counts those drops and derives the size arithmetically: size = |self| + |src| - dups. Rebased reimplementation: when this PR was first written, union's `split` was the only splitter and gained an Int drop-count component. main has since grown `split_member` (the split-based difference/ intersection/symmetric_difference rewrite), whose found flag carries exactly the same information - so union now uses `split_member`, and the now-unused `split` is deleted along with its whitebox test, which is replaced by an equivalent `split_member` test pinning found for present, absent, below-range, above-range and empty splits. `split_member`'s result also becomes a `#valtype` struct (`SplitResult`: left/found/right) instead of a 3-tuple, keeping the per-node result stack-allocated on native (same pattern as `JsonNumberScan` in json/lex_number.mbt). This benefits every split-based operation, not just union. Benchmarks (moon bench --release, n=10K sets, means of 10x batches): union, main -> this branch (recount removed + valtype): 50% overlap native 518 -> 429us (-17%), js 297 -> 222us (-25%), wasm-gc 197 -> 158us (-20%) disjoint native 382 -> 304us (-20%), js 261 -> 153us (-41%), wasm-gc 154 -> 96us (-38%) identical native 592 -> 520us (-12%), js 323 -> 277us (-14%), wasm-gc 232 -> 193us (-17%) valtype struct vs tuple (isolated, all split-based ops): native -5% to -9% across union/difference/intersection; js flat; wasm-gc within noise (+/-2%). Tests: union exact-size assertions for overlapping, disjoint and identical operands cross-checked against an element count; the split_member whitebox test above; the existing invariant and quickcheck suites. Mutation-verified: double-counting dups fails 8 tests including the exact-size test and the whitebox invariant scenarios. Partially addresses #3824 (the defensive copy_tree of both operands is inherent to the mutable node design and stays). Codex CLI review (xhigh, first-round sign-off on the rebased reimplementation): the dups induction re-verified on split_member (disjoint fragments, pivot counted exactly once, empty-side arms cannot hide common elements), sizes read from unmutated copies, SplitResult preserves tuple semantics at all four call sites, no split callers remain, interface byte-identical. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Codex CLI <codex@openai.com>
1 parent 6da28eb commit 7929837

2 files changed

Lines changed: 68 additions & 65 deletions

File tree

sorted_set/set.mbt

Lines changed: 53 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -177,12 +177,19 @@ pub fn[V : Compare] SortedSet::union(
177177
self : SortedSet[V],
178178
src : SortedSet[V],
179179
) -> SortedSet[V] {
180+
// An element common to both sets is dropped exactly once, at the found
181+
// pivot of `split_member`; counting those drops gives the result size
182+
// arithmetically instead of re-traversing the merged tree.
183+
let mut dups = 0
180184
fn aux(a : Node[V]?, b : Node[V]?) -> Node[V]? {
181185
match (a, b) {
182186
(Some(_), None) => a
183187
(None, Some(_)) => b
184188
(Some({ value: va, left: la, right: ra, .. }), Some(_)) => {
185-
let (l, r) = split(b, va)
189+
let { left: l, found, right: r } = split_member(b, va)
190+
if found {
191+
dups += 1
192+
}
186193
Some(join(aux(la, l), va, aux(ra, r)))
187194
}
188195
(None, None) => None
@@ -191,15 +198,8 @@ pub fn[V : Compare] SortedSet::union(
191198

192199
match (self.root, src.root) {
193200
(Some(_), Some(_)) => {
194-
let t1 = copy_tree(self.root)
195-
let t2 = copy_tree(src.root)
196-
let t = aux(t1, t2)
197-
let mut ct = 0
198-
let ret = { root: t, size: 0 }
199-
// TODO: optimize this. Avoid counting the size of the set.
200-
ret.each(_x => ct += 1)
201-
ret.size = ct
202-
ret
201+
let t = aux(copy_tree(self.root), copy_tree(src.root))
202+
{ root: t, size: self.size + src.size - dups }
203203
}
204204
(Some(_), None) => { root: copy_tree(self.root), size: self.size }
205205
(None, Some(_)) => { root: copy_tree(src.root), size: src.size }
@@ -208,42 +208,32 @@ pub fn[V : Compare] SortedSet::union(
208208
}
209209

210210
///|
211-
fn[V : Compare] split(root : Node[V]?, value : V) -> (Node[V]?, Node[V]?) {
212-
match root {
213-
None => (None, None)
214-
Some(node) => {
215-
let comp = value.compare(node.value)
216-
if comp == 0 {
217-
(node.left, node.right)
218-
} else if comp < 0 {
219-
let (l, r) = split(node.left, value)
220-
(l, Some(join(r, node.value, node.right)))
221-
} else {
222-
let (l, r) = split(node.right, value)
223-
(Some(join(node.left, node.value, l)), r)
224-
}
225-
}
226-
}
211+
// `#valtype` keeps the split result stack-allocated on the native target:
212+
// `split_member` builds and returns one per visited node on every
213+
// set-operation pivot path.
214+
#valtype
215+
priv struct SplitResult[V] {
216+
left : Node[V]?
217+
found : Bool
218+
right : Node[V]?
227219
}
228220

229221
///|
230-
/// Splits a tree by a value, returning (left, found, right).
231-
fn[V : Compare] split_member(
232-
root : Node[V]?,
233-
value : V,
234-
) -> (Node[V]?, Bool, Node[V]?) {
222+
/// Splits a tree by a value into the elements less than it, a flag for
223+
/// whether it was present, and the elements greater than it.
224+
fn[V : Compare] split_member(root : Node[V]?, value : V) -> SplitResult[V] {
235225
match root {
236-
None => (None, false, None)
226+
None => { left: None, found: false, right: None }
237227
Some(node) => {
238228
let comp = value.compare(node.value)
239229
if comp == 0 {
240-
(node.left, true, node.right)
230+
{ left: node.left, found: true, right: node.right }
241231
} else if comp < 0 {
242-
let (l, found, r) = split_member(node.left, value)
243-
(l, found, Some(join(r, node.value, node.right)))
232+
let { left, found, right } = split_member(node.left, value)
233+
{ left, found, right: Some(join(right, node.value, node.right)) }
244234
} else {
245-
let (l, found, r) = split_member(node.right, value)
246-
(Some(join(node.left, node.value, l)), found, r)
235+
let { left, found, right } = split_member(node.right, value)
236+
{ left: Some(join(node.left, node.value, left)), found, right }
247237
}
248238
}
249239
}
@@ -369,7 +359,7 @@ pub fn[V : Compare] SortedSet::difference(
369359
(None, _) => None
370360
(_, None) => a
371361
(Some({ value: va, left: la, right: ra, .. }), _) => {
372-
let (lb, found, rb) = split_member(b, va)
362+
let { left: lb, found, right: rb } = split_member(b, va)
373363
if found {
374364
found_count += 1
375365
concat(aux(la, lb), aux(ra, rb))
@@ -425,7 +415,7 @@ pub fn[V : Compare] SortedSet::symmetric_difference(
425415
(None, _) => b
426416
(_, None) => a
427417
(Some({ value: va, left: la, right: ra, .. }), _) => {
428-
let (lb, found, rb) = split_member(b, va)
418+
let { left: lb, found, right: rb } = split_member(b, va)
429419
if found {
430420
found_count += 1
431421
concat(aux(la, lb), aux(ra, rb))
@@ -475,7 +465,7 @@ pub fn[V : Compare] SortedSet::intersection(
475465
match (a, b) {
476466
(None, _) | (_, None) => None
477467
(Some({ value: va, left: la, right: ra, .. }), _) => {
478-
let (lb, found, rb) = split_member(b, va)
468+
let { left: lb, found, right: rb } = split_member(b, va)
479469
if found {
480470
found_count += 1
481471
Some(join(aux(la, lb), va, aux(ra, rb)))
@@ -1038,41 +1028,39 @@ test "union" {
10381028

10391029
///|
10401030
#warnings("-deprecated")
1041-
test "split" {
1042-
let (l, r) = split(from_array([7, 2, 9, 4, 5, 6, 3, 8, 1]).root, 5)
1031+
test "split_member" {
1032+
let { left: l, found, right: r } = split_member(
1033+
from_array([7, 2, 9, 4, 5, 6, 3, 8, 1]).root,
1034+
5,
1035+
)
1036+
inspect(found, content="true")
10431037
inspect(l, content="Some([1, 2, 3, 4])")
10441038
inspect(r, content="Some([6, 7, 8, 9])")
1045-
let (l, r) = split(from_array([7, 2, 9, 4, 5, 6, 3, 8, 1]).root, 0)
1039+
let { left: l, found, right: r } = split_member(
1040+
from_array([7, 2, 9, 4, 5, 6, 3, 8, 1]).root,
1041+
0,
1042+
)
1043+
inspect(found, content="false")
10461044
inspect(l, content="None")
10471045
inspect(r, content="Some([1, 2, 3, 4, 5, 6, 7, 8, 9])")
1048-
let (l, r) = split(from_array([7, 2, 9, 4, 5, 6, 3, 8, 1]).root, 10)
1046+
let { left: l, found, right: r } = split_member(
1047+
from_array([7, 2, 9, 4, 5, 6, 3, 8, 1]).root,
1048+
10,
1049+
)
1050+
inspect(found, content="false")
10491051
inspect(l, content="Some([1, 2, 3, 4, 5, 6, 7, 8, 9])")
10501052
inspect(r, content="None")
1051-
let (l, r) = split(from_array([7, 2, 9, 4, 5, 6, 3, 8, 1]).root, 4)
1053+
let { left: l, found, right: r } = split_member(
1054+
from_array([7, 2, 9, 4, 5, 6, 3, 8, 1]).root,
1055+
4,
1056+
)
1057+
inspect(found, content="true")
10521058
inspect(l, content="Some([1, 2, 3])")
10531059
inspect(r, content="Some([5, 6, 7, 8, 9])")
1054-
let (l, r) = split(from_array([]).root, 7)
1060+
let { left: l, found, right: r } = split_member(from_array([]).root, 7)
1061+
inspect(found, content="false")
10551062
inspect(l, content="None")
10561063
inspect(r, content="None")
1057-
let (l, r) = split(
1058-
from_array([
1059-
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21,
1060-
22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40,
1061-
41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59,
1062-
60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78,
1063-
79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97,
1064-
98, 99, 100,
1065-
]).root,
1066-
50,
1067-
)
1068-
inspect(
1069-
l,
1070-
content="Some([0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49])",
1071-
)
1072-
inspect(
1073-
r,
1074-
content="Some([51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100])",
1075-
)
10761064
}
10771065

10781066
///|

sorted_set/set_test.mbt

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -564,3 +564,18 @@ test "intersection and difference preserve self's stored representatives" {
564564
let out3 = lone.difference(far).to_array()
565565
assert_true(out3.length() == 1 && out3[0].payload == 1)
566566
}
567+
568+
///|
569+
test "union size is exact for overlapping, disjoint and identical sets" {
570+
let a = @sorted_set.from_array([1, 2, 3, 4, 5])
571+
let b = @sorted_set.from_array([4, 5, 6, 7])
572+
inspect(a.union(b).length(), content="7")
573+
inspect(b.union(a).length(), content="7")
574+
inspect(a.union(@sorted_set.from_array([10, 11])).length(), content="7")
575+
inspect(a.union(a).length(), content="5")
576+
// size must agree with an actual element count
577+
let u = a.union(b)
578+
let mut n = 0
579+
u.each(_x => n = n + 1)
580+
inspect(u.length() == n, content="true")
581+
}

0 commit comments

Comments
 (0)