fix(profiling): get rid of leaky string table (DataDog#11612)

Profiler samples typically contain several strings, for sample labels
(task name, endpoint/resource, etc) and frame info. Those strings tend
to be repeated. We build samples across several API calls, from Python,
Cython, C Python extensions, and C++ code, making it hard to reason
about the lifetimes of the strings we get. So, it's easier to just copy
the strings we need while building a sample. And it's preferably to only
store one copy. So, our libdatadog wrapper uses a string table, which
addresses both of these concerns. However, nothing is ever removed from
the table. And some things like task names can have new values each
time, meaning we have unbounded memory growth.

I previously tried to fix this by periodically clearing the string
table. However, this comes with its own new problems. We can construct
samples in parallel with profile uploading. So if we try to clear the
table when we upload the profile, or really any other time, we need to
make sure in-progress samples keep their strings around until the sample
is flushed to libdatadog. And we need to handle forking so we don't leak
a whole string table on fork.

It turns out we don't really need that string table at all. Libdatadog
already interns strings once the sample makes it into the libdatadog
profile. That solves the duplicate string problem. We still want to copy
strings while building the sample, but we can just do that on a
per-sample basis, and the strings only need to be alive while we build a
sample. We can use a small amount of scratch space for each sample to
copy strings. We will likely only have a few samples in flight at any
time, and we already pool the samples. So we can reuse this scratch
memory effectively. We'll only use as memory in proportion to the
number of samples being built concurrently.

So, this commit implements a simple string arena for each sample object
to use. We may want to tweak the default buffer size and how much memory
we keep around when resetting the arena. We could even deduplicate,
since we might see some duplicates per-sample from frame info.

PROF-10824

Author: nsrip-dd

ddtrace/internal/datadog/profiling/dd_wrapper/include/profile.hpp

@@ -4,12 +4,10 @@
 #include "types.hpp"
 
 #include <atomic>
-#include <deque>
 #include <memory>
 #include <mutex>
 #include <string>
 #include <string_view>
-#include <unordered_set>
 #include <vector>
 
 extern "C"
@@ -19,10 +17,6 @@ extern "C"
 
 namespace Datadog {
 
-// Unordered containers don't get heterogeneous lookup until gcc-10, so for now use this
-// strategy to dedup + store strings.
-using StringTable = std::unordered_set<std::string_view>;
-
 // Serves to collect individual samples, as well as lengthen the scope of string data
 class Profile
 {
@@ -33,11 +27,6 @@ class Profile
     std::atomic<bool> first_time{ true };
     std::mutex profile_mtx{};
 
-    // Storage for strings
-    std::deque<std::string> string_storage{};
-    StringTable strings{};
-    std::mutex string_table_mtx{};
-
     // Configuration
     SampleType type_mask{ 0 };
     unsigned int max_nframes{ g_default_max_nframes };
@@ -69,9 +58,6 @@ class Profile
     ddog_prof_Profile& profile_borrow();
     void profile_release();
 
-    // String table manipulation
-    std::string_view insert_or_get(std::string_view str);
-
     // constref getters
     const ValueIndex& val();
 

ddtrace/internal/datadog/profiling/dd_wrapper/include/sample.hpp

@@ -15,6 +15,44 @@ extern "C"
 
 namespace Datadog {
 
+namespace internal {
+
+// StringArena holds copies of strings we need while building samples.
+// StringArena is intended to amortize allocations, so that in the common
+// case we can just do a memcpy for each string we want to copy rather than
+// a new allocation for each string.
+//
+// We need to make copies right now because we don't have strong guarantees
+// that the strings we get (from Python, Cython, C++, etc) are alive the
+// whole time we build samples.
+struct StringArena
+{
+    // Default size, in bytes, of each Chunk. The value is a power of 2 (nice to
+    // allocate) that is bigger than any actual sample string size seen over a
+    // random selection of a few hundred Python profiles at Datadog. So ideally
+    // we only need one chunk, which we can reuse between samples
+    static constexpr size_t DEFAULT_SIZE = 16 * 1024;
+    // Strings are backed by fixed-size Chunks. The Chunks can't grow, or
+    // they'll move and invalidate pointers into the arena. At the same time,
+    // they must be dynamically sized at creation because we get arbitrary
+    // user-provided strings.
+    using Chunk = std::vector<char>;
+    // We keep the Chunks for this arena in a vector so we can track them, and
+    // free them when the StringArena is deallocated.
+    std::vector<Chunk> chunks;
+
+    StringArena();
+    // Clear the backing data of the arena, except for a smaller initial segment.
+    // Views returned by insert are invalid after this call.
+    void reset();
+    // Copies the contents of s into the arena and returns a view of the copy in
+    // the arena. The returned view is valid until the next call to reset, or
+    // until the arena is destroyed.
+    std::string_view insert(std::string_view s);
+};
+
+} // namespace internal
+
 class SampleManager; // friend
 
 class Sample
@@ -45,6 +83,9 @@ class Sample
     // Additional metadata
     int64_t endtime_ns = 0; // end of the event
 
+    // Backing memory for string copies
+    internal::StringArena string_storage{};
+
   public:
     // Helpers
     bool push_label(ExportLabelKey key, std::string_view val);

ddtrace/internal/datadog/profiling/dd_wrapper/src/profile.cpp

@@ -162,21 +162,6 @@ Datadog::Profile::one_time_init(SampleType type, unsigned int _max_nframes)
     first_time.store(false);
 }
 
-std::string_view
-Datadog::Profile::insert_or_get(std::string_view str)
-{
-    const std::lock_guard<std::mutex> lock(string_table_mtx); // Serialize access
-
-    auto str_it = strings.find(str);
-    if (str_it != strings.end()) {
-        return *str_it;
-    }
-
-    string_storage.emplace_back(str);
-    strings.insert(string_storage.back());
-    return string_storage.back();
-}
-
 const Datadog::ValueIndex&
 Datadog::Profile::val()
 {

ddtrace/internal/datadog/profiling/dd_wrapper/src/sample.cpp

@@ -4,8 +4,42 @@
 
 #include <algorithm>
 #include <chrono>
+#include <string_view>
 #include <thread>
 
+Datadog::internal::StringArena::StringArena()
+{
+    chunks.emplace_back();
+    chunks.back().reserve(Datadog::internal::StringArena::DEFAULT_SIZE);
+}
+
+void
+Datadog::internal::StringArena::reset()
+{
+    // Free chunks. Keep the first one around so it's easy to reuse this without
+    // needing new allocations every time. We can completely drop it to get rid
+    // of everything
+    // TODO - we could consider keeping more around if it's not too costly.
+    // The goal is to not retain more than we need _on average_. If we have
+    // mostly small samples and then a rare huge one, we can end up with
+    // all samples in our pool using as much memory as the largets ones we've seen
+    chunks.front().clear();
+    chunks.erase(++chunks.begin(), chunks.end());
+}
+
+std::string_view
+Datadog::internal::StringArena::insert(std::string_view s)
+{
+    auto chunk = &chunks.back();
+    if ((chunk->capacity() - chunk->size()) < s.size()) {
+        chunk = &chunks.emplace_back();
+        chunk->reserve(std::max(s.size(), Datadog::internal::StringArena::DEFAULT_SIZE));
+    }
+    int base = chunk->size();
+    chunk->insert(chunk->end(), s.begin(), s.end());
+    return std::string_view(chunk->data() + base, s.size());
+}
+
 Datadog::Sample::Sample(SampleType _type_mask, unsigned int _max_nframes)
   : max_nframes{ _max_nframes }
   , type_mask{ _type_mask }
@@ -28,8 +62,8 @@ void
 Datadog::Sample::push_frame_impl(std::string_view name, std::string_view filename, uint64_t address, int64_t line)
 {
     static const ddog_prof_Mapping null_mapping = { 0, 0, 0, to_slice(""), to_slice("") };
-    name = profile_state.insert_or_get(name);
-    filename = profile_state.insert_or_get(filename);
+    name = string_storage.insert(name);
+    filename = string_storage.insert(filename);
 
     CodeProvenance::get_instance().add_filename(filename);
 
@@ -73,7 +107,7 @@ Datadog::Sample::push_label(const ExportLabelKey key, std::string_view val)
     }
 
     // Otherwise, persist the val string and add the label
-    val = profile_state.insert_or_get(val);
+    val = string_storage.insert(val);
     auto& label = labels.emplace_back();
     label.key = to_slice(key_sv);
     label.str = to_slice(val);
@@ -106,6 +140,7 @@ Datadog::Sample::clear_buffers()
     labels.clear();
     locations.clear();
     dropped_frames = 0;
+    string_storage.reset();
 }
 
 bool

releasenotes/notes/profiling-fix-unbounded-string-buffer-d18056263f3e4bdc.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    profiling: fix unbounded memory usage growth caused by keeping arbitrary
+     user-generated strings (e.g. asyncio Task names) in an internal table and
+     never removing them.