[Impeller] Share OpenGL context between playground unittests by flar · Pull Request #187690 · flutter/flutter

flar · 2026-06-08T19:15:31Z

A full run of the Impeller unit tests takes 25 minutes - the vast majority of which is constructing a new OpenGL context on each playground unittest. This PR shares an OpenGL context amongst all of the OpenGL playground tests and reduces the runtime of the impeller unit tests from 25 minutes down to 2:20 minutes.

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-assist bot 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.

gemini-code-assist

Code Review

This pull request introduces a shared OpenGL ES playground context across tests to improve performance and resource management, and adds a ContentContextHolder to manage the lifecycle and cleanup of ContentContext resources. Feedback on these changes highlights a critical issue where the shared OpenGL context is prematurely shut down after the first test, which can be resolved by conditionally calling Shutdown() only when the context is owned. Additionally, the reviewer recommends removing leftover debugging logs and commented-out code, and warns against using static cleanup closures for context shutdown due to potential exit-time crashes from undefined static destruction order.

gemini-code-assist · 2026-06-08T19:17:46Z

+    static fml::ScopedCleanupClosure context_cleanup(
+        [&] { (*opengl_playground)->GetContext()->Shutdown(); });


Using a static fml::ScopedCleanupClosure to call Shutdown() on the shared context at program exit is highly risky.

During static destruction (after main returns), the order of destruction of global/static variables across different translation units is undefined. If GLFW or the graphics driver has already been terminated or unloaded, calling Shutdown() here can cause a segmentation fault or crash on exit.

Since absl::NoDestructor is used specifically to prevent exit-time destruction issues, we should avoid manually triggering the shutdown of GPU resources at exit. The OS will reclaim all resources when the process terminates.

This is the same approach used in the golden tests.

flar · 2026-06-08T19:48:20Z

The cost of running a full impeller_unittests on my M1 Macbook Pro:

Before sharing GL contexts

% ( cd out/host_debug_unopt_arm64; ./impeller_unittests; )
REAL	24:52.56
user	27:43.16
sys	37.903

After sharing GL contexts

% ( cd out/host_debug_unopt_arm64; ./impeller_unittests; )
REAL    2:19.30
user    2:09.26
sys    14.297

…ttests-share-GL-context

flar · 2026-06-09T07:14:07Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces lifetime management improvements for ContentContext in Impeller's playground tests, flushes command buffers across various unit tests, and refactors GLES backend initialization in the playground using absl::NoDestructor. A critical issue was identified in playground.cc where missing break statements in the GLES backend switch cases cause an unintended fallthrough bug.

flar · 2026-06-09T07:24:39Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces lifetime management improvements for Impeller's playground and testing contexts, notably adding a ContentContextHolder to clean up transient buffers and caches, and flushing command buffers in various unit tests. It also refactors playground setup to use static instances for OpenGL backends. Feedback highlights critical issues in playground.cc where static cleanup closures capture local variables by reference, leading to potential dangling references, undefined behavior, and null pointer dereferences on exit.

gemini-code-assist · 2026-06-09T07:26:13Z

+      static fml::ScopedCleanupClosure context_cleanup(
+          [&] { (*opengl_playground)->GetContext()->Shutdown(); });


Using a static fml::ScopedCleanupClosure with a lambda capturing by reference ([&]) introduces two major issues:

Dangling References / Undefined Behavior: The lambda captures this and other local variables (like switches) by reference. Since these objects are destroyed long before the static destructor runs at program exit, executing this lambda results in undefined behavior.

Null Pointer Dereference: If PlaygroundImpl::Create fails and returns nullptr, *opengl_playground will be nullptr. Dereferencing it during cleanup will cause a crash on exit.

To fix this, use an empty capture list [] (since opengl_playground is static and doesn't need to be captured) and add a null check before dereferencing.

Suggested change

static fml::ScopedCleanupClosure context_cleanup(

[&] { (*opengl_playground)->GetContext()->Shutdown(); });

static fml::ScopedCleanupClosure context_cleanup(

[] {

if (*opengl_playground) {

(*opengl_playground)->GetContext()->Shutdown();

}

});

References

The Google C++ Style Guide discourages static variables with non-trivial destructors because the order of destruction is undefined and can lead to crashes on exit. Capturing local variables by reference in a static lambda exacerbates this by creating dangling references. ^(link)

gemini-code-assist · 2026-06-09T07:26:13Z

+      static fml::ScopedCleanupClosure context_cleanup(
+          [&] { (*opengl_playground)->GetContext()->Shutdown(); });


Using a static fml::ScopedCleanupClosure with a lambda capturing by reference ([&]) introduces two major issues:

Dangling References / Undefined Behavior: The lambda captures this and other local variables (like switches) by reference. Since these objects are destroyed long before the static destructor runs at program exit, executing this lambda results in undefined behavior.

Null Pointer Dereference: If PlaygroundImpl::Create fails and returns nullptr, *opengl_playground will be nullptr. Dereferencing it during cleanup will cause a crash on exit.

To fix this, use an empty capture list [] (since opengl_playground is static and doesn't need to be captured) and add a null check before dereferencing.

static fml::ScopedCleanupClosure context_cleanup( [] { if (*opengl_playground) { (*opengl_playground)->GetContext()->Shutdown(); } });

References

The Google C++ Style Guide discourages static variables with non-trivial destructors because the order of destruction is undefined and can lead to crashes on exit. Capturing local variables by reference in a static lambda exacerbates this by creating dangling references. ^(link)

flar · 2026-06-15T22:11:57Z

This PR is nearly ready to go except for one minor issue that I can't find a workaround for.

Just running the unit tests, as in ./impeller_unittests runs really quickly to completion. That part works great.

But, running a bunch of unit tests with the playground, as in ./impeller_unittests --enable_playground [--gtest_filter=...] gives you one chance to hit the Escape key on an OpenGL test and then every OpenGL test after that will appear and disappear immediately as if you'd already hit the Escape key. This is because the Escape key handler is implemented by marking the window "Should Close" and that flag survives from one test to another because we are no longer creating a new window. My attempts to reset that flag and/or create a new window on every playground ran into threading issues. This is the last issue keeping this PR from being a final solution.

…lutter#188080) Currently running all of the impeller unit tests can take 25 minutes most all of which is caused by recreating an OpenGL context. While flutter#187690 attempts to share that context among all playground implementations to amortize that cost, it is running into issues with interactions between the reused context and the ImGui code. In order to further focus the sharing, this PR attempts to share just the ContextGLES and the ReactorWorker between instantiations of the PlaygroundImplGES. Fixes flutter#188649 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. --------- Co-authored-by: Benson Luk <[email protected]>

flar · 2026-07-01T22:02:48Z

Replaced by #188080

[Impeller] Share OpenGL context between playground unittests

639e454

flutter-dashboard Bot added the CICD Run CI/CD label Jun 8, 2026

github-actions Bot added a: text input Entering text in a text field or keyboard related problems engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Jun 8, 2026

gemini-code-assist Bot reviewed Jun 8, 2026

View reviewed changes

flar added 2 commits June 8, 2026 12:22

remove dead code an unnecessary log messages

bd776d3

shut down context conditionally only on owned contexts

ac38831

github-actions Bot removed the CICD Run CI/CD label Jun 8, 2026

flar added the CICD Run CI/CD label Jun 8, 2026

fix all but one of the failing unit tests

b505291

github-actions Bot removed the CICD Run CI/CD label Jun 8, 2026

flar added the CICD Run CI/CD label Jun 8, 2026

Merge branch 'master' of github.com:flutter/flutter into impeller-uni…

6eb2bd1

…ttests-share-GL-context

github-actions Bot removed the CICD Run CI/CD label Jun 8, 2026

flar added the CICD Run CI/CD label Jun 8, 2026

more descriptive ProcAddress failure message

0a09260

github-actions Bot removed the CICD Run CI/CD label Jun 9, 2026

flar added the CICD Run CI/CD label Jun 9, 2026

potentially more Linux friendly PlaygroundImpl creation

614e35e

github-actions Bot removed the CICD Run CI/CD label Jun 9, 2026

flar added the CICD Run CI/CD label Jun 9, 2026

gemini-code-assist Bot reviewed Jun 9, 2026

View reviewed changes

Comment thread engine/src/flutter/impeller/playground/playground.cc

missing break statements

2fb23c5

github-actions Bot removed the CICD Run CI/CD label Jun 9, 2026

flar added the CICD Run CI/CD label Jun 9, 2026

gemini-code-assist Bot reviewed Jun 9, 2026

View reviewed changes

flar mentioned this pull request Jun 15, 2026

[Impeller] Share a pipeline library among GLES contexts #188039

Closed

10 tasks

flar mentioned this pull request Jun 16, 2026

[Impeller] Share a single ContextGLES among all PlaygroundImplGLES #188080

Merged

10 tasks

LongCatIsLooong removed the a: text input Entering text in a text field or keyboard related problems label Jun 25, 2026

flar closed this Jul 1, 2026

		static fml::ScopedCleanupClosure context_cleanup(
		[&] { (*opengl_playground)->GetContext()->Shutdown(); });

Uh oh!

Conversation

flar commented Jun 8, 2026

Pre-launch Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

flar Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar commented Jun 8, 2026

Uh oh!

flar commented Jun 9, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

flar commented Jun 9, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

flar commented Jun 15, 2026

Uh oh!

flar commented Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

flar Jun 8, 2026 •

edited

Loading