perf(vector): prepare embeddings once and add portable SIMD scoring - #218
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 27, 2026, 5:15 PM ET / 21:15 UTC. ClawSweeper reviewWhat this changesThe branch prepares embeddings once for cluster and exact-neighbor scoring, adds portable SIMD kernels, and enables SIMD in release and Docker builds. Merge readiness⛔ Blocked before merge - 3 items remain Keep open. Current main and v0.12.0 lack this optimization, and the measured patch appears sound. Shipping the experimental SIMD path in official builds still needs an explicit maintainer decision. Priority: P2 Review scores
Verification
How this fits togetherGitcrawl stores embeddings for synced GitHub threads. Clustering compares them to form durable groups, while exact-neighbor search ranks related threads. flowchart LR
A[Stored thread embeddings] --> B[Validate and scale vectors]
B --> C[Cosine scoring]
C --> D{Use case}
D --> E[Threshold cluster edges]
E --> F[Durable clusters]
D --> G[Rank exact neighbors]
G --> H[Neighbor results]
Decision needed
Why: The measured speedup and current test coverage support the code path, but choosing an experimental dependency for every distributed binary is a release policy decision that the patch and bot-only discussion cannot settle. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain prepared scalar scoring in every build and ship SIMD in official artifacts only after the release owner explicitly accepts the experimental API maintenance commitment. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug reproduction. The captured PR body gives controlled 2,000-thread cluster and 20,000-thread neighbor benchmark scenarios with before-and-after measurements. Is this the best way to solve the issue? Yes for avoiding repeated vector preparation in the two scoring paths; the release-wide experimental build choice needs explicit acceptance. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against 0a4cefe1858b. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Summary
Makes
clusterand exactneighborsmuch faster, in two layers:vector.Cosinere-validated both vectors for NaN/Inf, rescaled them by max-abs, and accumulated dot plus both magnitudes on every call. The O(n²) cluster loop repeated that work for every pair. The newvector.Preparedoes validation, scaling and the magnitude once per vector, so each pair costs a single dot product. Scores are bit-identical toCosine(same scaled values, same summation order), andCosineitself is unchanged. The pairwise loop is extracted asscoreClusterEdgeswithout behaviour changes.queryExactprepares the query once and reuses one candidate buffer.GOEXPERIMENT=simd). Uses Go 1.27's experimentalsimdpackage for the prepared dot product and for preparation (NaN/Inf detection via masks, scaling, magnitude), with four independent float64 accumulators. Emulated or unsupported hardware andGODEBUG=simd=0fall back to the scalar kernels..goreleaser.yaml(both build blocks) and theDockerfilenow build withGOEXPERIMENT=simd, because SIMD cleared the >1.3× bar over the new scalar default on user-visible operations. The trade-off is that release binaries depend on an experimental API that may change in Go 1.28. When the toolchain is bumped, a breaking change would fail the build loudly rather than silently. To support that, CI now runs the full test suite underGOEXPERIMENT=simd, since that's the configuration that ships, plus a forced-emulation pass. If you'd rather not ship an experiment, drop the two goreleaser lines and the Dockerfile flag; the scalar improvement stands on its own.Benchmarks
Default model
text-embedding-3-smallat gitcrawl's 1,024 dimensions, Apple M3 Ultra, Go 1.27.1,-count=10. The shared host was heavily loaded, so intervals are wide:GOEXPERIMENT=simd)cluster, 2,000 threadsA maintainer re-run measured cluster edges at 2.25 s (default) against about 1.0 s (SIMD), and neighbors at 61 ms against 33 ms. Cluster preparation holds one extra float64 copy of the vectors (about 16 MiB at 2,000 × 1,024).
A float32 SIMD variant was also evaluated. It was another 2.2× faster, but max cosine error reached 1.5e-6 at 3,072 dims, which is over the 1e-6 budget, so float64 stays.
Correctness
go test ./...andGOEXPERIMENT=simd go test ./...pass. So do the forced-emulation run,go vetin both modes,make check(85.7% coverage), and amd64 SIMD tests under Rosetta (128-bit). GoReleaser snapshot builds pass for all six SIMD targets.Storepanic in a release binary.