From b08a977d52cf7fae9be964c57ac9d37ac343a95b Mon Sep 17 00:00:00 2001 From: Matt Wozniski Date: Thu, 28 Sep 2023 18:11:06 -0400 Subject: [PATCH 1/2] Hold interned strings by reference, not by ID Rather than having a `StringStorage` class which exposes a custom identifier used as a token for resolving an interned string, expose an `InternedString` type directly constructible from a `std::string`. This type handles the interning automatically, and using this type in signatures allows us to encode whether a particular function needs to be called with an interned string or could be called with any string. Signed-off-by: Matt Wozniski --- src/memray/_memray/native_resolver.cpp | 117 +++++++++++-------------- src/memray/_memray/native_resolver.h | 60 +++++-------- 2 files changed, 73 insertions(+), 104 deletions(-) diff --git a/src/memray/_memray/native_resolver.cpp b/src/memray/_memray/native_resolver.cpp index 308581bf11..d476ff427a 100644 --- a/src/memray/_memray/native_resolver.cpp +++ b/src/memray/_memray/native_resolver.cpp @@ -14,53 +14,46 @@ static const logLevel RESOLVE_LIB_LOG_LEVEL = DEBUG; static const logLevel RESOLVE_LIB_LOG_LEVEL = WARNING; #endif -StringStorage::StringStorage() +std::unordered_set InternedString::s_interned_data = []() { + std::unordered_set ret; + ret.reserve(4096); + return ret; +}(); + +std::mutex InternedString::s_mutex; + +InternedString::InternedString(const std::string& orig) +: d_ref(internString(orig)) { - d_interned_data.reserve(4096); - d_interned_data_storage.reserve(4096); } -size_t -StringStorage::internString(const std::string& str, const char** interned_string) +const std::string& +InternedString::get() const { - if (str.empty()) { - return 0; - } - - const size_t id = d_interned_data.size() + 1; - auto inserted = d_interned_data.insert({str, id}); - if (interned_string) { - *interned_string = inserted.first->first.c_str(); - } - - if (!inserted.second) { - return inserted.first->second; - } - // C++11 standard ยง 23.2.5/8: Rehashing the elements of an unordered associative container - // invalidates iterators, changes ordering between elements, and changes which buckets elements - // appear in, but does not invalidate pointers or references to elements. - d_interned_data_storage.push_back(&inserted.first->first); + return d_ref.get(); +} - return id; +InternedString::operator const std::string&() const +{ + return d_ref.get(); } -const std::string& -StringStorage::resolveString(size_t index) const +std::reference_wrapper +InternedString::internString(const std::string& orig) { - assert(index != 0); - return *d_interned_data_storage.at(index - 1); + std::lock_guard lock(s_mutex); + auto inserted = s_interned_data.insert(orig); + return *inserted.first; } MemorySegment::MemorySegment( - std::string filename, + InternedString filename, uintptr_t start, uintptr_t end, - backtrace_state* state, - size_t filename_index) -: d_filename(std::move(filename)) + backtrace_state* state) +: d_filename(filename) , d_start(start) , d_end(end) -, d_index(filename_index) , d_state(state) { } @@ -107,7 +100,7 @@ MemorySegment::resolveFromSymbolTable(uintptr_t address, MemorySegment::Expanded auto error_callback = [](void* _data, const char* msg, int errnum) { auto* data = reinterpret_cast(_data); LOG(ERROR) << "Error getting backtrace for address " << std::hex << data->address << std::dec - << " in segment " << data->segment->d_filename << " (errno " << errnum + << " in segment " << data->segment->d_filename.get() << " (errno " << errnum << "): " << msg; }; backtrace_syminfo(d_state, address, callback, error_callback, &data); @@ -157,14 +150,15 @@ MemorySegment::resolveIp(uintptr_t address) const bool MemorySegment::operator<(const MemorySegment& segment) const { - return std::tie(d_start, d_end, d_index) < std::tie(segment.d_start, segment.d_end, segment.d_index); + return std::tie(d_start, d_end, d_filename.get()) + < std::tie(segment.d_start, segment.d_end, segment.d_filename.get()); } bool MemorySegment::operator!=(const MemorySegment& segment) const { - return std::tie(d_start, d_end, d_index) - != std::tie(segment.d_start, segment.d_end, segment.d_index); + return std::tie(d_start, d_end, d_filename.get()) + != std::tie(segment.d_start, segment.d_end, segment.d_filename.get()); } bool @@ -185,38 +179,29 @@ MemorySegment::end() const return d_end; } -uintptr_t -MemorySegment::filenameIndex() const -{ - return d_index; -} - -const std::string& +InternedString MemorySegment::filename() const { return d_filename; } -ResolvedFrame::ResolvedFrame( - const MemorySegment::Frame& frame, - const std::shared_ptr& d_string_storage) -: d_string_storage(d_string_storage) -, d_symbol_index(d_string_storage->internString(frame.symbol)) -, d_file_index(d_string_storage->internString(frame.filename)) -, d_line(frame.lineno) +ResolvedFrame::ResolvedFrame(InternedString symbol, InternedString filename, int lineno) +: d_symbol(symbol) +, d_filename(filename) +, d_line(lineno) { } const std::string& ResolvedFrame::Symbol() const { - return d_string_storage->resolveString(d_symbol_index); + return d_symbol; } const std::string& ResolvedFrame::File() const { - return d_string_storage->resolveString(d_file_index); + return d_filename; } int @@ -224,6 +209,7 @@ ResolvedFrame::Line() const { return d_line; } + PyObject* ResolvedFrame::toPythonObject(python_helpers::PyUnicode_Cache& pystring_cache) const { @@ -255,7 +241,7 @@ ResolvedFrame::toPythonObject(python_helpers::PyUnicode_Cache& pystring_cache) c const std::string& ResolvedFrames::memoryMap() const { - return d_string_storage->resolveString(d_memory_map_index); + return d_interned_memory_map_name; } const std::vector& @@ -315,27 +301,28 @@ SymbolResolver::resolveFromSegments(uintptr_t ip, size_t generation) if (expanded_frame.empty()) { return nullptr; } - auto segment_index = segment->filenameIndex(); std::transform( expanded_frame.begin(), expanded_frame.end(), std::back_inserter(frames), - [this](const auto& frame) { - return ResolvedFrame{frame, d_string_storage}; + [](const auto& frame) { + return ResolvedFrame{ + InternedString(frame.symbol), + InternedString(frame.filename), + frame.lineno, + }; }); - return std::make_shared(segment_index, std::move(frames), d_string_storage); + return std::make_shared(segment->filename(), std::move(frames)); } void SymbolResolver::addSegment( - const std::string& filename, + InternedString filename, backtrace_state* backtrace_state, - const size_t filename_index, const uintptr_t address_start, const uintptr_t address_end) { - currentSegments() - .emplace_back(filename, address_start, address_end, backtrace_state, filename_index); + currentSegments().emplace_back(filename, address_start, address_end, backtrace_state); d_are_segments_dirty = true; } @@ -347,10 +334,8 @@ SymbolResolver::addSegments( { // We use a char* for the filename to reduce the memory footprint and // because the libbacktrace callback in findBacktraceState operates on char* - const char* interned_filename = nullptr; - auto filename_index = d_string_storage->internString(filename, &interned_filename); - - auto state = findBacktraceState(interned_filename, addr); + InternedString interned_filename(filename); + auto state = findBacktraceState(interned_filename.get().c_str(), addr); if (state == nullptr) { LOG(RESOLVE_LIB_LOG_LEVEL) << "Failed to prepare a backtrace state for " << filename; return; @@ -359,7 +344,7 @@ SymbolResolver::addSegments( for (const auto& segment : segments) { const uintptr_t segment_start = addr + segment.vaddr; const uintptr_t segment_end = addr + segment.vaddr + segment.memsz; - addSegment(filename, state, filename_index, segment_start, segment_end); + addSegment(interned_filename, state, segment_start, segment_end); } } diff --git a/src/memray/_memray/native_resolver.h b/src/memray/_memray/native_resolver.h index f77a2aef38..30879d9d42 100644 --- a/src/memray/_memray/native_resolver.h +++ b/src/memray/_memray/native_resolver.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -25,24 +26,20 @@ namespace memray::native_resolver { static constexpr int PREALLOCATED_BACKTRACE_STATES = 64; static constexpr int PREALLOCATED_IPS_CACHE_ITEMS = 32768; -class StringStorage +class InternedString { public: - // Constructors - StringStorage(); - StringStorage(StringStorage& other) = delete; - StringStorage(StringStorage&& other) = delete; - void operator=(const StringStorage&) = delete; - void operator=(StringStorage&&) = delete; - - // Methods - size_t internString(const std::string& str, const char** interned_string = nullptr); - const std::string& resolveString(size_t index) const; + explicit InternedString(const std::string& orig); + const std::string& get() const; + operator const std::string&() const; private: - // Data members - std::unordered_map d_interned_data; - std::vector d_interned_data_storage; + static std::reference_wrapper internString(const std::string& orig); + + std::reference_wrapper d_ref; + + static std::mutex s_mutex; + static std::unordered_set s_interned_data; }; class MemorySegment @@ -59,12 +56,8 @@ class MemorySegment using ExpandedFrame = std::vector; // Constructors - MemorySegment( - std::string filename, - uintptr_t start, - uintptr_t end, - backtrace_state* state, - size_t filename_index); + MemorySegment(InternedString filename, uintptr_t start, uintptr_t end, backtrace_state* state); + ExpandedFrame resolveIp(uintptr_t address) const; bool operator<(const MemorySegment& segment) const; bool operator!=(const MemorySegment& segment) const; @@ -73,8 +66,7 @@ class MemorySegment // Getters uintptr_t start() const; uintptr_t end() const; - size_t filenameIndex() const; - const std::string& filename() const; + InternedString filename() const; private: // Methods @@ -82,10 +74,9 @@ class MemorySegment void resolveFromSymbolTable(uintptr_t address, ExpandedFrame& expanded_frame) const; // Data members - std::string d_filename; + InternedString d_filename; uintptr_t d_start; uintptr_t d_end; - size_t d_index; backtrace_state* d_state; }; @@ -93,9 +84,7 @@ class ResolvedFrame { public: // Constructors - ResolvedFrame( - const MemorySegment::Frame& frame, - const std::shared_ptr& string_storage); + ResolvedFrame(InternedString symbol, InternedString filename, int lineno); // Methods PyObject* toPythonObject(python_helpers::PyUnicode_Cache& pystring_cache) const; @@ -107,9 +96,8 @@ class ResolvedFrame private: // Data members - std::shared_ptr d_string_storage; - size_t d_symbol_index; - size_t d_file_index; + InternedString d_symbol; + InternedString d_filename; int d_line; }; @@ -118,10 +106,9 @@ class ResolvedFrames public: // Constructors template - ResolvedFrames(size_t memory_map_index, T&& frames, std::shared_ptr strings_storage) - : d_memory_map_index(memory_map_index) + ResolvedFrames(InternedString interned_memory_map_name, T&& frames) + : d_interned_memory_map_name(interned_memory_map_name) , d_frames(std::forward(frames)) - , d_string_storage(std::move(strings_storage)) { } @@ -131,9 +118,8 @@ class ResolvedFrames private: // Data members - size_t d_memory_map_index{0}; + InternedString d_interned_memory_map_name; std::vector d_frames{}; - std::shared_ptr d_string_storage{nullptr}; }; class SymbolResolver @@ -170,9 +156,8 @@ class SymbolResolver // Methods void addSegment( - const std::string& filename, + InternedString filename, backtrace_state* backtrace_state, - size_t filename_index, uintptr_t address_start, uintptr_t address_end); std::vector& currentSegments(); @@ -182,7 +167,6 @@ class SymbolResolver std::unordered_map> d_segments; bool d_are_segments_dirty = false; std::unordered_map d_backtrace_states; - std::shared_ptr d_string_storage{std::make_shared()}; mutable std::unordered_map d_resolved_ips_cache; }; From 8b30350273352337f5f970bc2fbbb9808807c3f2 Mon Sep 17 00:00:00 2001 From: Matt Wozniski Date: Mon, 2 Oct 2023 12:54:12 -0400 Subject: [PATCH 2/2] 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