Follow up on post-merge review of performance work on sharded searcher

[mrnugget]

This adresses the comments by @keegancsmith in #14 after the merge.

I'm quoting the commit message of the first commit here:

This commit does a couple of things to make the added behavior correct:

1. It reuses the simplified query produced by selectRepoSet which
   in turn reduces the work the shards have to do when evaluating the
   query
2. It only evaluates the query.RepoSet if it's part of a top-level
   query.And. The previous code was too naive when considering that a
   query.RepoSet can appear anywhere in a query.Q. See this comment
   for more details:
   Improve performance of sharded search when using RepoSets #14 (comment)
3. It changes the const value that's returned in the simplified version
   so that typeRepoSearcher, which evaluates query.TypeRepo queries to
   a query.RepoSet works (the tests in shards/eval_test.go break
   without this.

I think the behavior is now correct.

I don't think it's worth it, but if the usecase of multiple RepoSet queries ever arises we can easily remove the break and use a set to not append duplicates to the filtered slice.

[keegancsmith]

I think you can just always return here, don't have to check len of filtered.

Also return query.Simplify(and), to make Simplify do nothing per shard.

[mrnugget]

Hmm, I can take a look again, but when I tried that (returning the simplified version unconditionally) the tests for TypeRepo broke because it’s using a RepoSet.

…

On Sat 20. Jul 2019 at 15:17, Keegan Carruthers-Smith < ***@***.***> wrote: ***@***.**** approved this pull request. ------------------------------ In shards/shards.go <#15 (comment)>: > - return &query.Const{Value: true} - }) - query.Simplify(eval) - - if len(filtered) != 0 { - return filtered + // Stop after first RepoSet, otherwise we might append duplicate + // shards to `filtered` + if len(filtered) != 0 { + return filtered, and I think you can just always return here, don't have to check len of filtered. Also return query.Simplify(and), to make Simplify do nothing per shard. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15?email_source=notifications&email_token=AAJBLZN5OIQ3MVVMV4S3SCDQAMF63A5CNFSM4IFGSJ2KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB7B6POY#pullrequestreview-264497083>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJBLZIKSC5ZGXBRVOMZ4ZLQAMF63ANCNFSM4IFGSJ2A> .

[keegancsmith]

Tried out add Simplify, and the tests pass. Also the benchmarks show an improvement:

name                                                 old time/op    new time/op    delta
ShardedSearch/substring_all_results-8                   1.54s ±12%     1.48s ± 8%     ~     (p=0.421 n=5+5)
ShardedSearch/substring_no_results-8                   5.43ms ± 2%    5.31ms ± 5%     ~     (p=0.548 n=5+5)
ShardedSearch/substring_some_results-8                 11.3ms ± 2%    10.6ms ± 3%   -5.99%  (p=0.008 n=5+5)
ShardedSearch/substring_all_results_and_repo_set-8      665ms ± 3%     659ms ± 3%     ~     (p=0.421 n=5+5)
ShardedSearch/substring_some_results_and_repo_set-8    6.21ms ± 4%    6.10ms ± 6%     ~     (p=0.690 n=5+5)
ShardedSearch/substring_no_results_and_repo_set-8      3.49ms ± 4%    3.25ms ± 3%   -6.96%  (p=0.008 n=5+5)

name                                                 old alloc/op   new alloc/op   delta
ShardedSearch/substring_all_results-8                  2.98GB ± 0%    2.98GB ± 0%     ~     (p=1.000 n=5+5)
ShardedSearch/substring_no_results-8                   8.35MB ± 0%    8.35MB ± 0%     ~     (p=0.286 n=4+5)
ShardedSearch/substring_some_results-8                 18.4MB ± 0%    18.4MB ± 0%     ~     (p=1.000 n=5+5)
ShardedSearch/substring_all_results_and_repo_set-8     1.50GB ± 0%    1.50GB ± 0%   -0.01%  (p=0.008 n=5+5)
ShardedSearch/substring_some_results_and_repo_set-8    9.32MB ± 0%    9.13MB ± 0%   -2.06%  (p=0.016 n=5+4)
ShardedSearch/substring_no_results_and_repo_set-8      4.59MB ± 0%    4.40MB ± 0%   -4.18%  (p=0.008 n=5+5)

name                                                 old allocs/op  new allocs/op  delta
ShardedSearch/substring_all_results-8                   27.2M ± 0%     27.2M ± 0%     ~     (p=0.686 n=4+4)
ShardedSearch/substring_no_results-8                     105k ± 0%      105k ± 0%     ~     (all equal)
ShardedSearch/substring_some_results-8                   240k ± 0%      240k ± 0%     ~     (p=1.000 n=4+5)
ShardedSearch/substring_all_results_and_repo_set-8      13.6M ± 0%     13.6M ± 0%   -0.04%  (p=0.008 n=5+5)
ShardedSearch/substring_some_results_and_repo_set-8      126k ± 0%      120k ± 0%   -4.75%  (p=0.000 n=5+4)
ShardedSearch/substring_no_results_and_repo_set-8       58.6k ± 0%     52.6k ± 0%  -10.24%  (p=0.008 n=5+5)

[mrnugget]

Ah, sweet! Thanks for that! I guess I had a combination of the previous version and this that didn't work.