Remove boost thread_specific_ptr #4221

fpsunflower · 2024-04-10T21:03:21Z

Description

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.

Tests

Going to rely on the CI to run the testsuite here.

Checklist:

I have read the contribution guidelines.
I have updated the documentation, if applicable.
I have ensured that the change is tested somewhere in the testsuite
(adding new test cases if necessary).
If I added or modified a C++ API call, I have also amended the
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
My code follows the prevailing code style of this project. If I haven't
already run clang-format before submitting, I definitely will look at the CI
test that runs clang-format and fix anything that it highlights as being
nonconforming.

… reduce the dependency on the boost thread library Signed-off-by: Chris Kulla <ckulla@gmail.com>

…d inside ImageCacheImpl with a simpler idiom using a thread_local map Signed-off-by: Chris Kulla <ckulla@gmail.com>

… are no longer used Signed-off-by: Chris Kulla <ckulla@gmail.com>

fpsunflower · 2024-04-10T21:05:16Z

I split this up into three commits to help with the review.

There is a bit of a pattern with the error messages that could maybe be unified - but I opted to stay as close to the original code as possible for now.

I only tested that this works properly on my local machine - so will be monitoring the CI to ensure its working properly everywhere else.

lgritz

I really like this!

I made a comment about ImageOutput, but I think it applies across the board. Are you not missing a provision in the object destructors to remove that object's doodad from the per-thread map?

lgritz · 2024-04-10T21:13:45Z

src/libOpenImageIO/imageoutput.cpp

-        errptr = new std::string;
-        m_impl->m_errormessage.reset(errptr);
-    }
+    std::string& err_str = error_messages[this];


So there is a per-thread map of ImageOutput -> string. This line adds a slot in this thread's map for this ImageOutput. When the thread terminates, I assume the whole map (for that thread) will delete. But as long as the thread is alive, the strings just sit there even when the ImageOutput they correspond to is long gone, no? Don't we want something in ImageOutput::Impl::~Impl that does an erase for its slot in the map?

The problem is the destructor only runs on one thread. So it can't reach into other thread's storage without additional bookkeeping.

It probably is solveable, though I worry about introducing complicated threaded bookkeeping just to release the memory a few error messages.

Oooooh, yes of course.

OK, so for the error things, it might pseudo-leak a few strings -- and only if there are actual errors. So in practice, NBD.

And for ImageCache, the "object" is the cache itself, of which there is generally exactly one, so we don't really worry about a succession of IC's being created from scratch, then being destroyed without freeing their per-thread cruft.

Sounds ok to me, then.

Chris, I still think that maybe the object destructors should have a

if (threadlocal.contains(key)) threadlocal.erase(key);

It's true that it will only free the thing if the same thread that's destroying the ImageOutput (say) is the same one that called error(). But that's a very common case! So we could still leak a few strings, but usually it will cause us to fully clean up.

I can definitely add this to all the destructors. It shouldn't cost much in the common case and keeps the map for the main thread small(er).

I also just realized that technically, these error strings have the same bug I fixed in the image cache. It's much less likely to cause issues, but the bug would be:

create an ImageInput - gets allocated at address 0x1234 (for example)

do some operations (potentially across threads) which lead to errors, but you don't check or clear them

close the ImageInput

create a new ImageInput which happens to get allocated at 0x1234 again

now all threads still have the error message from the first one which never got cleared :(

The way I solved it for the per thread infos was to make the map use a strictly incrementing counter instead of their address for the lookup (in practice most apps only ever create a single image cache so its not a big diference). I think I should do the same for the error messages (even though its asymptomatic in the testsuite, the potentialy for confusion if it ever happens would be quite high). The only drawback is an extra 64 bit ID per ImageInput,ImageOutput.

I'll ponder this a bit more to see if I think of any other clever way to solve this ....

haha, I think our messages crossed, I had typed mine a couple hours ago, but only hit the button now without seeing you commented on the same thing.

In short, I don't think it's worth doing anything elaborate. Let's consider it a user error to destroy an ImageInput that you used, accumulated errors, but never retrieved the error messages. But if you make the destructor clear the error for that thread (which will usually be the only thread), I think the stars will rarely align to give a problem, even when the user makes that mistake.