Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Oct 30, 2024

This should speed up the tests a bit (see comment below for results).

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 30, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31182.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #31257 (ci: make ctest stop on failure by furszy)

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.

@maflcko maflcko force-pushed the 2410-ci-tmpfs branch 3 times, most recently from fa62102 to faabc18 Compare October 30, 2024 11:26
@laanwj
Copy link
Member

laanwj commented Oct 30, 2024

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).

@maflcko maflcko marked this pull request as ready for review October 30, 2024 12:20
@maflcko
Copy link
Member Author

maflcko commented Oct 30, 2024

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).

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.

@laanwj
Copy link
Member

laanwj commented Oct 30, 2024

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).

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.

@maflcko
Copy link
Member Author

maflcko commented Oct 30, 2024

(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)

@TheCharlatan
Copy link
Contributor

(see comment below for results).

What is this referring to?

@maflcko maflcko marked this pull request as draft October 31, 2024 07:46
@maflcko
Copy link
Member Author

maflcko commented Oct 31, 2024

(see comment below for results).

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):

  • Current master

Screen Shot 2024-10-31 at 08 53 24

  • This pull

Screen Shot 2024-10-31 at 08 53 14

@willcl-ark
Copy link
Member

That is odd, and a little disappointing. The code appears correct to me.

I was wondering whether tmpfs according to docker might just mean "ephemeral" (but still disk-based), but it appears that it it is both ephemeral and ramdisk based as per the underlying filesystem tmpfs.

I will run the CI on my own self-hosted runner, and see what sort of numbers I get from it...

@maflcko
Copy link
Member Author

maflcko commented Nov 4, 2024

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.
@DrahtBot DrahtBot changed the title ci: Place datadirs for tests under tmpfs ci: Place datadirs for tests under tmpfs Nov 22, 2024
@DrahtBot DrahtBot added the Tests label Nov 22, 2024
@maflcko
Copy link
Member Author

maflcko commented Nov 22, 2024

I tested it on a Pi 4B, with 4GB of memory, Ubuntu 24.04, using 1 CPU thread: CCACHE_MAXSIZE=1500M MAKEJOBS="-j1" FILE_ENV="./ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh" /usr/bin/time --format='%U+%S %e (%E)' ./ci/test_run_all.sh. However, I couldn't find any difference either. Moreover, with MAKEJOBS="-j4", this pull performed worse than the commit prior to it (55min vs 1h5min).

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.

@maflcko maflcko closed this Nov 22, 2024
@maflcko maflcko deleted the 2410-ci-tmpfs branch November 22, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants