Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Refactor search benchmarks #756

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Refactor search benchmarks #756

wants to merge 1 commit into from

Conversation

loiclec
Copy link
Contributor

@loiclec loiclec commented Dec 21, 2022

Refactor and extend the search benchmarks.

  1. Use the new search parameter to test the set-based, iterative, and dynamic implementations independently. This allows us to surface performance improvements or regressions that could be hidden in the past. e.g. if there's a regression in the iterative implementation, but the benchmark just happened to use the set-based version all the time, we wouldn't detect the regression.
  2. Avoid reindexing the dataset between groups of searches. Instead, the dataset is indexed only once at the beginning, and then the settings are changed between each group of searches if needed.
  3. Avoid collecting and analysing the time that each individual search query takes. Instead, collect the total time that the whole group of search requests took. This will speed up the benchmarks and make it easier to read their results. Note that we should then try to group searches by similarity.
  4. Add new search requests that are known to have performance problems. Previously, the benchmarks only tested relatively easy search requests.
  5. Limit the number of samples used by the benchmarking tool to 10. This will also speed up the benchmarks, at the cost of adding some noise. However, I don't think noise will be a big problem, since we mostly want to detect large changes in performance. And since the benchmarks are now fast, we can also easily re-run them.

@loiclec loiclec added benchmarks Related to the benchmarks no breaking The related changes are not breaking (DB nor API) labels Dec 21, 2022
@irevoire
Copy link
Member

Seen with @loiclec:
We’re waiting for @Kerollmops to come back from vacation because, if merged as-is this will break all the measurements we already have.

@Kerollmops
Copy link
Member

We’re waiting for @Kerollmops to come back from vacation because, if merged as-is this will break all the measurements we already have.

Talked about that in a meeting. It is ok for me to merge this even if it breaks the relation with the previous ones. This is not an issue we want to see garphs going down starting from this PR 📉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
benchmarks Related to the benchmarks no breaking The related changes are not breaking (DB nor API)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants