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.

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

feat: fast build environment variables #12509

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Conversation

juanjux
Copy link
Collaborator

@juanjux juanjux commented Feb 25, 2025

Description

  • Adds DD_FAST_BUILD which, if present, should halve the time to build the project (in my machine, 146 seconds without to 69 seconds without sccache) or be 4x faster if previously catched with sccache (44 seconds to 10) by exchanging some optimizations (disable abseil lib in IAST, -O0) for better build time. To be used for development and CI (CI config is also modified to use this env var, except for the main branch to catch corner cases that could happen only when optimized).

  • Adds DD_COMPILE_ABSEIL which, is present and "0" will disable the compilation of abseil in the native module independently of the release mode (currently is compiled by default if in Release mode only; this behavior will continue if DD_COMPILE_ABSEIL is not present).

  • Add a new "build" section in docs/configuration.rst with these two environment vars plus two that were not
    documented.

  • Fix an error in the documentation of DD_COMPILE_DEBUG (it enables Debug mode, not RelWithDebInfo as stated, and does it for all native extensions not only pyx ones). I will add an option to compile with RelWithDebInfo in a further PR.

Signed-off-by: Juanjo Alvarez [email protected]## Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@juanjux juanjux self-assigned this Feb 25, 2025
Copy link
Contributor

github-actions bot commented Feb 25, 2025

CODEOWNERS have been resolved as:

.circleci/config.templ.yml                                              @DataDog/python-guild @DataDog/apm-core-python
ddtrace/appsec/_iast/_taint_tracking/CMakeLists.txt                     @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/TaintTracking/TaintRange.h         @DataDog/asm-python
ddtrace/appsec/_iast/_taint_tracking/tests/CMakeLists.txt               @DataDog/asm-python
hatch.toml                                                              @DataDog/python-guild
scripts/gen_gitlab_config.py                                            @DataDog/apm-core-python
setup.py                                                                @DataDog/python-guild

@juanjux juanjux added the changelog/no-changelog A changelog entry is not required for this PR. label Feb 25, 2025
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Feb 25, 2025

Datadog Report

Branch report: juanjux/fast-build-option
Commit report: d1cfb07
Test service: dd-trace-py

✅ 0 Failed, 43 Passed, 460 Skipped, 51.84s Total duration (9m 16.71s time saved)

@juanjux juanjux force-pushed the juanjux/fast-build-option branch from 7a2eddc to 72b311c Compare February 25, 2025 16:49
@pr-commenter
Copy link

pr-commenter bot commented Feb 25, 2025

Benchmarks

Benchmark execution time: 2025-02-26 17:26:13

Comparing candidate commit d1cfb07 in PR branch juanjux/fast-build-option with baseline commit b1cc34a in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 418 metrics, 2 unstable metrics.

Signed-off-by: Juanjo Alvarez <[email protected]>
@juanjux juanjux changed the title chore: fast build environment variable feast: fast build environment variables Feb 26, 2025
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
@juanjux juanjux marked this pull request as ready for review February 26, 2025 09:18
@juanjux juanjux requested review from a team as code owners February 26, 2025 09:18
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, mostly some nits and minor suggestions.

I didn't realize we had docs for the build/package env variables, there are a few I've added which are missing from that 🙈 (_DD_RUST_DEBUG, _DD_CYTHON_ANNOTATE, _DD_DEBUG_EXT... although looking at the other env vars in setup.py, not sure we need the _ prefix)

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! another thought, you updates to set DD_FAST_BUILD=1 for build_base_venvs, do we need to do the same for all hatch jobs?

@RamyElkest
Copy link
Contributor

Do you have documentation on why this is faster? -O0 is the default optimization iiuc

@juanjux
Copy link
Collaborator Author

juanjux commented Feb 26, 2025

Do you have documentation on why this is faster? -O0 is the default optimization iiuc

For cmake's Release builds (like the ones we use in CI), -O3 is used by default if not overridden with -DCMAKE_C_FLAGS_Release=-O0 like we do.

As for "why", not compiling abseil removed around 40% of the compilation time, and for -O0 I can only guess but I tested a lot of gcc parameters and this really made a difference, it shaved 50 seconds from an otherwise 170 seconds build.

@juanjux juanjux requested a review from brettlangdon February 26, 2025 16:54
@juanjux
Copy link
Collaborator Author

juanjux commented Feb 26, 2025

oh! another thought, you updates to set DD_FAST_BUILD=1 for build_base_venvs, do we need to do the same for all hatch jobs?

hatch updated

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last little non-blocking nit.

hatch.toml Outdated
[envs.appsec_iast_packages.env-vars]
CMAKE_BUILD_PARALLEL_LEVEL = "12"
CARGO_BUILD_JOBS = "12"
DD_FAST_BUILD = "1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just set this in the GitLab config?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for the parallel args?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants