ci: Place datadirs for tests under tmpfs #31182

1. 
   maflcko commented at 10:12 am on October 30, 2024: member

   This should speed up the tests a bit (see comment below for results).
2. 
   DrahtBot commented at 10:12 am on October 30, 2024: contributor

   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.

3. maflcko force-pushed on Oct 30, 2024
4. maflcko force-pushed on Oct 30, 2024
5. maflcko force-pushed on Oct 30, 2024
6. 
   laanwj commented at 11:47 am on October 30, 2024: member

   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).
7. maflcko force-pushed on Oct 30, 2024
8. maflcko marked this as ready for review on Oct 30, 2024
9. 
   maflcko commented at 12:26 pm on October 30, 2024: member

   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.

10. 
    laanwj commented at 12:35 pm on October 30, 2024: member

    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.

11. maflcko force-pushed on Oct 30, 2024
12. 
    maflcko commented at 1:51 pm on October 30, 2024: member

    (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)
13. 
    TheCharlatan commented at 7:43 pm on October 30, 2024: contributor

    (see comment below for results).

    What is this referring to?

14. maflcko marked this as a draft on Oct 31, 2024
15. 
    maflcko commented at 7:55 am on October 31, 2024: member

    (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

    

    • This pull

    

16. 
    willcl-ark commented at 11:33 am on November 4, 2024: 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…

17. 
    maflcko commented at 11:50 am on November 4, 2024: member

    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.
18. DrahtBot added the label Needs rebase on Nov 11, 2024
19. ci: Place datadirs for tests under tmpfs

    Also, includes a fixup to the fuzz/test_runner.py to pass on the env
    var.

    fa0081bd0f
20. maflcko force-pushed on Nov 22, 2024
21. DrahtBot removed the label Needs rebase on Nov 22, 2024
22. DrahtBot renamed this:
    ci: Place datadirs for tests under tmpfs
    ci: Place datadirs for tests under tmpfs
    on Nov 22, 2024
23. DrahtBot added the label Tests on Nov 22, 2024
24. 
    maflcko commented at 12:54 pm on November 22, 2024: member

    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.

25. maflcko closed this on Nov 22, 2024

    ---

26. maflcko deleted the branch on Nov 22, 2024

Contributors
maflcko DrahtBot laanwj TheCharlatan willcl-ark

Labels
Tests