-
Notifications
You must be signed in to change notification settings - Fork 470
chore(build): consolidate libdatadog symbols in lib_native #13984
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
Conversation
* Add new cmake file to find libnative components. * Modify dd_wrapper cmake in order to link against libnative. * Modify ddup cmake in order to link against libnative. * Change crashtracker compilation so it links agains libnative. Link to python runtime as a workaround to solve secondary dependencies from libnative.so (temporary until the crashtracker PyO3 bindings are ready).
…uild_standalone.sh
* Call build_libnative from project's root folder. * Fix FindLibNative in order not to include libnative whole folder structure. * Workaround test linking.
|
|
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 278 ± 2 ms. The average import time from base is: 280 ± 3 ms. The import time difference between this PR and base is: -2.2 ± 0.1 ms. Import time breakdownThe following import paths have grown:
|
* Link by library name instead of soname. * Pass extension suffix and native library as CMake variable. * Remove soname configuration in config.toml.
* Handle IS_EDITABLE when issuing pip install -e . * Set soname dynamically for _native library so auditwheel is able to locate it. * Set RPATH so the linker is able to locate the library without setting LD_LIBRARY_PATH.
fbfa05c to
97b05e7
Compare
117ef50 to
4f03739
Compare
…ent python version
build_base_venv jobs issuing cargo build in the same directory, it doesn't see build artifacts overwritten. However, this could potentially save some build time. I'll address this in a separate PR.
|
I've been seeing a very weird issue where Before this PR it was ok for There are few other things that I discovered while debugging this PR
|
Both profiling and src/native depend on libdatadog, but slightly different manner prior to this PR. - Profiling consumed statically built libdatadog libraries - src/native Rust crate simply added libdatadog crates as dependencies This PR makes sure that we build libdatadog dependency as part of src/native and export profiling ffi symbols there. Regarding benchmark SLOs: - Mostly increasing `max_rss_usage`. Looks ok and acceptable to @taegyunkim. We're changing the way we build and consume libdatadog dependency. [PROF-12235](https://datadoghq.atlassian.net/browse/PROF-12235?atlOrigin=eyJpIjoiNzM4ODY4ZTI3MjFjNGMwMjgzMGUwNjhkODhlNjA1NjIiLCJwIjoiaiJ9) ## Checklist - [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) [PROF-12235]: https://datadoghq.atlassian.net/browse/PROF-12235?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Signed-off-by: Taegyun Kim <[email protected]> Co-authored-by: Taegyun Kim <[email protected]> Co-authored-by: Gabriele N. Tornetta <[email protected]>
#13984 broke baseline:build job as 1) it upgraded in PyO3 version 2) changed how _taint_tracking is built ## Checklist - [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
## Description Use Python version specific `CARGO_TARGET_DIR` for src/native, Rust extension module. This has a few benefits 1. Parallelizing build_base_venv jobs. While working on #13984, I noticed that since build_base_venv jobs on GitLab run in the same dd-trace-py directory and use the same `CARGO_TARGET_DIR`, and there's cargo lock, only one of them could run at a time. 2. Re-use Rust build artifacts as much as possible. Consider a scenario one builds dd-trace-py using multiple different Python versions in a row. In that case, the build had to spend extra time after switching to a different Python version, as src/native has to be re-linked and re-built with different Python binaries. <!-- Provide an overview of the change and motivation for the change --> ## Testing <!-- Describe your testing strategy or note what tests are included --> ## Risks <!-- Note any risks associated with this change, or "None" if no risks --> ## Additional Notes <!-- Any other information that would be helpful for reviewers -->
Both profiling and src/native depend on libdatadog, but slightly different manner prior to this PR.
This PR makes sure that we build libdatadog dependency as part of src/native and export profiling ffi symbols there.
Regarding benchmark SLOs:
max_rss_usage. Looks ok and acceptable to @taegyunkim. We're changing the way we build and consume libdatadog dependency.PROF-12235
Checklist
Reviewer Checklist