From 6eb8e193785e5cb9528ea8b63e7de8798c992dfd Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Fri, 22 May 2026 20:16:39 -0700 Subject: [PATCH] feat: allow freeing of ustring table for debugging cases This patch optionally frees the contents of the ustring table as the process is exiting. It's triggered one of two ways: - `OIIO::attribute("ustring:cleanup", 1);` - Environment variable `OIIO_USTRING_CLEANUP=1` If neither is used, the teardown won't happen, thus avoiding any pointless work when it should be exiting the process. The purpose of this is that when using certain debugging tools, the ustring table (which only grows and never frees) looks like a memory leak, even though it's by design. Someimes, it's useful to make it not appear ot leak, and you are happy for a large table to spend time pointlessly freeing all the data. Fixes 3913 Signed-off-by: Larry Gritz --- src/include/OpenImageIO/imageio.h | 10 ++++++++ src/include/imageio_pvt.h | 6 ++++- src/libOpenImageIO/imageio.cpp | 20 ++++++++++----- src/libutil/ustring.cpp | 42 +++++++++++++++++++++++++++++-- 4 files changed, 69 insertions(+), 9 deletions(-) diff --git a/src/include/OpenImageIO/imageio.h b/src/include/OpenImageIO/imageio.h index 82fa598969..a7decea696 100644 --- a/src/include/OpenImageIO/imageio.h +++ b/src/include/OpenImageIO/imageio.h @@ -3968,6 +3968,16 @@ OIIO_API std::string geterror(bool clear = true); /// enable globally in an environment where security is a higher priority /// than being tolerant of partially broken image files. /// +/// - `ustring:cleanup` (int: 0) +/// +/// If nonzero, upon exit, do a thorough (and possibly expensive) teardown +/// of ustring internal resources to ensure that there are no apparent +/// memory leaks. This is only desirable in certain debugging situations. +/// Ordinarily, it is better to finish as quickly as possible, so the +/// default of 0 skips a time consuming and pointless teardown of the +/// ustring internal allocations when the app exits. Note that this can +/// also be enabled with the `OIIO_USTRING_CLEANUP` environment variable. +/// /// EXAMPLES: /// ``` /// // Setting single simple values simply: diff --git a/src/include/imageio_pvt.h b/src/include/imageio_pvt.h index c490a0a42e..eb1aebb798 100644 --- a/src/include/imageio_pvt.h +++ b/src/include/imageio_pvt.h @@ -53,7 +53,11 @@ extern atomic_ll IB_local_mem_current; extern atomic_ll IB_local_mem_peak; extern std::atomic IB_total_open_time; extern std::atomic IB_total_image_read_time; -extern OIIO_UTIL_API int oiio_use_tbb; // This lives in libOpenImageIO_Util + +// These live in libOpenImageIO_Util +extern OIIO_UTIL_API int oiio_use_tbb; +extern OIIO_UTIL_API int oiio_ustring_cleanup; + OIIO_API const std::vector& font_dirs(); OIIO_API const std::vector& diff --git a/src/libOpenImageIO/imageio.cpp b/src/libOpenImageIO/imageio.cpp index 95a430cfe9..265aee896a 100644 --- a/src/libOpenImageIO/imageio.cpp +++ b/src/libOpenImageIO/imageio.cpp @@ -373,6 +373,10 @@ attribute(string_view name, TypeDesc type, const void* val) // Things below here need to buarded by the attrib_mutex std::lock_guard lock(attrib_mutex); + if (name == "debug" && type == TypeInt) { + oiio_print_debug = *(const int*)val; + return true; + } if (name == "read_chunk" && type == TypeInt) { oiio_read_chunk = *(const int*)val; return true; @@ -447,10 +451,6 @@ attribute(string_view name, TypeDesc type, const void* val) oiio_use_tbb = *(const int*)val; return true; } - if (name == "debug" && type == TypeInt) { - oiio_print_debug = *(const int*)val; - return true; - } if (name == "log_times" && type == TypeInt) { oiio_log_times = *(const int*)val; return true; @@ -471,6 +471,10 @@ attribute(string_view name, TypeDesc type, const void* val) oiio_try_all_readers = *(const int*)val; return true; } + if (name == "ustring:cleanup" && type == TypeInt) { + oiio_ustring_cleanup = *(const int*)val; + return true; + } return false; } @@ -511,6 +515,10 @@ getattribute(string_view name, TypeDesc type, void* val) // Things below here need to buarded by the attrib_mutex std::lock_guard lock(attrib_mutex); + if (name == "debug" && type == TypeInt) { + *(int*)val = oiio_print_debug; + return true; + } if (name == "read_chunk" && type == TypeInt) { *(int*)val = oiio_read_chunk; return true; @@ -649,8 +657,8 @@ getattribute(string_view name, TypeDesc type, void* val) *(int*)val = oiio_use_tbb; return true; } - if (name == "debug" && type == TypeInt) { - *(int*)val = oiio_print_debug; + if (name == "ustring:cleanup" && type == TypeInt) { + *(int*)val = oiio_ustring_cleanup; return true; } if (name == "log_times" && type == TypeInt) { diff --git a/src/libutil/ustring.cpp b/src/libutil/ustring.cpp index 043ce43dfb..ec255bf1dc 100644 --- a/src/libutil/ustring.cpp +++ b/src/libutil/ustring.cpp @@ -8,10 +8,29 @@ #include #include #include +#include #include #include #include + + +OIIO_NAMESPACE_BEGIN +namespace pvt { + +// If nonzero, the ustring table will be freed at process exit. This is off by +// default because cleanup is unnecessary (the OS reclaims the memory) and can +// add measurable time at exit for large tables. Enable it when using valgrind +// or other leak detectors to suppress false positives. Settable via +// OIIO::attribute("ustring:cleanup",1) or the OIIO_USTRING_CLEANUP +// environment variable. +OIIO_UTIL_API int oiio_ustring_cleanup = Strutil::stoi( + Sysutil::getenv("OIIO_USTRING_CLEANUP")); + +} // namespace pvt +OIIO_NAMESPACE_END + + OIIO_NAMESPACE_3_1_BEGIN // Use rw spin locks @@ -43,10 +62,25 @@ template struct TableRepMap { , memory_usage(sizeof(*this) + POOL_SIZE + sizeof(ustring::TableRep*) * BASE_CAPACITY) { + pool_allocs.push_back(pool); } ~TableRepMap() - { /* just let memory leak */ + { + if (OIIO::pvt::oiio_ustring_cleanup) { + // If requested, take the time to properly destroy all the entries + // and everything we malloced. + ustring_write_lock_t lock(mutex); + for (size_t i = 0; i <= mask; ++i) { + if (entries[i]) + entries[i]->~TableRep(); + } + free(entries); + for (auto p : pool_allocs) + free(p); + } else { + /* just let memory leak */ + } } size_t get_memory_usage() @@ -187,13 +221,16 @@ template struct TableRepMap { if (len >= POOL_SIZE) { memory_usage += len; - return (char*)malloc(len); // no need to try and use the pool + char* r = (char*)malloc(len); // no need to try and use the pool + pool_allocs.push_back(r); + return r; } if (pool_offset + len > POOL_SIZE) { // NOTE: old pool will leak - this is ok because ustrings cannot be freed memory_usage += POOL_SIZE; pool = (char*)malloc(POOL_SIZE); pool_offset = 0; + pool_allocs.push_back(pool); } char* result = pool + pool_offset; pool_offset += len; @@ -207,6 +244,7 @@ template struct TableRepMap { char* pool; size_t pool_offset = 0; size_t memory_usage; + std::vector pool_allocs; #ifdef USTRING_TRACK_NUM_LOOKUPS size_t num_lookups = 0; #endif