Repository navigation
perf(compute): vectorize mixed 64-bit filters - #1311
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
No SIMD data-path correctness issue found. Focused AVX2 kernel tests and compute filter tests passed, including the noasm fallback. ARM64 was inspected but not executed. One optional build-constraint correction below; this is not a runtime blocker.
| @@ -0,0 +1,79 @@ | |||
| //+build !noasm !appengine | |||
There was a problem hiding this comment.
Non-blocking: the space in this legacy +build constraint means OR, so noasm-only and appengine-only builds still include this assembly source. go list -tags noasm -f '{{.SFiles}}' ./arrow/compute/internal/kernels confirms it. The Go caller is excluded, so it remains unreachable. Please match the uint32 sibling with //go:build go1.18 && amd64 && !noasm && !appengine and preserve the constraint during regeneration.
zeroshade
left a comment
There was a problem hiding this comment.
Thanks — the build constraint is fixed, and the Makefile change goes further than I asked.
filter_uint64_avx2_amd64.s now carries //go:build go1.18 && amd64 && !noasm && !appengine, matching the uint32 sibling exactly. Verified by build rather than by eye — go list -tags noasm and -tags appengine both now exclude the file, on amd64 and arm64 alike.
The Makefile rewrite is a nice addition: regeneration will no longer clobber the constraint. The uint32 sibling still has no such guard and would be clobbered; worth a follow-up, not this PR.
Correctness re-verified. preallocateData sizes the output buffer to exactly outputLength * 8 and GetSpanValues applies out.Offset, so len(outData) is precisely the popcount; the exact-size stores stay in bounds, and the 0xdeadbeef sentinel in the test asserts that directly. Input side, len(valuesData) == values.Len == length and the explicit filterByteOffset+numBytes > len(filterData) check covers the filter. The uint64 wrapper is logic-identical to the already-merged uint32 one.
NEON read, not executed. I traced the hand-written kernel against the Go table generator: positions 0-1 land in V4 and 2-3 in V5, the 0x80 fill yields zeroed lanes that are never stored, popcount lives at tables[512+mask], stores are exactly 8/16/24/32 bytes, the pointer advances 64 bytes per filter byte, and g/FP/LR are untouched so NOFRAME|NOSPLIT is valid. I have no ARM64 hardware or qemu here, so the exhaustive mask test on NEON is still unexecuted on my side — arm64 cross-build and vet are clean, but that is not the same thing.
Tests. All 256 mask bytes, the aligned-offset path, and seven guard rejections. I confirmed the AVX2 tests actually run on this host rather than skipping. go test, -tags noasm, and -race all pass across ./arrow/compute/....
Perf on AVX2, since the PR reports only NEON. 1M elements, -benchtime=200x -count=5, medians:
| Case | main | PR | Speedup |
|---|---|---|---|
| alternating | 10.13 ms | 1.57 ms | 6.5x |
| random50 | 6.77 ms | 1.32 ms | 5.1x |
| clustered50 | 1.03 ms | 0.92 ms | 1.1x |
No regressions. A first pass at -benchtime=50x looked like clustered50 regressed ~14%, but that was noise. Worth noting separately: main's timings swing a lot (alternating ranged 4.7-18.4 ms across runs) while this stays tight at 1.43-1.76 ms — the SIMD path is branch-predictor-independent, which is a real benefit beyond the median.
Approving on the code. One process note: CI has never run here — head 6089dc0a has zero check-runs and combined status pending. I would not merge until the matrix is green, particularly since the ARM64 path is the one neither of us has executed.
Non-blocking: the perl substitution matches only legacy //+build. If c2goasm is ever updated to emit //go:build directly, the rewrite silently no-ops and the constraint reverts to whatever the generator produced. A check that the line was actually replaced would make it durable.
What changed
Benchmark
Apple M1 Pro, macOS arm64, Go 1.26.3. Median of five runs. The baseline uses GODEBUG=cpu.all=off.
The mixed-mask cases also go from 33 to 32 allocations per operation. The output allocation is still the main B/op cost.
Tests