[hdEmbree] ensure we respect PXR_WORK_THREAD_LIMIT (standalone) #3370
Conversation
|
Filed as internal issue #USD-10340 |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
pmolodo
force-pushed
the
pr/hdEmbree-respect-PXR_WORK_THREAD_LIMIT-standalone
branch
from
57a4e85 to
5a818ce
Compare
pmolodo
force-pushed
the
pr/hdEmbree-respect-PXR_WORK_THREAD_LIMIT-standalone
branch
from
5a818ce to
32d1f06
Compare
|
Updated to not modify the API of the work library |
pmolodo
force-pushed
the
pr/hdEmbree-respect-PXR_WORK_THREAD_LIMIT-standalone
branch
from
32d1f06 to
98f79a8
Compare
pmolodo
force-pushed
the
pr/hdEmbree-respect-PXR_WORK_THREAD_LIMIT-standalone
branch
from
98f79a8 to
a6d8610
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hey Paul, just to clarify: the root issue here is that we're spawning TBB threads from the hydra render thread, which does not have a tbb::task_scheduler_init because libWork only constructs one on the main thread at static initialization time? Is that roughly correct? I'm not sure having it be scoped is quite right; the render thread is long lived and we should be able to just initialize one the first time we get into the callback and keep it around. I don't have a sense of how heavy they are to construct, though. We will likely need to tweak this slightly to make it oneTBB-safe; I can provide suggestions on github, or just fix it up inline as I pull it down; let me know. |
Things already work with oneTBB - it handles the thread limits in a different / better / more global way (though I no longer recall the exact details at this moment - can find it if you want, though). This PR should be
Yeah, it's been a minute since I wrote this / investigated, but that sounds about right. And absolutely, I think the "correct" thing to do would be to fix this in a more general way at a higher level - ie, libwork. However, since this issue only affects "old" TBB (things already work nicely in oneTBB), and I was a bit leery of making changes to If you want to fix it "right" at a higher level, though, go for it!
Fixing inline as you go is fine with me! |
tcauchois
left a comment
tcauchois
left a comment
There was a problem hiding this comment.
A few things that came up in internal review of this.
Let me know (1) what you think of the changes; and (2) whether I should check the modified version in and then send you a commit link, or bring it back to github as suggestions for you to apply to the PR or what.
| // change API of work lib. Will go away once we don't need to support tbb < 12 | ||
| // (ie, pre-OneTBB) | ||
| static unsigned | ||
| HdEmbree_GetConcurrencyLimitSetting() |
There was a problem hiding this comment.
I think I have consensus internally to move this function back into "work". If I make that change, do you want me to post it here for review or just check it in?
| auto limit = HdEmbree_GetConcurrencyLimitSetting(); | ||
| if (limit != 0) { | ||
| _tbbTaskSchedInit = | ||
| std::make_unique<tbb::task_scheduler_init>(limit); |
There was a problem hiding this comment.
I really wanted to find a way to jam this back into "work", but we're also simultaneously abstracting "work" to be able to use oneTBB as well as non-TBB threadpools, and there weren't any API contracts we felt good about adding. So I think keeping the tbb::task_scheduler_init here is the lesser of two evils; the only negative side effect it has is creating a hard dependency on TBB, which is fine for a few years I think.
I don't think the task_scheduler_init can be scoped like you're doing here, but to give it the lifetime of the thread we'd need to either add it to HdRenderThread directly or subclass HdRenderThread, which seems like too much work, so I'm fine with leaving this as-is.
|
|
||
| // Render by scheduling square tiles of the sample buffer in a parallel | ||
| // for loop. | ||
| #if TBB_INTERFACE_VERSION_MAJOR < 12 |
There was a problem hiding this comment.
See comment above about scoping; anyway, at the least this should probably move to the top of the function. I've done this internally but can post it back if you'd like.
| #include <thread> | ||
|
|
||
| // ------------------------------------------------------------------------- | ||
| // Old TBB workaround - can remove once OneTBB is mandatory |
There was a problem hiding this comment.
| // Old TBB workaround - can remove once OneTBB is mandatory | |
| // Old TBB workaround - we plan to remove this once OpenUSD adopts | |
| // oneTBB as a min spec. Note: for non-TBB-based work implementations, | |
| // this code shouldn't cause any trouble. |
pmolodo
force-pushed
the
pr/hdEmbree-respect-PXR_WORK_THREAD_LIMIT-standalone
branch
from
a6d8610 to
112039b
Compare
Fixes an issue where pre-oneTBB, threads spawned from the non-root thread (such as the render thread) would not respect PXR_WORK_THREAD_LIMIT. WorkGetConcurrencyLimitSetting is now exported from libwork, and for old TBB versions we create an appropriate task_scheduler_init from the render thread. - renderThread-spawned processes don't respect PXR_WORK_THREAD_LIMIT Closes PixarAnimationStudios#3198 Closes PixarAnimationStudios#3370 (Internal change: 2369113)
3 participants
Important
This is a duplicate of this PR:
...except pulled out of the UsdLux-HdEmbree PR "chain", to make for easier standalone integration, as it has no other PR dependencies, and is a simple/small change.
I will close this PR if the other is merged first, and vice-versa.
Description of Change(s)
Fixes an issue where pre-oneTBB, threads spawned from the non-root thread (such as the render thread) would not respect PXR_WORK_THREAD_LIMIT
Fixes Issue(s)