Replies: 7 comments 121 replies
-
|
Hello @GitFred4, You need to be careful when using the inplace operations in a threaded program, since they modify the image. The simplest solution is usually to call The best solution is probably to NOT use the inplace operations at all! They are only there for interactive "paintbox"-style programs, they are not supposed to be used for general image processing. Sometimes they are necessary for performance, but there's usually another solution. What's your use-case? |
Beta Was this translation helpful? Give feedback.
-
|
https://www.libvips.org/API/current/cpp/classVImage.html#a852b430469f8b63648cd8c006314cd72 |
Beta Was this translation helpful? Give feedback.
-
|
I will try that. The images are not shared between threads. I will see if I can replace the data() function, but I don't think it will work in all cases. |
Beta Was this translation helpful? Give feedback.
-
|
Ah OK. Yes, libvips will automatically share images between threads behind your back, so you can get races even if you think your code keeps them separate. |
Beta Was this translation helpful? Give feedback.
-
|
Now it's getting weird. And now the CPU does nothing and the thread is stuck in the vips::VImage::copy_memory() function Same spot |
Beta Was this translation helpful? Give feedback.
-
|
Did you call |
Beta Was this translation helpful? Give feedback.
-
Hmm, that is weird! The only reason I mentioned that was because I saw those calls in my I just opened PR #4122, which might solve the issue. Since I couldn't reproduce the issue locally, my plan is to build test binaries with that PR backported. If that doesn't work, I'll try to build test binaries from the 8.15 branch. |
Beta Was this translation helpful? Give feedback.
-
|
The 64-bit libvips 8.15.3 Windows binaries and corresponding PDBs built from commit libvips/build-win64-mxe@6074dec can be found here: Please remove that |
Beta Was this translation helpful? Give feedback.
-
|
No change, still locking |
Beta Was this translation helpful? Give feedback.
-
Hopefully, PR #4116 has resolved this issue. The 64-bit libvips 8.15.4 (unreleased) Windows binaries and corresponding PDBs built from commit libvips/build-win64-mxe@c92570b can be found here: Hopefully that would fix it. If not, I'm still need to find a way to reproduce this locally. |
Beta Was this translation helpful? Give feedback.
-
|
Still locking |
Beta Was this translation helpful? Give feedback.
-
Ah bummer, to summarize this issue:
@jcupitt Do you have any idea what could be the reason for this? (I started a new discussion thread as the other one is already quite full) |
Beta Was this translation helpful? Give feedback.
-
|
Only quick tests but |
Beta Was this translation helpful? Give feedback.
-
|
I could reproduce this issue on my AMD Ryzen 9 7900 workstation, or at least it seems to hit that 30 seconds semaphore timeout with those I'll investigate, but it may take a while. |
Beta Was this translation helpful? Give feedback.
-
Hmm, the new thread recycler was also available in 8.14.0-rc1. In any case, thanks for doing this bisection! |
Beta Was this translation helpful? Give feedback.
-
|
8.14.1 does not work And in the changelog of 8.14.1 there is |
Beta Was this translation helpful? Give feedback.
-
|
So I've been running my application over the night and so far over 14000 images with countless operations, without stopping. It really seems to be 8.14.1 which contains this bug |
Beta Was this translation helpful? Give feedback.
-
|
I had another thought -- the thing that sizes the threadpool up and down is relatively new and less well tested. Perhaps there's an issue there? If it sizes the threadpool down too far, perhaps there are so few threads that evaluation cannot continue and it deadlocks? It doesn't seem likely, but who knows. It might be worth testing a binary without that feature. We could maybe add a new env var, perhaps |
Beta Was this translation helpful? Give feedback.
-
|
Hmm, I just looked into this, but the shrinking and expanding of the threadpool never occur with a concurrency of 2 (as set in the last example program), at least not on Linux. Which is probably the reason why PR #4129 didn't work for this user. The last stack trace posted in this discussion seems to occur here: libvips/libvips/iofuncs/threadpool.c Lines 674 to 676 in d622bc8 If that deadlocks, the thread must somehow ungracefully terminated, which is very odd. So, my latest suspicion is that this is some kind of thread limitation in the OS, or worse, a hardware issue. Of course, I could be wrong. |
Beta Was this translation helpful? Give feedback.
-
|
... a deadlock might occur if the thread never executes |
Beta Was this translation helpful? Give feedback.
-
|
Looking at this again, I saw that libvips/libvips/iofuncs/semaphore.c Lines 86 to 92 in ad8b246 This sounds a bit expensive, as it only decrements by one. We also increment/decrement only by one throughout the codebase, so perhaps this could be simplified to a single |
Beta Was this translation helpful? Give feedback.
-
|
Oop, you're right, that is wasteful. I think if (n == 1)
g_cond_signal(s->cond);
else if (n > 1)
g_cond_broadcast(s->cond);(untested!!) |
Beta Was this translation helpful? Give feedback.
-
|
I tried this on main and it seems to work, but I've not seen a performance improvement. I tested |
Beta Was this translation helpful? Give feedback.
-
|
I suspect this is a issue in Details@@ -19,7 +19,7 @@ int main(int argc, char* argv[])
for (auto& thread : test) {
thread = std::thread([] {
VipsImage* text, * text2;
- VipsImage* mem, * mem2;
+ double out, out2;
if (vips_text(&text, "Test Text", "font", "sans 50", nullptr))
vips_error_exit(nullptr);
@@ -30,16 +30,14 @@ int main(int argc, char* argv[])
if (!text2 || !text)
vips_error_exit(nullptr);
- if (!(mem = vips_image_copy_memory(text)))
+ if (vips_avg(text, &out, nullptr))
vips_error_exit(nullptr);
- if (!(mem2 = vips_image_copy_memory(text2)))
+ if (vips_avg(text2, &out2, nullptr))
vips_error_exit(nullptr);
g_object_unref(text);
g_object_unref(text2);
- g_object_unref(mem);
- g_object_unref(mem2);
});
}
std::cout << "Join";I had an attempt to sync Which seems to work, but I've no idea way 🤷♂️. Interestingly, the CI also fails on that commit: @GitFred4's bisecting suggests that the issue might have been introduced in the threadpool scavenger (PR #3253). However, I'm not sure why it only fails in |
Beta Was this translation helpful? Give feedback.
-
|
Sounds promising, that write access violation sounds like a heap-use-after-free bug. In theory, it could race with the free here: libvips/libvips/iofuncs/threadset.c Line 166 in 9254e9e and when it tries to access libvips/libvips/iofuncs/threadset.c Line 285 in 9254e9e I'll investigate an alternative patch. |
Beta Was this translation helpful? Give feedback.
-
|
New Windows binaries are available here: (these binaries were built from commit libvips/build-win64-mxe@4e40987 and includes the latest changes from PR #4137) This new implementation leverages |
Beta Was this translation helpful? Give feedback.
-
|
No new bugs or old bugs, i guess it's fixed. |
Beta Was this translation helpful? Give feedback.
-
|
That's fantastic news! Thanks for testing and initially reporting this issue. The fix will probably be included in the upcoming 8.15.4 release. FWIW, I've updated my previous comment with new binaries of that PR. These new binaries limits the number of idle threads to a maximum of two. The earlier binaries lacked this limit, which could lead to thread oversubscription, where too many threads remained idle. |
Beta Was this translation helpful? Give feedback.
-
|
Yes, well done for being so persistent! I'll have some time to review later this week (hopefully). |
Beta Was this translation helpful? Give feedback.
-
|
This should be fixed after PR #4137. |
Beta Was this translation helpful? Give feedback.
-
Hi,
I have several threads, each processing its own image.
The images are only used in one thread.
Sometimes one of my threads does not finish and it always seems to get stuck in the data or inplace function of the vips::VImage class.
I can't reproduce the behaviour, it only happens sometimes. Maybe one in 1000 image inplace calls or less.
I have waited about 30-40 minutes and nothing has changed. Normally one inplace call should never exceed 20 seconds.
vips_concurrency was set to 5.
It always gets stuck in the g_cond_wait(s->cond, s->mutex); function in semaphore.c row 126
See attached Image of all running Threads.
Windows 10
LipVips 8.15.2
Beta Was this translation helpful? Give feedback.
All reactions