Make the tests run faster #4808
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
irevoire
force-pushed
the
make-the-tests-run-faster
branch
2 times, most recently
from
cfbd077
to
d1aed82
Compare
…unning in release mode
…when it's not required
irevoire
force-pushed
the
make-the-tests-run-faster
branch
from
2eb34df
to
9372c34
Compare
dureuill
reviewed
Jul 30, 2024
|
|
||
| /// add_documents is not allowed on shared index but we need to use it to initialize | ||
| /// a bunch of very common indexes in `common/mod.rs`. | ||
| pub(super) async fn _add_documents( |
There was a problem hiding this comment.
this is test code so I don't really mind, but for the record I'm not a fan of having underscore functions here.
underscore has the specific meaning of "maybe unused" in Rust, so if these are internal functions I would either have made them free functions of the module, or if self contains a lot of data, I would have made an internal index object exposing the functions
There was a problem hiding this comment.
With the pub(super) honestly, it could simply be in the module, and without the _.
I’m merging as-is because the PR is already huge, but if you want, I can do either:
- Remove the
_and let the method in the module - Free functions of the module
|
bors merge |
27 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Index-Scheduler
Only check the consistency of the index-scheduler on snapshots when running in release mode
This saves 12s on the tests, and since the tests run in release mode in the CI, we don't lose any information.
From 28s to 16s
We were snapshotting the index for no reason in
advance_till, I removed this callThis saved an additional 8s on the tests, going from 16s to 8s.
After these two optimizations, the test suite as a whole executes 14% quicker
Meilisearch integration tests
While profiling this test suite, nothing stands out. The only noticeable thing is that we're losing most of our time creating and dropping threads.
I made the theory that by sharing a single common instance between all integrations tests I would gain some time again.
In 355a7ac I saved another 15s by only testing this theory on the module that tests the error messages.
But we can do it on many more tests. We must take care of not making any test flaky, though.
Use two indexing threads
By moving from one to two indexing threads, we gain an additional 30% in performance.
Conclusion
Before
The execution of the test suite was taking around:
After
The execution of the test suite is taking around:
This means the test suite should now run ~50% faster on your computer; the CI may report errors twice faster, but we'll still wait for ~the same amount of time to merge a PR