[Impeller] Use the IO context for OpenGL program setup#35
[Impeller] Use the IO context for OpenGL program setup#35xiaowei-guan wants to merge 15 commits into
Conversation
|
/gemini review |
1 similar comment
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the lazy_shader_mode flag and introduces a PipelineCompileQueue system for Impeller's GLES and Vulkan backends to manage asynchronous pipeline compilation. This architecture allows the renderer to eagerly execute pending compilation jobs if they are required immediately by the render thread. Feedback identifies a race condition in the GLES sequential processing logic and suggests that the GLES implementation should wait for pipeline creation to finish to maintain the effectiveness of the prioritization buffer.
JSUYA
left a comment
JSUYA
left a comment
There was a problem hiding this comment.
Do I only need to apply this patch to improve launch performance?
I tested this PR, but I did not feel any improvement in launch speed. (I did not measure it with a timer; it is just a feeling.) I might be wrong.
(I tested the app with engine.so and impeller renderer that applied this PR.)
It looks that the flutter#180022 patch was included in the 3.44.0 release.
I tried cherry-picking our Tizen patch on the 3.44.0 branch and cherry-picking this patch as well, but I encountered many conflicts. I am not sure if I can successfully perform the migration.
I can request that you migrate only this patch separately after the flutter-tizen 3.44 update.
| std::move(cache_directory))), | ||
| worker_task_runner_(std::move(worker_task_runner)) { | ||
| worker_task_runner_(std::move(worker_task_runner)), | ||
| compile_queue_(PipelineCompileQueueVulkan::Create(worker_task_runner)) { |
There was a problem hiding this comment.
…of waiting. (flutter#180022) Also, removes "lazy shader mode" and re-enables eager PSO pre-flight. Impeller eagerly sets up the entire set of pipeline state objects (PSOs) it may need during renderer setup. This works fine on mid to high-end devices as the setup completes well before the first frame is rendered. However on low-end devices, not all PSOs may be ready before the first frame is rendered. Low-end device usually don't have as much available concurrency and are slower to boot. On these devices, the rendering thread had to wait for the background compile job to be completed. It was also observed that the relatively higher priority render thread was waiting on a background thread to finish a PSO compile job. The PSO compile job could also be stuck behind another job that was not immediately needed. This made the situation on low end devices less than ideal. To ameliorate this situation, a stop-gap was introduced called "lazy shader mode". This disabled PSO pre-flight entirely and only dispatched jobs when they were needed. This ameliorated somewhat the issue of unneeded jobs blocking the eagerly needed job but the other issues remained. It also meant that one could expect jank the first time a new PSO was needed. This mode was not widely used or advertised. In this patch, the lazy shader mode has been entirely removed and eager PSO pre-flight is the default in all configurations. Pipeline compile jobs are now handled by a separate compile queue. This queue operates like a transparent concurrent worker pool job dispatcher in the usual case. However, when the pipeline for a specific descriptor is needed, instead of waiting for the job to complete, the queue allows the job for that descriptor to skip to the head of the queue and be performed on the calling thread. This effectively make the calling thread do the job instead of waiting. Another nice side effect of this behavior is that the higher priority thread is now doing the job it was previously waiting on. This fixes all issues on lower end devices except one. During eager pre-flight, it would still be better if PSO compile jobs for that were somehow divined to be needed first were towards the front of the pre-flight queue. After all, only a handful of rendering features account for the majority of rendering operations. A future augmentation to the compile queue would be to allow higher prioritization of more widely used pipeline compile jobs as instrumented in flutter#176660. This task is tracked in flutter#176665. This patch only wires up the compile queue for Vulkan. flutter#176657 tracks doing the same for OpenGL. The efficacy of the re-prioritizations can be traced using the PrioritiesElevated trace counter. On higher end devices, there should be zero of these. Fixes flutter#176656 Fixes flutter#176663 Fixes flutter#176658
Because IO thread is the owner of resource context, pipeline library gles can load resource by IO thread.
…inish before starting the next one.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request removes the lazy_shader_mode flag and introduces a PipelineCompileQueue architecture to manage asynchronous pipeline compilation for GLES and Vulkan backends. This system enables eager execution of pending compilation jobs when a specific pipeline is required for rendering. Feedback focuses on preventing potential stack overflows in the GLES compile queue during synchronous execution and simplifying the job-flushing logic in the base class to avoid redundant locking.
| void PipelineCompileQueueGLES::ProcessJobsSequentially() { | ||
| PostJob([weak_queue = weak_from_this()]() { | ||
| if (auto queue = std::static_pointer_cast<PipelineCompileQueueGLES>( | ||
| weak_queue.lock())) { | ||
| queue->DoOneJob(); | ||
| { | ||
| Lock lock(queue->processing_mutex_); | ||
| if (!queue->HasPendingJobs()) { | ||
| queue->is_processing_ = false; | ||
| return; | ||
| } | ||
| } | ||
| queue->ProcessJobsSequentially(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
If worker_task_runner_ is null (which can happen in certain test or playground environments), PostJob executes the closure synchronously. This leads to ProcessJobsSequentially calling itself recursively through the closure until the queue is empty. For a large number of pipelines, this could potentially cause a stack overflow. It is safer to handle the synchronous case with a loop.
void PipelineCompileQueueGLES::ProcessJobsSequentially() {
if (!worker_task_runner_) {
while (auto job = TakeNextJob()) {
job();
}
Lock lock(processing_mutex_);
is_processing_ = false;
return;
}
PostJob([weak_queue = weak_from_this()]() {
if (auto queue = std::static_pointer_cast<PipelineCompileQueueGLES>(
weak_queue.lock())) {
queue->DoOneJob();
{
Lock lock(queue->processing_mutex_);
if (!queue->HasPendingJobs()) {
queue->is_processing_ = false;
return;
}
}
queue->ProcessJobsSequentially();
}
});
}
There was a problem hiding this comment.
Code Review
This pull request removes the lazy_shader_mode flag and introduces a PipelineCompileQueue system to manage asynchronous pipeline compilation across GLES and Vulkan backends. This system enables eager execution of pending compilation jobs when a pipeline is requested before background processing finishes. Review feedback identifies a potential stack overflow in the GLES sequential processing logic when running without a task runner and recommends a null check for the task runner in the Vulkan backend to avoid potential crashes.
| void PipelineCompileQueueGLES::ProcessJobsSequentially() { | ||
| PostJob([weak_queue = weak_from_this()]() { | ||
| if (auto queue = std::static_pointer_cast<PipelineCompileQueueGLES>( | ||
| weak_queue.lock())) { | ||
| queue->DoOneJob(); | ||
| { | ||
| Lock lock(queue->processing_mutex_); | ||
| if (!queue->HasPendingJobs()) { | ||
| queue->is_processing_ = false; | ||
| return; | ||
| } | ||
| } | ||
| queue->ProcessJobsSequentially(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current implementation of ProcessJobsSequentially is recursive when worker_task_runner_ is null (e.g., in some test environments). If there are many pending jobs, this could lead to a stack overflow. Consider refactoring this to use a loop if the task is executed synchronously, or ensure that the recursion depth is limited.
Hello |
1.Change is_processing type to bool. 2.Change DoOneJob,PostJob,HasPendingJobs to protect.
1.Extrac a new method OnJobAdded 2.Replace std::mutex with Mutex 3.Add null check for io task runner 4.Change mthod ProcessJobsSequentially to private
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request removes the lazy_shader_mode flag and introduces a PipelineCompileQueue architecture to manage asynchronous pipeline state object (PSO) compilation for GLES and Vulkan backends. The implementation includes base and backend-specific queue classes that support background compilation and eager execution for high-priority requests. Review feedback identifies a potential deadlock in the GLES synchronous execution path, incorrect smart pointer usage for task runners in the Vulkan implementation, and opportunities to improve task scheduling predictability and efficiency by using FIFO ordering and avoiding redundant duplicate job executions.
1.Remove not used code. 2.Add comments for io task runner. 3.Remove io task runner from pipeline library gles.
3 participants
Impeller eagerly sets up the entire set of pipeline state objects (PSOs) it may need during renderer setup. This works fine on mid to high-end devices as the setup completes well before the first frame is rendered.
However on low-end devices, not all PSOs may be ready before the first frame is rendered. Low-end device usually don't have as much available concurrency and are slower to boot. On these devices, the rendering thread had to wait for the background compile job to be completed. It was also observed that the relatively higher priority render thread was waiting on a background thread to finish a PSO compile job. The PSO compile job could also be stuck behind another job that was not immediately needed. This made the situation on low end devices less than ideal.
flutter#180022, this PR fix PSO stuck issue for Vulkan on low end devices.
Current PR can fix the PSO stuck issue for GLES on low end devices. Related issue : flutter#176657