I confirm that fixes it.

I had a go ... now there's VIPS_MAX_THREADS to preallocate the threadset, and set a hard limit. What do you think? Allocation happens on first use, is that OK for wasm?

Thanks! For Wasm, it needs to setup the fixed threadpool during vips_init(), so I moved it to vips__threadpool_init() instead with commit kleisauke/wasm-vips@ee410bf.

wasm-vips had previously reverted commit 6946c3b for this purpose, but that probably wouldn't work on macOS, where we need to delay setting up the threadpool to ensure fork() safety.

I wonder if we should expose VIPS_MAX_THREADS? As far as I know, WebAssembly is the only platform that requires this, so I'm not sure if we should include it by default, we could perhaps #ifdef __EMSCRIPTEN__ it instead.

Note that running the wasm-vips test suite still seems to exhaust the fixed threadpool, see for example this run:
https://github.com/kleisauke/wasm-vips/actions/runs/3312785036/jobs/5470249864

I'm not sure what's causing this, the previous GLib threadpool implementation didn't had this issue. It can be reproduced by running the test suite on the reimplement-threadpool-wasm branch or with:

$ export VIPS_MAX_THREADS="$(($(nproc) + 3))"
$ echo $VIPS_MAX_THREADS
9

(+3 is the amount of background threads required by vips_sink_disc, I'm aware that dzsave may require more threads than this, but I haven't compiled libgsf for Wasm yet)

Ah, according to the g_thread_pool_push() documentation, new threads are only started when the number of currently running threads is less than the maximal allowed number of threads. Otherwise, it stays in the queue until a thread in the pool finishes its previous task.

I confirm that fixes it.

Hooray!

For Wasm, it needs to setup the fixed threadpool during vips_init(),

Ah I wasn't sure, OK, let's move it. I hoped to delay making the threads until first use to help systems like macOS, but it sounds like that's not possible (or useful).

I wonder if we should expose VIPS_MAX_THREADS?

I was thinking it would be handy for testing, but you're right, not useful for most people outside wasm. We can leave it undocumented and unsupported.

+3 is the amount of background threads required by vips_sink_disc

I think it's two, isn't it? There's one thread for each of the two write behind buffers, plus the main thread (which we don't count). I see:

$ VIPS_MAX_THREADS=5 VIPS_CONCURRENCY=3 vips gaussblur k2.jpg x.jpg 100
$

So concurrency 3 runs with six threads: 3 workers, 2 write-behind, one main thread.

Otherwise, it stays in the queue until a thread in the pool finishes its previous task.

Perhaps we should add a queue like this to threadset? At the moment it just hard fails. Though I think there's maybe a danger of deadlock, ouch.

I think it's two, isn't it?

Oh huh, perhaps I miscounted. Though, this one seems to fail.

$ vips black x.jpg 100 100
$ VIPS_MAX_THREADS=5 VIPS_CONCURRENCY=3 vips autorot x.jpg x2.jpg
VipsThreadset: threadset is exhausted

Commit kleisauke@a11afaa on the wasm-master branch also seems to fail on the test suite. That branch contains the rebased wasm-vips patches applied to the master branch for reference.

Perhaps we should add a queue like this to threadset? At the moment it just hard fails. Though I think there's maybe a danger of deadlock, ouch.

Deadlocks are indeed the symptoms when things don't work out. For example, I also had to apply commit kleisauke@6100d34 and needed to configure libwebp with --disable-threading for Wasm, since this fixed thread limit applies to the entire binary.

I think that example is running two pipelines:

$ VIPS_MAX_THREADS=7 VIPS_CONCURRENCY=3 vips autorot k2.jpg x.jpg --vips-progress
vips temp-4: 1450 x 2048 pixels, 3 threads, 1450 x 16 tiles, 128 lines in buffer
vips k2.jpg: 1450 x 2048 pixels, 3 threads, 1450 x 16 tiles, 128 lines in buffer
vips k2.jpg: done in 0.0154s          
vips temp-4: done in 0.0304s          

Some libvips threads are optional (eg. when it tries to expand a threadpool), but some must happen or we'll deadlock (eg. it must have a thread watching a write-behind buffer).

We could add API for the optional case. It'd make it a bit more robust against threadset exhaustion.

Ah, you're right. vips_autorot() is a non-sequential operation, so two pipelines are needed. For example, it can also be reproduced with the gaussblur sample when this line is commented out:

I've clarified with commit kleisauke/wasm-vips@acbfff6 why the +3 is needed.

We could add API for the optional case. It'd make it a bit more robust against threadset exhaustion.

GLib threadpool's uses the g_async_queue_* functions for this. Though, I'm not sure if we should do the same, we previously passed max_threads = -1 to g_thread_pool_new(), so a new thread was spawned when no idle threads were available. IIUC, a queue implementation is only handy for a fixed threadpool (VIPS_MAX_THREADS), it probably adds some overhead when there's no hard limit.

It's a bit unfortunate that WebAssembly has this limitation. :(

I think we're done, let's merge and try living with it for a bit.

Thank you very much for reviewing this thing!

GLib threadpool's uses the g_async_queue_* functions for this. Though, I'm not sure if we should do the same, we previously passed max_threads = -1 to g_thread_pool_new(), so a new thread was spawned when no idle threads were available. IIUC, a queue implementation is only handy for a fixed threadpool (VIPS_MAX_THREADS), it probably adds some overhead when there's no hard limit.

FWIW, I implemented this with commit kleisauke@fe02527 for wasm-vips (only the changes in threadpool.c and threadset.c are interesting). It pushes tasks to the global GAsyncQueue when the threadset is limited with VIPS_MAX_THREADS and exhausted. vips_threadset_work() is slightly modified to pop these tasks from the global queue and process them before the thread becomes idle. This seems to pass the wasm-vips testsuite using:

VIPS_MAX_THREADS="$(($(nproc) + 3))"

I'm not sure if it's worth upstreaming that, since it could easily deadlock when being used in combination with dzsave or when VIPS_MAX_THREADS is lower than 3.

Oh, interesting.

It'd be great if we could upstream everything and have wasm as a fully supported target. Maybe we could hide the extra code behind ifdef WASM?

I suppose the downside is that libvips would then have code that was difficult to test, and added maintenance effort. What do you think?

I'd like to officially support Wasm as a supported target in libvips. Indeed, you can just #ifdef Wasm-specific code behind __EMSCRIPTEN__ or __wasm__ (if necessary).

Unfortunately, the main blocker for this is GLib, see for example:
GNOME/glib@2.75.0...kleisauke:wasm-vips-2.75.0

I suppose this should be discussed in GLib first, especially commit kleisauke/glib@4ed9ec0 (since that would cause an ABI break). GLib currently deliberately ignores the -Wcast-function-type and -Wbad-function-cast warnings due to this (and I'm aware that running with -fsanitize=cfi-icall is not supported either) - see:
https://gitlab.gnome.org/GNOME/glib/-/blob/2.75.0/meson.build#L483-485
https://gitlab.gnome.org/GNOME/glib/-/blob/2.75.0/meson.build#L502-504

For this reason, I marked the GLib patches as stalled in the "Upstream patches" item at Stable release for wasm-vips, but hopefully I can revisit that in the future.

Ah I see. They would really hate an ABI break, but I can see it might be worthwhile to open a discussion about it and raise the issue. They might want to delay for glib-3.0.

I did a quick search and couldn't see a statement from the glib maintainers on emscripten support. If they've not taken an official position, perhaps opening a PR with that change would be useful?

I mentioned the patches and my intention to upstream them in this comment, but I plan to discuss this also on GNOME's Discourse instance (using the glib tag and possibly the strawman tag for the function pointer cast issues), this to avoid that either the issue or MR (GitLab's meaning of PR) is immediately closed (e.g. like this one 😅).

Though, perhaps I should check the #gtk IRC channel on irc.gnome.org first to see whether the maintainers are interested in this. Hopefully they won't reject this in favor of https://gitlab.gnome.org/GNOME/gjs.

/cc @RReverser you might also be interested in this.

Ah sensible, yes I remember that PR being closed without comment. How can a PR not be actionable? Ah well.

Sure, discourse is maybe a better place to start discussion. Please post a link here, I'd be interested to read.