perf(quickdup): hoist token sets out of the pairwise similarity loop - #13
Merged
Conversation
tokenSimilarity took two []string and built a map[string]bool for each on every call. clusterBySimilarity calls it once per pattern pair, so each pattern's set was rebuilt n-1 times: 2*O(n^2) map allocations per clustering pass. Build each set once in the tokenize pass that already runs, and pass the prebuilt sets in. The Jaccard math and every edge case are unchanged; comparison now iterates the smaller set since the result is symmetric. Measured with both implementations in one binary (BenchmarkPairLoop): n=100 109-206ms -> 12.7-15.7ms 56,962 -> 7,861 allocs 14.1MB -> 0.55MB n=300 565-1327ms -> 88-96ms 470,874 -> 23,570 allocs 125MB -> 1.66MB This surfaced in a 180s CPU profile of a Faktorial daemon, where clusterBySimilarity was 29.9% of consumed CPU and its allocation churn drove most of the 23% spent in GC scanobject. similarity_hoist_test.go pins equivalence rather than relying on the existing suite: TestTokenSimilarityMatchesPreHoist checks exact float equality against a verbatim copy of the old function over 20,000 randomized pairs plus explicit empty/duplicate/reordered edge cases, and TestPairLoopsAgree pins that the benchmarked A/B pair reaches identical clustering decisions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes QuickDup’s pairwise Jaccard similarity clustering by hoisting per-pattern token set construction out of the O(n²) comparison loop, significantly reducing allocation churn and CPU time during clustering.
Changes:
- Introduces
tokenSetto build per-pattern deduplicated token sets once and reuses them in the clustering pair loop. - Updates
tokenSimilarityto operate on prebuilt token sets and iterates the smaller set for faster intersection counting. - Adds differential tests/benchmarks to pin behavioral equivalence vs the pre-hoist implementation and quantify performance impact.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/quickdup/similarity.go | Hoists token set creation and updates similarity computation to use prebuilt sets in the pairwise loop. |
| pkg/quickdup/similarity_hoist_test.go | Adds pre/post equivalence tests and an A/B benchmark harness for the pairwise loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+77
to
+83
| func tokenSet(tokens []string) map[string]bool { | ||
| set := make(map[string]bool, len(tokens)) | ||
| for _, t := range tokens { | ||
| set[t] = true | ||
| } | ||
| return set | ||
| } |
Comment on lines
102
to
107
| intersection := 0 | ||
| for t := range setA { | ||
| if setB[t] { | ||
| for t := range small { | ||
| if large[t] { | ||
| intersection++ | ||
| } | ||
| } |
Comment on lines
+145
to
+152
| func TestPairLoopsAgree(t *testing.T) { | ||
| for _, n := range []int{50, 200} { | ||
| locs := benchLocations(n) | ||
| if got, want := newPairLoop(locs, 0.8), oldPairLoop(locs, 0.8); got != want { | ||
| t.Fatalf("n=%d: new loop matched %d pairs, old matched %d", n, got, want) | ||
| } | ||
| } | ||
| } |
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.
Problem
tokenSimilaritytook two[]stringand allocated amap[string]boolfor each on every call.clusterBySimilaritycalls it once per pattern pair, so every pattern's set was rebuiltn-1times —2*O(n²)map allocations per clustering pass.Found in a 180s CPU profile of a Faktorial daemon:
clusterBySimilaritywas 29.9% of consumed CPU, and its allocation churn drove most of the 23% spent inruntime.scanobject.Change
Build each token set once in the tokenize pass that already runs, and pass the prebuilt sets in. Jaccard math and all edge cases unchanged. Comparison now iterates the smaller set, since the result is symmetric.
Measured
Both implementations in one binary (
BenchmarkPairLoop), so this is a like-for-like A/B:Equivalence
similarity_hoist_test.gopins equivalence rather than leaning on the existing suite:TestTokenSimilarityMatchesPreHoist— exact float equality against a verbatim copy of the old function across 20,000 randomized pairs, plus explicit empty / one-empty / duplicate-heavy / reordered edge cases.TestPairLoopsAgree— pins that the benchmarked A/B pair reaches identical clustering decisions, so the timings compare like with like.go build,go vet, and the full suite (101 tests) pass on the rebased tree.🤖 Generated with Claude Code