Skip to content
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.

Sign up for GitHub

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

Jump to bottom

tests: speed up builds with single-binary test bundles #14772

Closed
wants to merge 143 commits into from
Closed

tests: speed up builds with single-binary test bundles #14772

wants to merge 143 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 3, 2024 •
edited
Loading

Add support for single-block binaries that contain all libtests and
unit tests respectively.

Enable with:

  • autotools: --enable-test-bundles
  • cmake: -DCURL_TEST_BUNDLES=ON

(They are compatible with --enable-unity and -DCMAKE_UNITY_BUILD=ON
options, for further speed-up.)

Makes libtests and unit tests build fast, needing little disk space
even in static mode. Similar to CMake unity mode, but with a custom
script, also supporting autotools builds.

The price is having to deal with symbols/macros colliding between
lib*.c and unit*.c sources. Maybe with naming conventions or other
solutions this can be improved gradually and reduce the need for manual
intervention by mk-bundle.mk. I've included a script that does the bulk
of detecting name collisions.

Also:

  • CI: enable test bundles.
  • CI: build tests in more jobs:
    • aws-lc cmake +6s
    • linux-mingw cmake +12s
  • lib2305: fix FILE handle leak.
  • unit1661: fix memleak found by torture test by releasing the bufref
    structure in unit_stop() that was allocated in unit_setup(). 
    test 1661...[bufref unit tests]
Leak detected: memory still allocated: 13 bytes
 allocated by /home/runner/work/curl/curl/tests/unit/unit1661.c:70
 1661: torture FAILED: function number 1 in test.
    Ref: https://github.com/curl/curl/actions/runs/10967279334/job/30456745290?pr=14772#step:8:41

Similar test suite builds with autotools default and cmake+bundle+unity:

Autotools test suite builds compared between master → --enable-test-bundles:

Cmake test suite builds compared between master → -DCURL_TEST_BUNDLES=ON + unity:

@vszakats vszakats marked this pull request as draft September 3, 2024 22:10
@vszakats vszakats changed the title [WIP] tests: bundles for libtests and unit tests [WIP] tests: build bundles of libtests and unit tests Sep 3, 2024
@github-actions github-actions bot added the CI Continuous Integration label Sep 4, 2024
@dfandrich
Copy link
Contributor

Analysis of PR #14772 at 2bdff798:

Test 500 failed, which has NOT been flaky recently, so there could be a real issue in the PR. Note that this test has failed in 9 different CI jobs (the link just goes to one of them).

Test 504 failed, which has NOT been flaky recently, so there could be a real issue in the PR. Note that this test has failed in 9 different CI jobs (the link just goes to one of them).

Test 501 failed, which has NOT been flaky recently, so there could be a real issue in the PR. Note that this test has failed in 9 different CI jobs (the link just goes to one of them).

Test 502 failed, which has NOT been flaky recently, so there could be a real issue in the PR. Note that this test has failed in 9 different CI jobs (the link just goes to one of them).

Test 507 failed, which has NOT been flaky recently, so there could be a real issue in the PR. Note that this test has failed in 9 different CI jobs (the link just goes to one of them).

Test 508 failed, which has NOT been flaky recently, so there could be a real issue in the PR. Note that this test has failed in 9 different CI jobs (the link just goes to one of them).

Test 510 failed, which has NOT been flaky recently, so there could be a real issue in the PR. Note that this test has failed in 9 different CI jobs (the link just goes to one of them).

There are more failures, but that's enough from Gha.

Generated by Testclutch

@vszakats vszakats force-pushed the tests-bundle branch 2 times, most recently from a09ae03 to 3741f97 Compare September 5, 2024 10:31
@vszakats vszakats marked this pull request as ready for review September 5, 2024 18:07
@vszakats
Copy link
Member Author

vszakats commented Sep 5, 2024 •
edited
Loading

This is ready now for the most part.

Remains is to set it up in CI without enabling these features by default.
Though, if the reception is good, we might as well enable it by default.
It also gives a significant speed (and disk consumption) improvements
in local builds.

It may make building and running tests more "accessible".

@vszakats vszakats changed the title [WIP] tests: build bundles of libtests and unit tests tests: add option to build libtests and unit tests into single-binary bundles Sep 5, 2024
@vszakats vszakats changed the title tests: add option to build libtests and unit tests into single-binary bundles tests: add option to build tests into single-binary bundles Sep 5, 2024
@vszakats vszakats changed the title tests: add option to build tests into single-binary bundles tests: speed up building tests with single-binary bundles Sep 6, 2024
@vszakats vszakats force-pushed the tests-bundle branch 2 times, most recently from 4c20cb4 to d84875b Compare September 6, 2024 15:36
@vszakats vszakats mentioned this pull request Sep 6, 2024
Closed
5 tasks
@vszakats vszakats changed the title tests: speed up building tests with single-binary bundles tests: speed up builds with single-binary test bundles Sep 8, 2024
@vszakats vszakats force-pushed the tests-bundle branch 2 times, most recently from 073d0b5 to 89fa5f4 Compare September 10, 2024 21:32
@vszakats vszakats mentioned this pull request Sep 14, 2024
Closed
@vszakats vszakats force-pushed the tests-bundle branch 2 times, most recently from 098c2f0 to 089782c Compare September 15, 2024 14:02
@vszakats vszakats mentioned this pull request Sep 15, 2024
Closed
3 tasks
@vszakats vszakats force-pushed the tests-bundle branch 2 times, most recently from 318e04d to 2f8f53e Compare September 19, 2024 19:37
@vszakats
Copy link
Member Author

Apart from one pending question/decision: #14772 (comment),
this is ready to merge.

(The same optimization can be done for tests/server, but I'll do that in a separate PR.)

@bagder
Copy link
Member

bagder commented Sep 21, 2024

I'm wondering if bundling tests into a single executable reduces the effectivenes of torture tests for libcurl code?

I don't follow. Why would they? Are the bundled versions of the tests executing more/other code than the "regular" builds?

@vszakats
Copy link
Member Author

vszakats commented Sep 21, 2024 •
edited
Loading

I'm wondering if bundling tests into a single executable reduces the effectivenes of torture tests for libcurl code?

I don't follow. Why would they? Are the bundled versions of the tests executing more/other code than the "regular" builds?

They don't, besides a short lookup / dispatch in main() (that's not using malloc()). There is of course much more code in the binary, but all is idle besides the actual test executed.

It was curious the bundles caught two new leaks, that were never caught before. But this was more like a wild guess to explain it, and to not break something by accident.

@vszakats vszakats closed this in 71cf0d1 Sep 22, 2024
@vszakats vszakats deleted the tests-bundle branch September 22, 2024 07:52
@vszakats vszakats mentioned this pull request Sep 22, 2024
Closed
@vszakats vszakats mentioned this pull request Sep 22, 2024
Draft
7 tasks
vszakats added a commit that referenced this pull request Sep 22, 2024
@vszakats
GHA: revert some build test steps added by #14772
4b378ea 
They are still slow in these jobs/combinations.

- non-native/FreeBSD/arm64 autotools +36s
- non-native/FreeBSD/arm64 cmake +1m
- windows/linux-cross-mingw-w64 autotools +33s

These ones remain:
- linux/aws-lc cmake +6s
- windows/linux-cross-mingw-w64 cmake +12s

Follow-up to 71cf0d1 #14772
@bagder
Copy link
Member

bagder commented Sep 22, 2024

It was curious the bundles caught two new leaks, that were never caught before. But this was more like a wild guess to explain it, and to not break something by accident.

I have not seen those fixes so I cannot comment on why they were not found before. Can you point them out for me? I suspect maybe it rather indicatives that we might have include problems or something in the affected tests.

@vszakats
Copy link
Member Author

The two sub-commits with the fixes:

@bagder
Copy link
Member

bagder commented Sep 22, 2024

As I suspected, fix here: #15007

I'll ponder on a system where this can be caught better.

@vszakats
Copy link
Member Author

Oh right, indeed, nice catch!

@vszakats vszakats mentioned this pull request Sep 22, 2024
Closed
vszakats added a commit to vszakats/curl that referenced this pull request Sep 22, 2024
@vszakats
CI: add missed updates to the configure-libssh job
489b81d 
Disable dependency tracking and enable unity + test bundles for
the `configure-libssh` job that was missed in earlier commits.

Follow-up to 71cf0d1 curl#14772
Follow-up to dff6619 curl#14975
@vszakats vszakats mentioned this pull request Sep 22, 2024
Closed
vszakats added a commit that referenced this pull request Sep 22, 2024
@vszakats
CI: add missed updates to the configure-libssh job
8e7ccfd 
Disable dependency tracking and enable unity + test bundles for
the `configure-libssh` job that was missed in earlier commits.

Follow-up to 71cf0d1 #14772
Follow-up to dff6619 #14975

Closes #15010
@vszakats vszakats mentioned this pull request Sep 22, 2024
Closed
vszakats added a commit that referenced this pull request Sep 23, 2024
@vszakats
build: fix possible -Wformat-overflow in lib557 with test bundle bu…
4619b41 
…ilds

- lib557: suppress `-Wformat-overflow` warning in source.
  Fixes:
  ```
  lib557.c: In function ‘test_float_formatting’:
  lib557.c:1408:37: error: ‘%*f’ directive output of 2147483648 bytes exceeds ‘INT_MAX’ [-Werror=format-overflow=]
   1408 |   curl_msnprintf(buf, sizeof(buf), "%*f", INT_MIN, 9.1);
        |                                     ^~~
  lib557.c:1408:3: note: ‘curl_msnprintf’ output 2147483649 bytes
   1408 |   curl_msnprintf(buf, sizeof(buf), "%*f", INT_MIN, 9.1);
        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ```
  Ref: https://app.circleci.com/pipelines/github/curl/curl/10226/workflows/87642ee9-cda6-4916-8206-c82aac5f595e/jobs/107669?invite=true#step-106-40996_46

  The root cause of why this option gets enabled remains undiscovered.

  Reported-by: Daniel Stenberg
  Fixes #15008
  Follow-up to 71cf0d1 #14772

- build: drop `-Wno-format-overflow` from picky warning list.
  These options only get used with picky warnings enabled.
  Follow-up to 145f87b #14598

- unit1652: suppress in source (and not rely on picky warnings anymore.)

Closes #15012
vszakats added a commit to vszakats/curl that referenced this pull request Sep 23, 2024
@vszakats
GHA/linux: enable test bundles for cmake jobs
1d2142c 
Test build step speed-up (3x): 18s -> 6s

Follow-up to 71cf0d1 curl#14772
@vszakats vszakats mentioned this pull request Sep 23, 2024
Closed
vszakats added a commit that referenced this pull request Sep 24, 2024
@vszakats
GHA/linux: enable test bundles for cmake jobs
9b3a7d1 
Test build step speed-up (3x): 18s -> 6s

Follow-up to 71cf0d1 #14772
Closes #15022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build CI Continuous Integration performance tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vszakats @dfandrich @bagder