-
Notifications
You must be signed in to change notification settings - Fork 37.7k
ci: Place datadirs for tests under tmpfs #31182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31182. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
fa62102
to
faabc18
Compare
Using tmpfs for temporary test directories can potentially speed up tests a lot, something to be wary for is that it significantly increases memory usage as well (no idea if this is a problem for the current CI). |
faabc18
to
bbbb53b
Compare
I'd say the memory increase shouldn't matter, because on modern distros the default temp dir is already a tmpfs. So if memory was an issue, then running the tests normally (outside the CI) would already lead to issue reports. (I haven't seen any, but I may be missing some). So I think it is fine to enable this by default without an off-switch for now. If there is need, a new env var toggle can be added to disable it. |
i don't think anyone runs the tests in low-memory/disk conditions 😄 i've had some issues on custom RISC-V hardware in the past, but nothing significant enough to create an issue about. At least the CI will start with a clean slate every time so no cruft accumulates in tmpfs. |
bbbb53b
to
fa26642
Compare
(force pushed to fix a type bug in the modified Python code, discovered by the Windows CI. I wish this code was written in Rust or another type-safe language, so that issues like this are caught at compile-time, but this can be done in a follow-up) |
What is this referring to? |
I was still running the commit (and master) twice each for all CI tasks, but I couldn't find any difference at all. I checked that the folder is correctly picked up and used by the tests, so I guess modern storage is fast enough to not make a difference? Alternatively, there is an issue I was missing. It would be good if someone else double checked the code and whether there is a performance difference at all. For reference (showing no difference):
|
That is odd, and a little disappointing. The code appears correct to me. I was wondering whether I will run the CI on my own self-hosted runner, and see what sort of numbers I get from it... |
My plan is to get hold on an HDD (or some other slow storage) and try with that. Any effect should be more pronounced on slower persistent storage. |
Also, includes a fixup to the fuzz/test_runner.py to pass on the env var.
fa26642
to
fa0081b
Compare
I tested it on a Pi 4B, with 4GB of memory, Ubuntu 24.04, using 1 CPU thread: Not sure why, but I guess modern Linux will cache the file IO sufficiently to not make a difference for this projects' CI? Whereas using tmpfs with limited memory may be a pessimization due to the overhead of swap? Closing for now, until someone can show how to actually improve the performance with this. |
This should speed up the tests a bit (see comment below for results).