Remove boost thread_specific_ptr (AcademySoftwareFoundation#4221)

Remove the dependency on boost's thread_specific_ptr by using hashmap's of static duration between the owning object and the target datatype. After digging a bit into how boost's pointer works behind the scenes, it is actually a very similar mechanism, with the lookup being into an std::map. I don't have an easy way to measure performance here, but none of these are on a hot path (with the possible exception of the per_thread_info for the image cache, but this one can be managed externally if you want the best performance. --------- Signed-off-by: Chris Kulla <[email protected]>
lgritz · Apr 14, 2024 · eef733c · eef733c
1 parent c76824b
commit eef733c
Show file tree

Hide file tree

Showing 8 changed files with 130 additions and 174 deletions.
diff --git a/src/libOpenImageIO/CMakeLists.txt b/src/libOpenImageIO/CMakeLists.txt
@@ -170,7 +170,6 @@ target_link_libraries (OpenImageIO
             $<TARGET_NAME_IF_EXISTS:Freetype::Freetype>
             ${BZIP2_LIBRARIES}
             ZLIB::ZLIB
-            $<TARGET_NAME_IF_EXISTS:Boost::thread>
             ${CMAKE_DL_LIBS}
         )
 

diff --git a/src/libOpenImageIO/imageinput.cpp b/src/libOpenImageIO/imageinput.cpp
@@ -8,6 +8,8 @@
 #include <memory>
 #include <vector>
 
+#include <tsl/robin_map.h>
+
 #include <OpenImageIO/dassert.h>
 #include <OpenImageIO/deepdata.h>
 #include <OpenImageIO/filesystem.h>
@@ -19,21 +21,19 @@
 
 #include "imageio_pvt.h"
 
-#include <boost/thread/tss.hpp>
-using boost::thread_specific_ptr;
-
 
 OIIO_NAMESPACE_BEGIN
 using namespace pvt;
 
 
+// store an error message per thread, for a specific ImageInput
+static thread_local tsl::robin_map<const ImageInput*, std::string>
+    input_error_messages;
 
 class ImageInput::Impl {
 public:
     // So we can lock this ImageInput for the thread-safe methods.
     std::recursive_mutex m_mutex;
-    // Thread-specific error message for this ImageInput.
-    thread_specific_ptr<std::string> m_errormessage;
     int m_threads = 0;
 
     // The IOProxy object we will use for all I/O operations.
@@ -81,7 +81,13 @@ ImageInput::ImageInput()
 
 
 
-ImageInput::~ImageInput() {}
+ImageInput::~ImageInput()
+{
+    // Erase any leftover errors from this thread
+    // TODO: can we clear other threads' errors?
+    // TODO: potentially unsafe due to the static destruction order fiasco
+    // input_error_messages.erase(this);
+}
 
 
 
@@ -1090,18 +1096,14 @@ ImageInput::append_error(string_view message) const
 {
     if (message.size() && message.back() == '\n')
         message.remove_suffix(1);
-    std::string* errptr = m_impl->m_errormessage.get();
-    if (!errptr) {
-        errptr = new std::string;
-        m_impl->m_errormessage.reset(errptr);
-    }
+    std::string& err_str = input_error_messages[this];
     OIIO_DASSERT(
-        errptr->size() < 1024 * 1024 * 16
+        err_str.size() < 1024 * 1024 * 16
         && "Accumulated error messages > 16MB. Try checking return codes!");
-    if (errptr->size() < 1024 * 1024 * 16) {
-        if (errptr->size() && errptr->back() != '\n')
-            *errptr += '\n';
-        *errptr += std::string(message);
+    if (err_str.size() < 1024 * 1024 * 16) {
+        if (err_str.size() && err_str.back() != '\n')
+            err_str += '\n';
+        err_str.append(message.begin(), message.end());
     }
 }
 
@@ -1110,8 +1112,10 @@ ImageInput::append_error(string_view message) const
 bool
 ImageInput::has_error() const
 {
-    std::string* errptr = m_impl->m_errormessage.get();
-    return (errptr && errptr->size());
+    auto iter = input_error_messages.find(this);
+    if (iter == input_error_messages.end())
+        return false;
+    return iter.value().size() > 0;
 }
 
 
@@ -1120,11 +1124,11 @@ std::string
 ImageInput::geterror(bool clear) const
 {
     std::string e;
-    std::string* errptr = m_impl->m_errormessage.get();
-    if (errptr) {
-        e = *errptr;
+    auto iter = input_error_messages.find(this);
+    if (iter != input_error_messages.end()) {
+        e = iter.value();
         if (clear)
-            errptr->clear();
+            input_error_messages.erase(iter);
     }
     return e;
 }

diff --git a/src/libOpenImageIO/imageoutput.cpp b/src/libOpenImageIO/imageoutput.cpp
@@ -9,6 +9,8 @@
 #include <memory>
 #include <vector>
 
+#include <tsl/robin_map.h>
+
 #include <OpenImageIO/dassert.h>
 #include <OpenImageIO/deepdata.h>
 #include <OpenImageIO/filesystem.h>
@@ -21,23 +23,23 @@
 
 #include "imageio_pvt.h"
 
-#include <boost/thread/tss.hpp>
-using boost::thread_specific_ptr;
 
 
 OIIO_NAMESPACE_BEGIN
 using namespace pvt;
 
 
+// store an error message per thread, for a specific ImageInput
+static thread_local tsl::robin_map<const ImageOutput*, std::string>
+    output_error_messages;
+
 
 class ImageOutput::Impl {
 public:
     // Unneeded?
     //  // So we can lock this ImageOutput for the thread-safe methods.
     //  std::recursive_mutex m_mutex;
 
-    // Thread-specific error message for this ImageOutput.
-    thread_specific_ptr<std::string> m_errormessage;
     int m_threads = 0;
 
     // The IOProxy object we will use for all I/O operations.
@@ -82,7 +84,13 @@ ImageOutput::ImageOutput()
 
 
 
-ImageOutput::~ImageOutput() {}
+ImageOutput::~ImageOutput()
+{
+    // Erase any leftover errors from this thread
+    // TODO: can we clear other threads' errors?
+    // TODO: potentially unsafe due to the static destruction order fiasco
+    // output_error_messages.erase(this);
+}
 
 
 
@@ -261,17 +269,13 @@ ImageOutput::append_error(string_view message) const
 {
     if (message.size() && message.back() == '\n')
         message.remove_suffix(1);
-    std::string* errptr = m_impl->m_errormessage.get();
-    if (!errptr) {
-        errptr = new std::string;
-        m_impl->m_errormessage.reset(errptr);
-    }
+    std::string& err_str = output_error_messages[this];
     OIIO_DASSERT(
-        errptr->size() < 1024 * 1024 * 16
+        err_str.size() < 1024 * 1024 * 16
         && "Accumulated error messages > 16MB. Try checking return codes!");
-    if (errptr->size() && errptr->back() != '\n')
-        *errptr += '\n';
-    *errptr += std::string(message);
+    if (err_str.size() && err_str.back() != '\n')
+        err_str += '\n';
+    err_str.append(message.begin(), message.end());
 }
 
 
@@ -694,8 +698,10 @@ ImageOutput::copy_tile_to_image_buffer(int x, int y, int z, TypeDesc format,
 bool
 ImageOutput::has_error() const
 {
-    std::string* errptr = m_impl->m_errormessage.get();
-    return (errptr && errptr->size());
+    auto iter = output_error_messages.find(this);
+    if (iter == output_error_messages.end())
+        return false;
+    return iter.value().size() > 0;
 }
 
 
@@ -704,11 +710,11 @@ std::string
 ImageOutput::geterror(bool clear) const
 {
     std::string e;
-    std::string* errptr = m_impl->m_errormessage.get();
-    if (errptr) {
-        e = *errptr;
+    auto iter = output_error_messages.find(this);
+    if (iter != output_error_messages.end()) {
+        e = iter.value();
         if (clear)
-            errptr->clear();
+            output_error_messages.erase(iter);
     }
     return e;
 }