Improve performance of sharded search when using RepoSets

[mrnugget]

No description provided.

[keegancsmith]

LGTM so far. Mind sharing the benchmark output as is?

[mrnugget]

Current (before) benchmark output:

go test -run ^$ -bench=ShardedSearch -count 5 ./shards | tee ~/old.txt
goos: darwin
goarch: amd64
pkg: github.com/google/zoekt/shards
BenchmarkShardedSearch/substring-8                     1        1990092959 ns/op        2982246896 B/op 27177514 allocs/op
BenchmarkShardedSearch/substring-8                     1        1720852606 ns/op        2982222784 B/op 27177228 allocs/op
BenchmarkShardedSearch/substring-8                     1        1643912181 ns/op        2982224400 B/op 27177237 allocs/op
BenchmarkShardedSearch/substring-8                     1        1932622855 ns/op        2982214896 B/op 27177203 allocs/op
BenchmarkShardedSearch/substring-8                     1        1747838090 ns/op        2982222128 B/op 27177234 allocs/op
BenchmarkShardedSearch/substring_and_repo_set-8                        2         738268420 ns/op        1504994896 B/op 13618665 allocs/op
BenchmarkShardedSearch/substring_and_repo_set-8                        2         696805344 ns/op        1504992688 B/op 13618657 allocs/op
BenchmarkShardedSearch/substring_and_repo_set-8                        2         770910385 ns/op        1504991272 B/op 13618653 allocs/op
BenchmarkShardedSearch/substring_and_repo_set-8                        2         775941308 ns/op        1504991424 B/op 13618651 allocs/op
BenchmarkShardedSearch/substring_and_repo_set-8                        2         741441679 ns/op        1504990616 B/op 13618652 allocs/op
PASS
ok      github.com/google/zoekt/shards  27.494s

Comparing the output against itself to get human-readable numbers via benchstat:

$ cp ~/old.txt ~/old2.txt
$ benchstat ~/old.txt ~/old.txt
name                                    old time/op    new time/op    delta
ShardedSearch/substring-8                  1.81s ±10%     1.81s ±10%   ~     (p=1.000 n=10+10)
ShardedSearch/substring_and_repo_set-8     745ms ± 6%     745ms ± 6%   ~     (p=1.000 n=10+10)

name                                    old alloc/op   new alloc/op   delta
ShardedSearch/substring-8                 2.98GB ± 0%    2.98GB ± 0%   ~     (p=1.000 n=8+8)
ShardedSearch/substring_and_repo_set-8    1.50GB ± 0%    1.50GB ± 0%   ~     (p=1.000 n=10+10)

name                                    old allocs/op  new allocs/op  delta
ShardedSearch/substring-8                  27.2M ± 0%     27.2M ± 0%   ~     (p=1.000 n=8+8)
ShardedSearch/substring_and_repo_set-8     13.6M ± 0%     13.6M ± 0%   ~     (p=1.000 n=10+10)

[mrnugget]

I've added a few sub-benchmarks that allow us to quickly see what impact a change has when (a) having no results (b) some results or (c) all (every file matches) results.

Now, here's the impact of the optimizations in a161fb5:

benchstat ~/before_all.txt ~/after_all.txt
name                                                 old time/op    new time/op    delta
ShardedSearch/substring_all_results-8                   1.56s ± 7%     1.57s ± 4%     ~     (p=0.780 n=9+10)
ShardedSearch/substring_no_results-8                   5.44ms ± 3%    5.29ms ± 3%   -2.71%  (p=0.023 n=10+10)
ShardedSearch/substring_some_results-8                 11.4ms ± 4%    11.2ms ± 4%   -2.28%  (p=0.015 n=10+10)
ShardedSearch/substring_all_results_and_repo_set-8      684ms ± 6%     686ms ±11%     ~     (p=0.579 n=10+10)
ShardedSearch/substring_some_results_and_repo_set-8    7.87ms ±30%    5.95ms ± 4%  -24.43%  (p=0.000 n=10+10)
ShardedSearch/substring_no_results_and_repo_set-8      4.87ms ±13%    3.23ms ± 4%  -33.67%  (p=0.000 n=9+10)

name                                                 old alloc/op   new alloc/op   delta
ShardedSearch/substring_all_results-8                  2.98GB ± 0%    2.98GB ± 0%     ~     (p=0.863 n=9+9)
ShardedSearch/substring_no_results-8                   8.35MB ± 0%    8.35MB ± 0%     ~     (p=0.267 n=9+9)
ShardedSearch/substring_some_results-8                 18.4MB ± 0%    18.4MB ± 0%     ~     (p=0.853 n=10+10)
ShardedSearch/substring_all_results_and_repo_set-8     1.51GB ± 0%    1.50GB ± 0%   -0.07%  (p=0.000 n=10+10)
ShardedSearch/substring_some_results_and_repo_set-8    10.3MB ± 0%     9.2MB ± 0%  -10.76%  (p=0.000 n=10+9)
ShardedSearch/substring_no_results_and_repo_set-8      5.61MB ± 0%    4.50MB ± 0%  -19.86%  (p=0.000 n=10+10)

name                                                 old allocs/op  new allocs/op  delta
ShardedSearch/substring_all_results-8                   27.2M ± 0%     27.2M ± 0%     ~     (p=0.931 n=9+9)
ShardedSearch/substring_no_results-8                     105k ± 0%      105k ± 0%     ~     (all equal)
ShardedSearch/substring_some_results-8                   240k ± 0%      240k ± 0%     ~     (p=0.789 n=10+10)
ShardedSearch/substring_all_results_and_repo_set-8      13.6M ± 0%     13.6M ± 0%   -0.17%  (p=0.000 n=10+10)
ShardedSearch/substring_some_results_and_repo_set-8      150k ± 0%      128k ± 0%  -14.98%  (p=0.000 n=10+10)
ShardedSearch/substring_no_results_and_repo_set-8       82.5k ± 0%     60.0k ± 0%  -27.25%  (p=0.000 n=10+10)

Even though it's 33%, I think the runtime optimizations are probably negligible. The allocation improvements, on the other hand, are interesting.

[tsenart]

FYI: benchstat works with a single file of benchmark results, no need to compare it with itself.

[tsenart]

Nice work!

[keegancsmith]

nice work! This works as is, but got some inline feedback so it makes less assumptions about the sort of queries we pass in at the moment.