RenderKit / embree

Embree ray tracing kernels repository.
Apache License 2.0
2.32k stars 383 forks source link

Ensure that a user-defined thread count takes precedence over getNumberOfLogicalThreads() #460

Closed dvicini closed 3 weeks ago

dvicini commented 9 months ago

Hi,

This pull request ensures that a user-defined thread count (i.e., in the device's config argument) consistently takes precedence over the detected number of logical threads. Previously, Embree would fail if the user-provided thread count was larger than the internally determined one. It internally allocated threading related buffers based on the return value of getNumberOfLogicalThreads(), which would then cause a segfault when invoking rtcJoinCommitScene using a thread pool that has a larger number of threads. Concretely, I encountered this when running Mitsuba 3 on a cluster setup. Currently, Embree effectively requires the user thread pool to be smaller or equal to getNumberOfLogicalThreads(), but this is not always straightforward to guarantee. Moreover, I would argue that this is neither documented nor immediately obvious for users.

This fix seems to work for me, but I am open to suggestions if it is not general enough to be merged in this state.

Best, Delio

dvicini commented 7 months ago

I would appreciate feedback on this and would also be happy to adjust the PR as needed if there are still some issues with it. Thanks!

freibold commented 2 months ago

Hi! Thanks for the PR! This was merged into our internal devel branch a while ago and is already released: https://github.com/RenderKit/embree/commit/fff2e0f6e0988a97fce4952372a3718f73f5070d. Sorry, that the attribution is missing. I don't know what happened there.

It would be interesting to know why use use the Embree's internal tasking system. What's the reason why you don't use TBB? I only ask because we discuss from time to time whether we want/can remove the internal tasking system to reduce maintenance overhead. But if this is something that is really valuable to you we'll of course keep it alive.

Cheers, Florian

dvicini commented 2 months ago

Ah great, I didn't realize it got merged!

We encountered this issue when using parallel scene commit using Mitsuba (https://github.com/mitsuba-renderer/mitsuba3). Mitsuba doesn't use TBB, it uses it's own threading system. As far as I can tell, the only interaction with Embrees internal tasks is during scene construction.

I am not sure what else the embree task system supports, but for us the main use is to run rtcJoinCommitScene from a thread pool we manage on our side.

wjakob commented 2 months ago

To add further historical context: Mitsuba/Dr.Jit implement a JIT-compiler that automatically parallelizes user-provided code. The multi-threading aspect of this originally relied on TBB via the Task API. Changes in TBB related to the oneAPI transition and various associated refactoring of the Task API caused so much friction that we eventually gave up the TBB dependency, replacing it with a minimal parallelization layer specific to the needs of Dr.Jit (https://github.com/mitsuba-renderer/nanothread).

In an application that already maintains an internal thread pool, it seems wasteful to spin up another one to build a BVH. So the fact that Embree has a "bring your own threads" interface via rtcJoinCommitScene is really helpful.