From fce0898b30a561c690d438bc400708be218da8df Mon Sep 17 00:00:00 2001 From: "T. Kowalski" Date: Wed, 22 Oct 2025 00:56:51 +0200 Subject: [PATCH] fix(profiling): adapt Stack V2 to exception-free Echion (#14933) This PR updates Echion to use the latest version. Included Echion PRs - [refactor: stop using exceptions for reporting errors](https://github.com/P403n1x87/echion/pull/156) - [misc: remove Error classes](https://github.com/P403n1x87/echion/pull/164) - [bug: avoid possibly unbounded memory allocations](https://github.com/P403n1x87/echion/pull/159) - [fix: remove extra loop in Frame::read() for 3.13+](https://github.com/P403n1x87/echion/pull/167) - [refactor: return std::reference_wrapper over raw pointer](https://github.com/P403n1x87/echion/pull/168) See comparison here https://github.com/P403n1x87/echion/compare/3ebeb3e975239f252fa0d6bb739344f35eaf1657...39d74a33a3f3abe810e6a29132721871e3127472 --------- Co-authored-by: Taegyun Kim (cherry picked from commit 2c72bca069062bb83478c4372e84e70b0b5af1d0) --- .../datadog/profiling/stack_v2/CMakeLists.txt | 2 +- .../stack_v2/include/stack_renderer.hpp | 2 +- .../datadog/profiling/stack_v2/src/sampler.cpp | 17 ++++++++++------- .../profiling/stack_v2/src/stack_renderer.cpp | 15 +++++++++------ .../datadog/profiling/stack_v2/src/stack_v2.cpp | 8 +++----- ...iling-echion-fix-alloc-f1204a794b1d3a1d.yaml | 7 +++++++ 6 files changed, 31 insertions(+), 20 deletions(-) create mode 100644 releasenotes/notes/profiling-echion-fix-alloc-f1204a794b1d3a1d.yaml diff --git a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt index 7760cfe5c3a..3a2efc7cd9d 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt +++ b/ddtrace/internal/datadog/profiling/stack_v2/CMakeLists.txt @@ -41,7 +41,7 @@ endif() # Add echion set(ECHION_COMMIT - "576ff5353fc4ed91c283a383b1eb1b32110b42cc" # https://github.com/P403n1x87/echion/commit/576ff5353fc4ed91c283a383b1eb1b32110b42cc + "39d74a33a3f3abe810e6a29132721871e3127472" # https://github.com/P403n1x87/echion/commit/39d74a33a3f3abe810e6a29132721871e3127472 CACHE STRING "Commit hash of echion to use") FetchContent_Declare( echion diff --git a/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp index 6f1e4ec0417..ce9faaf2170 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/include/stack_renderer.hpp @@ -40,7 +40,7 @@ class StackRenderer : public RendererInterface // the sample is created, this has to be reset. bool pushed_task_name = false; - void open() override {} + Result open() override { return Result::ok(); } void close() override {} void header() override {} void metadata(const std::string&, const std::string&) override {} diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp index 147e4d3b0b9..5c346f9f394 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/sampler.cpp @@ -2,6 +2,7 @@ #include "thread_span_links.hpp" +#include "echion/errors.h" #include "echion/greenlets.h" #include "echion/interp.h" #include "echion/tasks.h" @@ -153,7 +154,7 @@ Sampler::sampling_thread(const uint64_t seq_num) // Perform the sample for_each_interp([&](InterpreterInfo& interp) -> void { for_each_thread(interp, [&](PyThreadState* tstate, ThreadInfo& thread) { - thread.sample(interp.id, tstate, wall_time_us); + (void)thread.sample(interp.id, tstate, wall_time_us); }); }); @@ -245,9 +246,10 @@ Sampler::register_thread(uint64_t id, uint64_t native_id, const char* name) static bool has_errored = false; auto it = thread_info_map.find(id); if (it == thread_info_map.end()) { - try { - thread_info_map.emplace(id, std::make_unique(id, native_id, name)); - } catch (const ThreadInfo::Error& e) { + auto maybe_thread_info = ThreadInfo::create(id, native_id, name); + if (maybe_thread_info) { + thread_info_map.emplace(id, std::move(*maybe_thread_info)); + } else { if (!has_errored) { has_errored = true; std::cerr << "Failed to register thread: " << std::hex << id << std::dec << " (" << native_id << ") " @@ -255,9 +257,10 @@ Sampler::register_thread(uint64_t id, uint64_t native_id, const char* name) } } } else { - try { - it->second = std::make_unique(id, native_id, name); - } catch (const ThreadInfo::Error& e) { + auto maybe_thread_info = ThreadInfo::create(id, native_id, name); + if (maybe_thread_info) { + it->second = std::move(*maybe_thread_info); + } else { if (!has_errored) { has_errored = true; std::cerr << "Failed to register thread: " << std::hex << id << std::dec << " (" << native_id << ") " diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp index bc3bcdbd455..beb643a7fc0 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_renderer.cpp @@ -131,15 +131,18 @@ StackRenderer::render_frame(Frame& frame) static constexpr std::string_view missing_name = ""; std::string_view filename_str; std::string_view name_str; - try { - filename_str = string_table.lookup(frame.filename); - } catch (StringTable::Error&) { + + auto maybe_filename_str = string_table.lookup(frame.filename); + if (maybe_filename_str) { + filename_str = maybe_filename_str->get(); + } else { filename_str = missing_filename; } - try { - name_str = string_table.lookup(frame.name); - } catch (StringTable::Error&) { + auto maybe_name_str = string_table.lookup(frame.name); + if (maybe_name_str) { + name_str = maybe_name_str->get(); + } else { name_str = missing_name; } diff --git a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp index 5548be35c2c..2c78490fdcd 100644 --- a/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp +++ b/ddtrace/internal/datadog/profiling/stack_v2/src/stack_v2.cpp @@ -201,16 +201,14 @@ track_greenlet(PyObject* Py_UNUSED(m), PyObject* args) if (!PyArg_ParseTuple(args, "lOO", &greenlet_id, &name, &frame)) return NULL; - StringTable::Key greenlet_name; - - try { - greenlet_name = string_table.key(name); - } catch (StringTable::Error&) { + auto maybe_greenlet_name = string_table.key(name); + if (!maybe_greenlet_name) { // We failed to get this task but we keep going PyErr_SetString(PyExc_RuntimeError, "Failed to get greenlet name from the string table"); return NULL; } + auto greenlet_name = *maybe_greenlet_name; Sampler::get().track_greenlet(greenlet_id, greenlet_name, frame); Py_RETURN_NONE; diff --git a/releasenotes/notes/profiling-echion-fix-alloc-f1204a794b1d3a1d.yaml b/releasenotes/notes/profiling-echion-fix-alloc-f1204a794b1d3a1d.yaml new file mode 100644 index 00000000000..8ca655a780c --- /dev/null +++ b/releasenotes/notes/profiling-echion-fix-alloc-f1204a794b1d3a1d.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + profiling: Upgrades echion to resolve an issue where stack profiler can + allocate a large amount of memory unnecessarily. Resolves another issue + where the profiler can loop infinitely on Python 3.13. +