From 0ed71ab53bf93d0fd86cc8eb1f22f5a3fc8a2d6a Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Mon, 24 Jul 2023 10:46:21 -0700 Subject: [PATCH] fix an issue causing callbacks to be called too late We were waiting for programs from both queues to be compiled before calling the callback associated with one queue. In practice this caused the callback associated with high priority programs to be called only after low priority programs were ready. Also cleanup-up "token" so that it doesn't store the priority. Update the documentation and sample to better reflect what the implementation does. --- .../src/opengl/ShaderCompilerService.cpp | 39 ++++++++++++------- .../src/opengl/ShaderCompilerService.h | 8 ++-- .../src/opengl/platforms/PlatformCocoaGL.mm | 3 ++ filament/include/filament/Material.h | 8 ++++ libs/gltfio/src/ArchiveCache.cpp | 30 +++++++------- 5 files changed, 57 insertions(+), 31 deletions(-) diff --git a/filament/backend/src/opengl/ShaderCompilerService.cpp b/filament/backend/src/opengl/ShaderCompilerService.cpp index 5e301d83cb..4542f252d4 100644 --- a/filament/backend/src/opengl/ShaderCompilerService.cpp +++ b/filament/backend/src/opengl/ShaderCompilerService.cpp @@ -87,7 +87,6 @@ struct ShaderCompilerService::ProgramToken { BlobCacheKey key; std::future binary; - CompilerPriorityQueue priorityQueue = CompilerPriorityQueue::HIGH; bool canceled = false; }; @@ -148,12 +147,23 @@ void ShaderCompilerService::CompilerThreadPool::init( } } +auto ShaderCompilerService::CompilerThreadPool::find( + program_token_t const& token) -> std::pair { + for (auto&& q: mQueues) { + auto pos = std::find_if(q.begin(), q.end(), [&token](auto&& item) { + return item.first == token; + }); + if (pos != q.end()) { + return { q, pos }; + } + } + // this can happen if the program is being processed right now + return { mQueues[0], mQueues[0].end() }; +} + auto ShaderCompilerService::CompilerThreadPool::dequeue(program_token_t const& token) -> Job { - auto& q = mQueues[size_t(token->priorityQueue)]; - auto pos = std::find_if(q.begin(), q.end(), [&token](auto&& item) { - return item.first == token; - }); Job job; + auto&& [q, pos] = find(token); if (pos != q.end()) { std::swap(job, pos->second); q.erase(pos); @@ -169,9 +179,10 @@ void ShaderCompilerService::CompilerThreadPool::makeUrgent(program_token_t const mQueueCondition.notify_one(); } -void ShaderCompilerService::CompilerThreadPool::queue(program_token_t const& token, Job&& job) { +void ShaderCompilerService::CompilerThreadPool::queue(CompilerPriorityQueue priorityQueue, + program_token_t const& token, Job&& job) { std::unique_lock const lock(mQueueLock); - mQueues[size_t(token->priorityQueue)].emplace_back(token, std::move(job)); + mQueues[size_t(priorityQueue)].emplace_back(token, std::move(job)); mQueueCondition.notify_one(); } @@ -204,7 +215,9 @@ void ShaderCompilerService::init() noexcept { // also glProgramBinary blocks if other threads are compiling. // - on Mali shader compilation can be multithreaded, but program linking happens on // a single service thread, so we don't bother using more than one thread either. - // - on desktop we could use more threads, tbd. + // - on macOS (M1 MacBook Pro/Ventura) there is global lock around all GL APIs when using + // a shared context, so parallel shader compilation yields no benefit. + // - on windows/linux we could use more threads, tbd. if (mDriver.mPlatform.isExtraContextSupported()) { mShaderCompilerThreadCount = 1; mCompilerThreadPool.init(mUseSharedContext, @@ -230,13 +243,13 @@ ShaderCompilerService::program_token_t ShaderCompilerService::createProgram( token->gl.program = OpenGLBlobCache::retrieve(&token->key, mDriver.mPlatform, program); if (!token->gl.program) { + CompilerPriorityQueue const priorityQueue = program.getPriorityQueue(); if (mShaderCompilerThreadCount) { // set the future in the token and pass the promise to the worker thread std::promise promise; token->binary = promise.get_future(); - token->priorityQueue = program.getPriorityQueue(); // queue a compile job - mCompilerThreadPool.queue(token, + mCompilerThreadPool.queue(priorityQueue, token, [this, &gl, promise = std::move(promise), program = std::move(program), token]() mutable { @@ -304,7 +317,7 @@ ShaderCompilerService::program_token_t ShaderCompilerService::createProgram( } - runAtNextTick(token->priorityQueue, token, [this, token]() { + runAtNextTick(priorityQueue, token, [this, token]() { if (mShaderCompilerThreadCount) { if (!token->gl.program) { // TODO: see if we could completely eliminate this callback here @@ -431,8 +444,8 @@ void ShaderCompilerService::notifyWhenAllProgramsAreReady(CompilerPriorityQueue // list all programs up to this point, both low and high priority utils::FixedCapacityVector, false> tokens; tokens.reserve(mRunAtNextTickOps.size()); - for (auto& [priority_, token, fn_] : mRunAtNextTickOps) { - if (token) { + for (auto& [itemPriority, token, fn] : mRunAtNextTickOps) { + if (token && fn && itemPriority == priority) { tokens.push_back(token); } } diff --git a/filament/backend/src/opengl/ShaderCompilerService.h b/filament/backend/src/opengl/ShaderCompilerService.h index 833636965c..3fc37abf5b 100644 --- a/filament/backend/src/opengl/ShaderCompilerService.h +++ b/filament/backend/src/opengl/ShaderCompilerService.h @@ -96,17 +96,19 @@ private: using Job = utils::Invocable; void init(bool useSharedContexts, uint32_t threadCount, OpenGLPlatform& platform) noexcept; void exit() noexcept; - void queue(program_token_t const& token, Job&& job); + void queue(CompilerPriorityQueue priorityQueue, program_token_t const& token, Job&& job); void makeUrgent(program_token_t const& token); private: + using Queue = std::deque>; std::vector mCompilerThreads; std::atomic_bool mExitRequested{ false }; std::mutex mQueueLock; std::condition_variable mQueueCondition; - std::array>, 2> mQueues; - Job mUrgentJob; + std::array mQueues; + Job mUrgentJob; // needs mQueueLock as well Job dequeue(program_token_t const& token); // lock must be held + std::pair find(program_token_t const& token); }; OpenGLDriver& mDriver; diff --git a/filament/backend/src/opengl/platforms/PlatformCocoaGL.mm b/filament/backend/src/opengl/platforms/PlatformCocoaGL.mm index 195d8d17e7..b19ec4cbbf 100644 --- a/filament/backend/src/opengl/platforms/PlatformCocoaGL.mm +++ b/filament/backend/src/opengl/platforms/PlatformCocoaGL.mm @@ -173,6 +173,9 @@ Driver* PlatformCocoaGL::createDriver(void* sharedContext, const Platform::Drive } bool PlatformCocoaGL::isExtraContextSupported() const noexcept { + // macOS supports shared contexts however, it looks like the implementation uses a global + // lock around all GL APIs. It's a problem for API calls that take a long time to execute, + // one such call is e.g.: glCompileProgram. return true; } diff --git a/filament/include/filament/Material.h b/filament/include/filament/Material.h index 5b9314a042..04d4039cb0 100644 --- a/filament/include/filament/Material.h +++ b/filament/include/filament/Material.h @@ -166,6 +166,14 @@ public: * many previous frames are enqueued in the backend. This also varies by backend. Therefore, * it is recommended to only call this method once per material shortly after creation. * + * If the same variant is scheduled for compilation multiple times, the first scheduling + * takes precedence; later scheduling are ignored. + * + * caveat: A consequence is that if a variant is scheduled on the low priority queue and later + * scheduled again on the high priority queue, the later scheduling is ignored. + * Therefore, the second callback could be called before the variant is compiled. + * However, the first callback, if specified, will trigger as expected. + * * @param priority Which priority queue to use, LOW or HIGH. * @param variants Variants to include to the compile command. * @param handler Handler to dispatch the callback or nullptr for the default handler diff --git a/libs/gltfio/src/ArchiveCache.cpp b/libs/gltfio/src/ArchiveCache.cpp index 118beec0fd..47798741fa 100644 --- a/libs/gltfio/src/ArchiveCache.cpp +++ b/libs/gltfio/src/ArchiveCache.cpp @@ -118,23 +118,23 @@ Material* ArchiveCache::getMaterial(const ArchiveRequirements& reqs) { mMaterials[i] = Material::Builder() .package(spec.package, spec.packageByteCount) .build(mEngine); + + // Don't attempt to precompile shaders on WebGL. + // Chrome already suffers from slow shader compilation: + // https://github.com/google/filament/issues/6615 + // Precompiling shaders exacerbates the problem. + #if !defined(__EMSCRIPTEN__) + // First compile high priority variants + mMaterials[i]->compile(Material::CompilerPriorityQueue::HIGH, + UserVariantFilterBit::DIRECTIONAL_LIGHTING | + UserVariantFilterBit::DYNAMIC_LIGHTING | + UserVariantFilterBit::SHADOW_RECEIVER); + + // and then, everything else at low priority + mMaterials[i]->compile(Material::CompilerPriorityQueue::LOW); + #endif } - // Don't attempt to precompile shaders on WebGL. - // Chrome already suffers from slow shader compilation: - // https://github.com/google/filament/issues/6615 - // Precompiling shaders exacerbates the problem. -#if !defined(__EMSCRIPTEN__) - // compile everything at low priority - mMaterials[i]->compile(Material::CompilerPriorityQueue::LOW); - - // promote variants we care about to high priority - mMaterials[i]->compile(Material::CompilerPriorityQueue::HIGH, - UserVariantFilterBit::DIRECTIONAL_LIGHTING | - UserVariantFilterBit::DYNAMIC_LIGHTING | - UserVariantFilterBit::SHADOW_RECEIVER); -#endif - return mMaterials[i]; } }