Skip to content

Main.yml: Move very long job from debug to release with -DDEBUG and FORCE_ASSERT #18081

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
Merged
merged 2 commits into from
Jun 30, 2025
Merged

Main.yml: Move very long job from debug to release with -DDEBUG and FORCE_ASSERT #18081

merged 2 commits into from
Jun 30, 2025

Conversation

carlopi
Copy link
Contributor

@carlopi carlopi commented Jun 27, 2025 •
edited
Loading

Currently we run the standard test suite in debug mode, with the aim of testing slow verifiers masked by #ifdef DEBUG, enabling D_ASSERT, and running additional sanitizers.
This works, but has a drawback that compiling for debug is tied to -O0, that is no optimizations performed.

Proposal here is to run this particular step in release mode, while:

  • passing down -DDEBUG, so that slow verifiers masked by #ifdef DEBUG are still included
  • enabling assertions (those are also turned on when DEBUG is defined or in relassert mode)
  • enabling extra sanitizers, currently via the FORCE_ASSERT CMake flag

Changes as done here are local to the workflow, but we could consider adding another compilation mode, since this might be of general usage.

Currently, on each PR iteration: 2* (15 minutes building + 1h45' testing)
Moving to release + -DDEBUG -DFORCE_ASSERT: 20 minutes building + 25 minutes testing

Build step might also be cached, this is 10x improvement for this step, that is at the moment the most expensive one run on every PR.

@carlopi
Move very long workflow from debug to reldebug
a3a7ccf 
Currently, on each PR iteration: 2* (15 minutes building + 1h45' testing)
Moving from debug to reldebug: 2* (18 minutes building + 2' testing)
Moving from 2 parallel jobs to a single one: 18 minutes building + 4' testing

Considering building step might also be cached, this is 10x to 20x improvement (in this particular step, that was the more time consuming at the moment).
@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 27, 2025 14:16
@carlopi carlopi marked this pull request as ready for review June 27, 2025 14:18
@carlopi carlopi marked this pull request as draft June 27, 2025 14:19
@carlopi carlopi force-pushed the debug_to_reldebug branch from 175e08e to ca917f0 Compare June 27, 2025 14:19
@carlopi carlopi marked this pull request as ready for review June 27, 2025 14:19
@Mytherin
Copy link
Collaborator

Thanks - but we run debug for a reason. If we remove this debug job we run no more debug runs in CI meaning that all of the checks we have added to debug builds are no longer tested/verified.

Perhaps it makes more sense to revisit some of those checks to speed up debug builds.

@carlopi
Copy link
Contributor Author

carlopi commented Jun 28, 2025

I see, right, thanks!

Maybe it's as simple as adding -DDEBUG define a release compilation mode? I do see now we want to run the #ifdef DEBUG code blocks, but I don't think there is much value in coupling that with no optimizations.

I will check if that works.

@carlopi
Copy link
Contributor Author

carlopi commented Jun 28, 2025 •
edited
Loading

mode duckdb CLI binary size time to run default tests DEBUG define enabled
reldebug 40.99 MB 108.52s no
reldebug + -DDEBUG 44.29 MB 288.64s yes
debug 455.39 MB >> (still running) yes

Checks are performed (tested adding a static_assert(0) that was ignored in reldebug and not in the other 2 modes).

@Mytherin
Copy link
Collaborator

Mytherin commented Jun 28, 2025 •
edited
Loading

We should also run asserts then, right? Or does DEBUG also enable asserts?

@carlopi
Copy link
Contributor Author

carlopi commented Jun 28, 2025 •
edited
Loading

I think debug does enable asserts already, but I will check with a bogus D_ASSERT(false) in a taken codepath.
If not, should also be added, unsure if there are other catches.

I stopped after a while, but using the timing from CI where it's 2 minutes vs 100 minutes, I would say some form of release build + -DDEBUG should clock at 6 minutes or so, that is still significant to have.

@carlopi
Copy link
Contributor Author

carlopi commented Jun 28, 2025

Assertion do trigger when -DDEBUG is defined, unsure if there are other catches.

@Mytherin
Copy link
Collaborator

I think we're good then - I can't think of any reason not to make the switch

@Mytherin
Copy link
Collaborator

Actually hold on - we're not running sanitizers in this mode, right? Maybe that accounts for the difference.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 28, 2025 08:50
@carlopi
Copy link
Contributor Author

carlopi commented Jun 28, 2025

Maybe we look at this with some more care, iterating over CMake and checking stuff, maybe we need to bring in sanitizers and possibly other stuff, then the proper way it's not a single hack but refactoring CMake (so main, also on this there is no urgency really).

But I think basically between running with -O0 and -O3, expecially on our heavily templated code, 10x could be somewhat reasonable, and we would be happy to take any speed up here.

@carlopi carlopi changed the base branch from v1.3-ossivalis to main June 28, 2025 08:54
@carlopi carlopi changed the base branch from main to v1.3-ossivalis June 28, 2025 15:49
@carlopi carlopi marked this pull request as ready for review June 28, 2025 15:49
@carlopi carlopi force-pushed the debug_to_reldebug branch from ca1add8 to ee628ce Compare June 28, 2025 16:44
@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 28, 2025 16:45
@carlopi carlopi marked this pull request as ready for review June 28, 2025 16:45
@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 28, 2025 20:01
@carlopi carlopi force-pushed the debug_to_reldebug branch from 22e99c7 to e6efc1d Compare June 29, 2025 08:13
@carlopi carlopi marked this pull request as ready for review June 29, 2025 08:21
@carlopi
Copy link
Contributor Author

carlopi commented Jun 29, 2025

@Mytherin: updated PR message and approach, I think this should do what we want.

@carlopi carlopi changed the title Main.yml: Move very long job from debug to reldebug Main.yml: Move very long job from debug to release with -DDEBUG and FORCE_ASSERT Jun 29, 2025
@carlopi carlopi force-pushed the debug_to_reldebug branch from e6efc1d to 905e470 Compare June 29, 2025 08:45
@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 29, 2025 08:45
@carlopi carlopi marked this pull request as ready for review June 29, 2025 08:46
@carlopi carlopi force-pushed the debug_to_reldebug branch from 905e470 to 3134493 Compare June 29, 2025 08:50
@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 29, 2025 08:50
@carlopi carlopi marked this pull request as ready for review June 29, 2025 09:08
@Mytherin Mytherin merged commit 96719ee into duckdb:v1.3-ossivalis Jun 30, 2025
16 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

@carlopi carlopi deleted the debug_to_reldebug branch June 30, 2025 08:27
github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Jun 30, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@6267d4e
3943800 
fix statistics propagation for anti-joins on empty tables (duckdb/duckdb#17439)
Run Python workflow against both Python 3.9 and 3.13 on PR to ensure … (duckdb/duckdb#18080)
Main.yml: Move very long job from debug to release with `-DDEBUG` and `FORCE_ASSERT` (duckdb/duckdb#18081)
@github-actions github-actions bot mentioned this pull request Jun 30, 2025
Merged
github-actions bot added a commit to duckdb/duckdb-r that referenced this pull request Jun 30, 2025
@github-actions @krlmlr
vendor: Update vendored sources to duckdb/duckdb@6267d4e (#1317)
d04e9ba 
fix statistics propagation for anti-joins on empty tables (duckdb/duckdb#17439)
Run Python workflow against both Python 3.9 and 3.13 on PR to ensure … (duckdb/duckdb#18080)
Main.yml: Move very long job from debug to release with `-DDEBUG` and `FORCE_ASSERT` (duckdb/duckdb#18081)

Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
@carlopi carlopi mentioned this pull request Jul 1, 2025
Merged
Mytherin added a commit that referenced this pull request Jul 1, 2025
@Mytherin
OSX.yml: Move from using debug builds to release + DDEBUG + FORCE_ASS…
155a8a8 
…ERT (#18102)

Very minor, should have no real impact but shortening a bit CI checks on
forks and InvokeCI runs.

Equivalent to #18081
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
Ready For Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@carlopi @Mytherin