From 8b30350273352337f5f970bc2fbbb9808807c3f2 Mon Sep 17 00:00:00 2001 From: Matt Wozniski Date: Mon, 2 Oct 2023 12:54:12 -0400 Subject: [PATCH] Avoid creating more `backtrace_state*` than needed `libbacktrace` gives us no way to free a `backtrace_state*` once it's been created, so it's in our best interest to create as few of them as possible. Rather than having one cache of `backtrace_state*` per opened capture file, have a global cache of `backtrace_state*` (protected by a global mutex). Signed-off-by: Matt Wozniski --- news/473.bugfix.rst | 1 + src/memray/_memray/native_resolver.cpp | 34 +++++++++++++++++--------- src/memray/_memray/native_resolver.h | 16 ++++++++---- 3 files changed, 34 insertions(+), 17 deletions(-) create mode 100644 news/473.bugfix.rst diff --git a/news/473.bugfix.rst b/news/473.bugfix.rst new file mode 100644 index 0000000000..4b24c82884 --- /dev/null +++ b/news/473.bugfix.rst @@ -0,0 +1 @@ +Fix a memory leak in Memray itself when many different capture files are opened by a single Memray process and native stacks are being reported. This issue primarily affected ``pytest-memray``. diff --git a/src/memray/_memray/native_resolver.cpp b/src/memray/_memray/native_resolver.cpp index d476ff427a..f07a715762 100644 --- a/src/memray/_memray/native_resolver.cpp +++ b/src/memray/_memray/native_resolver.cpp @@ -22,6 +22,14 @@ std::unordered_set InternedString::s_interned_data = []() { std::mutex InternedString::s_mutex; +SymbolResolver::BacktraceStateCache SymbolResolver::s_backtrace_states = []() { + SymbolResolver::BacktraceStateCache ret; + ret.reserve(PREALLOCATED_BACKTRACE_STATES); + return ret; +}(); + +std::mutex SymbolResolver::s_backtrace_states_mutex; + InternedString::InternedString(const std::string& orig) : d_ref(internString(orig)) { @@ -252,7 +260,6 @@ ResolvedFrames::frames() const SymbolResolver::SymbolResolver() { - d_backtrace_states.reserve(PREALLOCATED_BACKTRACE_STATES); d_resolved_ips_cache.reserve(PREALLOCATED_IPS_CACHE_ITEMS); } @@ -332,10 +339,8 @@ SymbolResolver::addSegments( uintptr_t addr, const std::vector& segments) { - // We use a char* for the filename to reduce the memory footprint and - // because the libbacktrace callback in findBacktraceState operates on char* InternedString interned_filename(filename); - auto state = findBacktraceState(interned_filename.get().c_str(), addr); + auto state = getBacktraceState(interned_filename, addr); if (state == nullptr) { LOG(RESOLVE_LIB_LOG_LEVEL) << "Failed to prepare a backtrace state for " << filename; return; @@ -363,13 +368,17 @@ SymbolResolver::clearSegments() } backtrace_state* -SymbolResolver::findBacktraceState(const char* filename, uintptr_t address_start) +SymbolResolver::getBacktraceState(InternedString interned_filename, uintptr_t address_start) { - // We hash into "d_backtrace_states" using a char* as it's safe on the condition that every - // const char* used as a key in the map is one that was returned by "d_string_storage", - // and it's safe because no pointer that's returned by "d_string_storage" is ever invalidated. - auto it = d_backtrace_states.find(filename); - if (it != d_backtrace_states.end()) { + // We hash into "s_backtrace_states" using a `const char*`. This is safe + // because every `const char*` we save is owned by an interned string. + const char* filename = interned_filename.get().c_str(); + auto key = std::make_pair(filename, address_start); + + std::lock_guard lock(s_backtrace_states_mutex); + + auto it = s_backtrace_states.find(key); + if (it != s_backtrace_states.end()) { return it->second; } @@ -385,7 +394,7 @@ SymbolResolver::findBacktraceState(const char* filename, uintptr_t address_start << "(errno " << errnum << "): " << msg; }; - auto state = backtrace_create_state(data.fileName, false, errorHandler, &data); + auto state = backtrace_create_state(data.fileName, true, errorHandler, &data); if (!state) { return nullptr; @@ -432,7 +441,8 @@ SymbolResolver::findBacktraceState(const char* filename, uintptr_t address_start return nullptr; #endif } - d_backtrace_states.insert(it, {filename, state}); + + s_backtrace_states.insert(it, {key, state}); return state; } diff --git a/src/memray/_memray/native_resolver.h b/src/memray/_memray/native_resolver.h index 30879d9d42..b0462496af 100644 --- a/src/memray/_memray/native_resolver.h +++ b/src/memray/_memray/native_resolver.h @@ -137,7 +137,8 @@ class SymbolResolver uintptr_t addr, const std::vector& segments); void clearSegments(); - backtrace_state* findBacktraceState(const char* filename, uintptr_t address_start); + + static backtrace_state* getBacktraceState(InternedString filename, uintptr_t address_start); // Getters size_t currentSegmentGeneration() const; @@ -145,7 +146,8 @@ class SymbolResolver private: // Aliases and helpers using ips_cache_pair_t = std::pair; - struct ips_cache_pair_hash + + struct pair_hash { template std::size_t operator()(const std::pair& pair) const @@ -154,6 +156,9 @@ class SymbolResolver } }; + using BacktraceStateCache = + std::unordered_map, backtrace_state*, pair_hash>; + // Methods void addSegment( InternedString filename, @@ -166,9 +171,10 @@ class SymbolResolver // Data members std::unordered_map> d_segments; bool d_are_segments_dirty = false; - std::unordered_map d_backtrace_states; - mutable std::unordered_map - d_resolved_ips_cache; + mutable std::unordered_map d_resolved_ips_cache; + + static std::mutex s_backtrace_states_mutex; + static BacktraceStateCache s_backtrace_states; }; std::vector