Repository navigation
Speed up Diff.diff on inputs with repeated elements - #15
Conversation
diff found the longest common run by keeping the previous and current rows of the overlap table as Int->Int Maps, with a fresh Map per element of new and a Map.get plus a Map.put for every matching pair. It indexed old by copying an element's position array on every push, and every recursion level copied prefix/suffix slices of both inputs and rebuilt that index. Repeated elements make the number of matching pairs quadratic, so 2000 copies of one value with one change took 8.6 s. Elements are now interned to Int ids once, with an ascending list of old positions per id. The recursion works on index ranges of the original arrays and appends to one result array, slicing only for hunk payloads. Run lengths are kept per diagonal: a match extends the previous match on its diagonal only if that pair is also a match inside the current ranges, which comparing two ids decides. Binary search finds the old positions inside a range. The scan order and the strictly-greater tie-break are unchanged, so the output is identical. The two new tests pin the tie-break between two equally long runs around an insertion and around a deletion; both pass on master and fail if the tie-break or a range boundary is wrong.
There was a problem hiding this comment.
Build & Tests
- On head
49b949b, built with the local carp and the 10-02 compat core (the old bucketMap),carp -x tests/diff.carppasses 97/0. Master passes 95/0. - Master with this branch's
tests/diff.carppasses 97/0. So the two new rows describe behaviour master already has: they kill mutants and aren't regression tests, as the PR says. - CI's
Run testsstep printsPassed: 97 Failed: 0on both ubuntu-latest and macos-latest at49b949b. Those runners use upstream core with the open-addressingMap. anglerandcarp-fmt --checkare clean ondiff.carpandtests/diff.carp.Diff.diff's inferred type is unchanged,(Fn [(Ref (Array a)) (Ref (Array a))] (Array (Diff (Array a)))), and its doc string is kept.
Findings
No blocking issues.
Output matches master on my own differential.
-
I wrote a separate generator (Python; seeds 4242, 9001 and 7) and built one driver per side from
git archivesnapshots, then compared the two outputs withcmp. -
It covers 16,060 pairs and 26,838 results:
diffon Int, String and Double arrays, pluschar-diff,string-diffandline-diff. -
The shapes are:
- unrelated arrays over alphabets of 1 to 1,000 symbols;
- near-identical pairs with random insert, delete, replace and block-move edits;
- repeated blocks with edits;
- rotations and reversals;
- a shared prefix and suffix around a changed middle;
- 60 pairs of 300 to 900 elements.
736 pairs have an empty side, and 91 of those are empty on both sides.
-
There are 0 mismatches. The run isn't vacuous: 94% of results have an
Eqhunk and 65% have at least two, so the range recursion is well exercised. -
The
>=-for->mutant atdiff.carp:59changes 6,654 of 13,343 result lines on the same driver, so the harness can detect a change. -
Every
unsafe-nth/aset!in this build assertsn >= 0andn < a.len, and the clean run had those checks on. Sodiag,oid,nidandoccstayed in bounds on every pair.
diag reuse is sound. d runs over [1, n+m-1], which fits in diag's n+m slots (diff.carp:53, :81). diag[d] is read only when (p-1, inew-1) is a match inside the current ranges (:54-55, with prev-id set to -1 at nlo, :47-49). The same call wrote that slot one row earlier, and nothing else on that diagonal is written in between. Recursion starts only after the loop.
Mutants. I ran 11 mutants, each against the 97-test suite and against the first differential set (8,000 pairs):
| mutant | suite | differential |
|---|---|---|
>= for > (:59) |
2 fail | 6,654 lines differ |
p > 0 for p > olo |
1 fail | 2,521 differ |
inew > 0 for inew > nlo |
1 fail | 2,564 differ |
range end = (Array.length ol) |
2 fail | 6,719 differ |
(>= p olo) |
aborts | aborts |
range start (inc olo) / end (dec ohi) |
20 / 19 fail | abort / 9,728 differ |
aset! only on a new best |
2 fail | aborts |
| Deletion before Insertion | 13 fail | 5,005 differ |
| id check dropped | 15 fail | aborts |
id check = → <= (:55) |
97/0, survives | aborts |
- Master's 95-test suite passes the first four mutants (95/0), and the two new rows kill each one, as the body claims.
- Optional: the
<=survivor, which the PR body already mentions, can be pinned with one more row, appended after the two new ones (aftertests/diff.carp:71):It passes on head and on master. The mutant returns(assert-equal test &[(Eq [2]) (Deletion [1]) (Eq [0]) (Insertion [0])] &(diff &[2 1 0] &[2 0 0]) "a run only extends over a matching previous pair")
[(Eq [2]) (Eq [0 0])], anEqhunk that pairs1with0. With the row added, the suite is 98/0 on head and 97/1 on the mutant, andcarp-fmt --checkandanglerstay clean.
Timings on this box (shared with other jobs, load about 2):
- All-
"x"with n=2000 and the middle element changed: master 8,617 ms, head 394-399 ms. That matches the body. line-diffon web.carp918eb34(3,910 lines) with every 40th line edited (my edits: 98 lines with a suffix appended): master 6,252 ms, head 437 ms, a 14x speedup. The body's own edit pattern gives 5,928 → 238 ms, so the ratio depends on the edits.
Memory and stack.
Debug.memory-balanceis 0 afterdiffon Int and String arrays,line-diff,char-diff,string-diffand empty sides. As a positive control, the same probe reads 16 while the inputs and result are still held.- Recursion depth is unchanged, because the split points are the same. The new frames are smaller: an interleaved
[0 1 2 …]vs[0 -1 1 -1 …]with N=1000 needs 1 MB of stack on master and 512 KB on head, and both crash at 256 KB.
Verdict: merge
Output is identical to master on 16,060 independent pairs, the speedups reproduce, CI is green on both platforms, and the only suite gap is the <= survivor the PR already mentions (optional one-row fix above).
Diff.diffpaid aMap.getand aMap.putfor every matching element pair, using a freshMapfor each element ofnew. It also built its index ofoldby copying an element's position array on every push. Every recursion level copied prefix/suffix slices of both inputs and rebuilt that index. A repeated element makes the number of matching pairs quadratic: 2000 copies of one string with one change took 8.6 s.This PR changes how
diffdoes that work:Intid once, with an ascending list ofoldpositions per id.oldpositions inside a range.The scan order and the strict
>tie-break are unchanged, so the output is byte-identical. The signatures ofdiff,line-diff,char-diffandstring-diffare unchanged. The onlyMapfunctions used are{},Map.putandMap.get-with-default, which exist in both the old core and the open-addressing one.Timing
Measured on armhf (Raspberry Pi), using the local carp binary with the old core (before the open-addressing
Map) and default build flags. Each row runsdiffon string arrays:"x", middle one changedAll-equal inputs are still quadratic, because every matching pair is still visited. Each visit now costs a few array reads and one write.
Verification
carp -x tests/diff.carp: 97 passed, 0 failed. That is the 95 existing tests plus 2 new ones, and the new ones also pass on master.prnoutput of master'sdiff(copied under another name) with this one. Inputs are Int and String arrays over 1–4 symbols, lengths 0–60, with an empty side in about 38% of pairs. Result: 0 mismatches over 20,000 pairs per element type for each of two seeds, and 0 over 2,000 more pairs with lengths up to 200.>=for>: 2,837 of 5,000 pairs differ.<=for=in the diagonal check both abort on a negative index.>=mutant nor the three range-boundary mutants. The two new rows are the smallest shapes, found by exhaustive search over 2-symbol arrays, that together kill all four. Each pins the tie-break between two equally long runs, one around an insertion and one around a deletion. The<=diagonal-check mutant still passes the suite. Dropping the check entirely fails 15 tests.diffandline-diff.carp-fmt --checkandanglerare clean.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.