Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove boost thread_specific_ptr #4221

Merged
merged 16 commits into from
Apr 14, 2024
Merged

Remove boost thread_specific_ptr #4221

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
534dfa7
Replace m_errormessage fields with a thread_local hash map instead to…
fpsunflower Apr 10, 2024
17a1d2f
Replace use of boost::thread_specific_ptr for the per thread info use…
fpsunflower Apr 10, 2024
5aa4023
Remove boost::thread and boost:filesystem from CMakeLists.txt as they…
fpsunflower Apr 10, 2024
661b178
Fix unity build errors by making the static variable names unique
fpsunflower Apr 10, 2024
6815efb
Add shrink_to_fit when clearing error messages to minimize wasted memory
fpsunflower Apr 10, 2024
cb86c35
Fix crashes in unit test that repeatedly created ImageCache objects. …
fpsunflower Apr 11, 2024
6554173
Simplify the perthread_info memory management by making the imagecach…
fpsunflower Apr 11, 2024
d66fb64
Run clang-format
fpsunflower Apr 11, 2024
bab0860
Erase error message entries instead of just clearing the string
fpsunflower Apr 12, 2024
d7e1afe
Erase any dangling error messages when objects that report errors are…
fpsunflower Apr 12, 2024
5bc1265
Run clang-format
fpsunflower Apr 12, 2024
ab60908
Tweak erase call
fpsunflower Apr 12, 2024
fd2db6e
Revert "Erase any dangling error messages when objects that report er…
fpsunflower Apr 12, 2024
ac5752c
Add calls in ImageInput/Output to clear error string, but leave the I…
fpsunflower Apr 14, 2024
da93004
Revert "Add calls in ImageInput/Output to clear error string, but lea…
fpsunflower Apr 14, 2024
00b8168
Leave the erase calls in the destructors, but commented out until we …
fpsunflower Apr 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/libOpenImageIO/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@ target_link_libraries (OpenImageIO
$<TARGET_NAME_IF_EXISTS:Freetype::Freetype>
${BZIP2_LIBRARIES}
ZLIB::ZLIB
$<TARGET_NAME_IF_EXISTS:Boost::thread>
${CMAKE_DL_LIBS}
)

Expand Down
44 changes: 22 additions & 22 deletions src/libOpenImageIO/imageinput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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.
Expand Down Expand Up @@ -1090,18 +1090,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());
}
}

Expand All @@ -1110,8 +1106,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;
}


Expand All @@ -1120,11 +1118,13 @@ std::string
ImageInput::geterror(bool clear) const
{
std::string e;
std::string* errptr = m_impl->m_errormessage.get();
if (errptr) {
e = *errptr;
if (clear)
errptr->clear();
auto iter = input_error_messages.find(this);
if (iter != input_error_messages.end()) {
e = iter.value();
if (clear) {
iter.value().clear();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was input_error_messages.erase(iter), instead of clear(), then any time an error was retrieved/clear by the right thread, the entry in the map would fully be cleaned up, instead of "leaking", even though the thing it leaks is usually an empty string.

iter.value().shrink_to_fit();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shrink_to_fit instead of erase? You're freeing the characters, but not getting rid of the node in the hash table that contains the string itself. You could imagine a pattern of repeated ImageInput constructions (with errors) that would lead to a fuller and fuller hash table containing unusabl empty strings? Versus actually removing the hash table entry when the ImageInput is destroyed by the right thread (which is mostly).

I don't think it's a problem in practice -- to be a real issue, you'd need tons of created and then destroyed ImageInput objects that encountered errors along the way. I'm happy to make that a "cross that very unlikely bridge when we get to it." But I'm curious if you did this because you spotted some big flaw in my proposal that I hadn't considered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see what you mean now. Yes of course erase makes more sense.

}
}
return e;
}
Expand Down
42 changes: 22 additions & 20 deletions src/libOpenImageIO/imageoutput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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.
Expand Down Expand Up @@ -261,17 +263,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());
}


Expand Down Expand Up @@ -677,8 +675,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;
}


Expand All @@ -687,11 +687,13 @@ std::string
ImageOutput::geterror(bool clear) const
{
std::string e;
std::string* errptr = m_impl->m_errormessage.get();
if (errptr) {
e = *errptr;
if (clear)
errptr->clear();
auto iter = output_error_messages.find(this);
if (iter != output_error_messages.end()) {
e = iter.value();
if (clear) {
iter.value().clear();
iter.value().shrink_to_fit();
}
}
return e;
}
Expand Down
Loading
Loading