-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[hdEmbree] ensure we respect PXR_WORK_THREAD_LIMIT (standalone) #3370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,87 @@ | |
| #include <chrono> | ||
| #include <thread> | ||
|
|
||
| // ------------------------------------------------------------------------- | ||
| // Old TBB workaround - can remove once OneTBB is mandatory | ||
| // ------------------------------------------------------------------------- | ||
| #include <tbb/tbb_stddef.h> | ||
|
|
||
| #if TBB_INTERFACE_VERSION_MAJOR < 12 | ||
|
|
||
| #include <memory> | ||
|
|
||
| #include <tbb/task_scheduler_init.h> | ||
|
|
||
| PXR_NAMESPACE_OPEN_SCOPE | ||
|
|
||
| // PXR_WORK_THREAD_LIMIT isn't exported as part of it's api, and we're not | ||
| // part of the work library, so we can't use: | ||
| // extern TfEnvSetting<int> PXR_WORK_THREAD_LIMIT; | ||
| extern std::variant<int, bool, std::string> const * | ||
| Tf_GetEnvSettingByName(std::string const&); | ||
|
|
||
| PXR_NAMESPACE_CLOSE_SCOPE | ||
|
|
||
| namespace { | ||
|
|
||
| PXR_NAMESPACE_USING_DIRECTIVE | ||
|
|
||
| // This function always returns either 0 (meaning "no change") or >= 1 | ||
| // | ||
| // Duplication of code from threadLimits.cpp - copied here to avoid having to | ||
| // change API of work lib. Will go away once we don't need to support tbb < 12 | ||
| // (ie, pre-OneTBB) | ||
| static unsigned | ||
| HdEmbree_NormalizeThreadCount(const int n) | ||
| { | ||
| // Zero means "no change", and n >= 1 means exactly n threads, so simply | ||
| // pass those values through unchanged. | ||
| // For negative integers, subtract the absolute value from the total number | ||
| // of available cores (denoting all but n cores). If |n| >= number of cores, | ||
| // clamp to 1 to set single-threaded mode. | ||
| return n >= 0 ? n : std::max<int>(1, n + WorkGetPhysicalConcurrencyLimit()); | ||
| } | ||
|
|
||
|
|
||
| // Returns the normalized thread limit value from the environment setting. Note | ||
| // that 0 means "no change", i.e. the environment setting does not apply. | ||
| // | ||
| // Duplication of code from threadLimits.cpp - copied here to avoid having to | ||
| // change API of work lib. Will go away once we don't need to support tbb < 12 | ||
| // (ie, pre-OneTBB) | ||
| static unsigned | ||
| HdEmbree_GetConcurrencyLimitSetting() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| { | ||
| std::variant<int, bool, std::string> const * | ||
| variantValue = Tf_GetEnvSettingByName("PXR_WORK_THREAD_LIMIT"); | ||
| int threadLimit = 0; | ||
| if (int const *value = std::get_if<int>(variantValue)) { | ||
| threadLimit = *value; | ||
| } | ||
| return HdEmbree_NormalizeThreadCount(threadLimit); | ||
| } | ||
|
|
||
|
|
||
| // Make the calling context respect PXR_WORK_THREAD_LIMIT, if run from a thread | ||
| // other than the main thread (ie, the renderThread) | ||
| class _ScopedThreadScheduler { | ||
| public: | ||
| _ScopedThreadScheduler() { | ||
| auto limit = HdEmbree_GetConcurrencyLimitSetting(); | ||
| if (limit != 0) { | ||
| _tbbTaskSchedInit = | ||
| std::make_unique<tbb::task_scheduler_init>(limit); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| } | ||
|
|
||
| std::unique_ptr<tbb::task_scheduler_init> _tbbTaskSchedInit; | ||
| }; | ||
|
|
||
| } // anonymous namespace | ||
|
|
||
| #endif // TBB_INTERFACE_VERSION_MAJOR < 12 | ||
|
|
||
|
|
||
| namespace { | ||
|
|
||
| PXR_NAMESPACE_USING_DIRECTIVE | ||
|
|
@@ -453,6 +534,9 @@ HdEmbreeRenderer::Render(HdRenderThread *renderThread) | |
|
|
||
| // Render by scheduling square tiles of the sample buffer in a parallel | ||
| // for loop. | ||
| #if TBB_INTERFACE_VERSION_MAJOR < 12 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| _ScopedThreadScheduler scheduler; | ||
| #endif | ||
| // Always pass the renderThread to _RenderTiles to allow the first frame | ||
| // to be interrupted. | ||
| WorkParallelForN(numTilesX*numTilesY, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.