Skip to content

Commit

Permalink
fix(profiling): remove usage of ensure_binary_or_empty from ddup [b…
Browse files Browse the repository at this point in the history
…ackport 2.14] (#11283)

Backport 6b73e73 from #11206 to 2.14.

`ensure_binary_or_empty()` calls can incur significant memory
allocations on CPython versions before 3.12 for stack v1. See below
images, and all services shown here are with stack v1 with libdatadog
exporter (`DD_PROFILING_EXPORT_LIBDD_ENABLED` or
`DD_PROFILING_TIMELINE_ENABLED`).

Python 3.11.x 237MiB/713MiB (33% of the profile) 
<img width="1509" alt="image"
src="https://github.com/user-attachments/assets/66ca57d8-11b0-4a63-9598-bb0d7537f1cf">

Python 3.9.x 209MiB/595MiB (35%)
<img width="1504" alt="image"
src="https://github.com/user-attachments/assets/f56eac9a-9bc0-4a84-97cc-37bfefc1bc9c">

Older CPython versions seem to have a less efficient implementation of
`str.encode()` which is used by `ensure_binary_or_empty()`.
`str.encode()` is implemented in C so we don't show any frame below
`ensure_binary()` in above images as Python profiler can't profile
native code.

This function is used across all profilers (stack, memory, and lock).
Though enabling stack v2 implementation via
`DD_PROFILING_STACK_V2_ENABLED` could remove most of it, since the
function is still going to be used for memory and lock profilers, we fix
it here.

We use CPython `PyUnicode_AsUTF8AndSize` to retrieve raw pointer for the
given Python `str` object, then create `std::string_view` to pass that
over to the `Sample`. We don't propagate any information if the string
is not in UTF-8. This behavior is ok for now as libdatadog exporter only
accepts utf-8, using `std::str::from_utf8`.

See below image for comparison of memory allocations before and after
this change with Python 3.11.x. The relative and absolute amount is
different from above examples, but still show a sizable reduction.

Before: 252MiB/1.26GiB (20%)
<img width="1497" alt="image"
src="https://github.com/user-attachments/assets/c35aaed6-6a64-4ba1-93b0-47c6b0553ec0">

After: 0MiB/1.02GiB, memory allocation from `ensure_binary_or_empty()`
is just completely gone
<img width="1510" alt="image"
src="https://github.com/user-attachments/assets/1ecebb73-29fd-440e-9a2a-3bbe7ae70a8a">


## 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)

Co-authored-by: Taegyun Kim <[email protected]>
  • Loading branch information
github-actions[bot] and taegyunkim authored Nov 5, 2024
1 parent 5e2c496 commit 9dd3b63
Show file tree
Hide file tree
Showing 5 changed files with 677 additions and 230 deletions.
Loading

0 comments on commit 9dd3b63

Please sign in to comment.