Repository navigation
[Impeller]Use the IO context for OpenGL program setup - #185723
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
Bot
added
engine
![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 introduces backend-specific pipeline compile queues for GLES and Vulkan in Impeller and refactors the base PipelineCompileQueue to support deferred job execution. Key changes include updating ContextGLES, PipelineLibraryGLES, and the embedder surface to use an IO task runner for asynchronous pipeline creation, alongside adding a flushing mechanism in Shell::OnPlatformViewCreated. Feedback identifies a potential hang if the wait_until_rendering_ flag is not reset during flushing, a data race requiring std::atomic<bool>, type inconsistencies in the Vulkan implementation, and a potential crash in the GLES implementation if the task runner is null.
There was a problem hiding this comment.
Code Review
This pull request refactors the PipelineCompileQueue into a base class with backend-specific implementations for GLES and Vulkan, enabling deferred pipeline compilation. It introduces an io_task_runner to the GLES backend and implements a mechanism in the shell to flush pending compilation jobs when a platform view is created. Feedback includes suggestions to simplify the logic in the flush_task lambda to reduce redundant calls and to correct a descriptive comment regarding the task runner used for flushing.
xiaowei-guan
changed the title
25d2282 to
8dac629
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors PipelineCompileQueue into a base class and introduces backend-specific subclasses, PipelineCompileQueueGLES and PipelineCompileQueueVulkan, to handle sequential and parallel scheduling strategies respectively. The GLES and Vulkan backend pipeline libraries, contexts, and embedder surfaces are updated to integrate these queues, and corresponding unit tests are added. Feedback on the changes suggests adding a null check for the worker task runner in PipelineCompileQueueVulkan::Create to prevent potential null pointer dereferences, and removing the sorting step in the GLES sequential processing unit test to ensure the execution order is correctly verified.
|
@flutter-dashboard I have added test |
There was a problem hiding this comment.
Code Review
This pull request refactors PipelineCompileQueue into a base class and introduces backend-specific subclasses: PipelineCompileQueueGLES for sequential scheduling and PipelineCompileQueueVulkan for parallel scheduling. It updates the GLES and Vulkan pipeline libraries, context creation, and embedder surfaces to integrate these queues. Feedback suggests adding a null check for worker_task_runner_ in the Vulkan compile queue to prevent null pointer dereferences, and removing std::sort from the GLES sequential execution unit test to ensure it correctly validates execution order.
github-actions
Bot
added
platform-android
|
@flutter-dashboard |
There was a problem hiding this comment.
Code Review
This pull request refactors PipelineCompileQueue into an abstract base class and introduces backend-specific implementations, PipelineCompileQueueGLES and PipelineCompileQueueVulkan, along with corresponding unit tests. It also updates GLES context creation across various platforms to accept and propagate an IO task runner. The review feedback suggests reducing lock contention in PipelineCompileQueueGLES::OnJobAdded by not calling DrainPendingJobs while holding a mutex, enforcing the factory pattern by making the constructors of both specialized compile queues private or protected, and adding a null check for the task runner in PipelineCompileQueueVulkan::Create.
|
@gaaclarke can you find an engine team reviewer? |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
@xiaowei-guan can you add information in your description to describe why we'd want to do this? That isn't outlined in the issue or the PR description currently.
I suspect Chinmay was hoping this could parallelize and speed up startup time. I suspect the driver is serializing the program setup anyways. Did you measure any difference with this PR?
…efine an empty destructor instead of {}
…tation on declarations only.
…m_view_android.cc Change the io_task_runner parameter in CreateAndroidContext function from pass-by-value to pass-by-const-reference to avoid unnecessary copying of fml::RefPtr<fml::TaskRunner>. This resolves the -Wperformance-unnecessary-value-param warning that was being treated as error during clang-tidy checks.
… the Shell destructor shuts down the IO thread Without this, PipelineCompileQueueGLES has a race condition during Shell shutdown. If a shader compilation task queued to the IO task runner by PipelineCompileQueueGLES runs after the Shell destructor completes IO thread shutdown, then the task may fail when executing GLES operations. This change adds a ConditionalBasicTaskRunner class that wraps a task runner and holds a function that indicates whether the task runner is valid. Posted tasks will not execute if the function returns false. The Shell uses this to provide a GetShutdownSafeIOTaskRunner API that returns a BasicTaskRunner tied to the state of the IO thread. If the Shell destructor has run the IO thread cleanup task and destroyed the ShellIOManager, then any future tasks queued on this task runner will not run.
…EGL context With the introduction of PipelineCompileQueueGLES, some of the embedder tests were crashing during shutdown. This happened because ANGLE worker threads were still doing pipeline initialization when the process exited. The TestEGLContext calls eglTerminate when the test exits, which should stop all activity in ANGLE. But ANGLE's EGL implementation did not wait for the worker threads to stop because some of the test's threads still had a current EGL context when they exited. This change adds a thread local object to TestEGLContext that releases EGL state. If a thread uses TestEGLContext to call eglMakeCurrent, then TestEGLContext will ensure that the state is cleared when the thread terminates.
…used an EGL context" This reverts commit e54f3d4. This was causing deadlocks during thread shutdown on Windows. The eglReleaseThread call made by the TestGLSurface thread shutdown handler would cause ANGLE to wait for its CleanUpThread to shut down. CleanUpThread::mTaskThread is a std::thread, and its shutdown calls the Windows FreeLibraryAndExitThread API. That API then deadlocks because the Flutter-owned thread is in a thread shutdown handler.
…utdown in the embedder library * Add an implementation of ReleaseResourceContext to EmbedderSurfaceGLImpeller. ReleaseResourceContext is called during IO thread shutdown in the Shell destructor to clear the EGL context on that thread. * Clear the EGL context on the raster thread during rasterizer teardown
PipelineCompileQueue::FinishAllJobs ensures that all remaining jobs will be executed or posted to a worker thread when the queue is destructed. But FinishAllJobs does not actually wait for jobs on the worker thread to complete.
|
@walley892 I think you may have to be the 2nd reviewer since jason has contributed? PTAL |
8 participants
Fix #176657
This PR can improve app startup time for impeller+GLES on low-end devices.
#180022 has provided solution for impeller + vulkan.
Impeller+GLES has a special case: resource loading cannot be arbitrarily placed on other threads; it needs to be done on the I/O thread because this thread shares the context with the GPU thread. Therefore, this PR introduces the I/O task runner into PipelineCompileQueueGLES. However, since the I/O task runner may handle other tasks, we cannot put all jobs into the I/O thread at once. My current solution is to wait for the previous job to finish before putting the next one into the I/O thread.
Main changes:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.