BinomialLLC / basis_universal

Basis Universal GPU Texture Codec
Apache License 2.0
2.72k stars 267 forks source link

Fix m_num_active_jobs management when pool is used concurrently #277

Closed zeux closed 2 years ago

zeux commented 2 years ago

Multiple sequences of

add_job
add_job
wait_for_all

Didn't behave correctly before this change because wait_for_all would run jobs without increasing the number of active jobs; that could lead to other threads calling wait_for_all, exiting early while some jobs were in flight, and subsequently trying to use the data and crashing.

Fixing this is important when using multiple concurrent encoders in the same process and having them target the same job pool (with this change this works correctly even though isn't necessarily optimal, something to look into in the future).

This change also fixes some missing std::move's and merges two add_job functions.

richgel999 commented 2 years ago

Yes definitely: "Fixing this is important when using multiple concurrent encoders in the same process and having them target the same job pool (with this change this works correctly even though isn't necessarily optimal, something to look into in the future)."

This usage scenario (where multiple concurrent encoders use the same job pool) wasn't a supported use case originally, so I can see why it would faceplant. Instead, the idea/intention was that you would use the encoder with a dummy job pool (i.e. one that didn't create any worker threads, and just executed the tasks on the same thread), and you would invoke X parallel encoders on different threads. This is what we're doing in v1.16.

I want to merge this, but I need to test it a bunch first.

zeux commented 2 years ago

Yeah, I understand that it wasn't a supported use case. The problem with separate job pools is that it's really difficult to size them - eg the dummy job pool idea works very well when you have a video input (presumably hundreds to thousands of frames of the same size), but doesn't work as well when you have a collection of textures with different sizes and different target formats (ETC vs UASTC), since you can't saturate the system due to imbalanced load of different textures.

This change is a simple fix to make this work; there's a way to make this a bit more scalable which I've implemented for gltfpack and I intend to contribute that in a future PR but it's a larger rework of job_pool so it's more difficult to review.

zeux commented 2 years ago

I'm going to close this since there's a different, larger but more important, change that I will submit a PR for - by itself this PR doesn't solve an obvious problem that basisu currently has, but an upcoming PR will.