Repository navigation
[Impeller] Minify GLSL shaders - #195240
Conversation
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 a GLSL minifier to optimize SPIRV-Cross GLSL output for OpenGL target platforms in Impeller, along with corresponding unit tests. Feedback on the implementation suggests several key improvements: handling the 'f' and 'F' float literal suffixes to ensure they are correctly shortened, replacing locale-dependent checks (std::isalnum and std::isdigit) with locale-independent ASCII checks to prevent parsing bugs, defensively checking the return status of std::to_chars, and preserving newlines between minified lines to maintain shader debuggability and prevent driver issues with extremely long lines.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
This seems like a liability Zach, are we sure it's worth the 20k in compressed size? Uncompressed isn't something we really care about. I wonder if there is something simpler that gets us the majority of the win? Like stripping comments?
We shouldn't be doing this for debug builds, that's for sure since it obfuscates peoples ability to debug shaders while they are being authored.
|
Yeah, that's a good question. Native .so's are only compressed in-transit. The in-memory and on-disk cost is the full 190KB, which is the motivation for doing this. There was already code for stripping comments in the compiler, but not sure if it was effective or not. I think the best way to simplify this change would be to remove the local variable renaming. That would keep most of the benefit and removing whitespace and re-writing floats is more obviously correct. |
jtmcdole
left a comment
jtmcdole
left a comment
There was a problem hiding this comment.
I like the direction, but I yield to Aaron.
| namespace impeller { | ||
| namespace compiler { | ||
|
|
||
| std::string StripGLSLComments(std::string_view source) { |
There was a problem hiding this comment.
Sometimes, I wish the compiler was written in dart...
There was a problem hiding this comment.
Yeah, FFI would have to get to the point where a Dart class can inherit from a C++ class because the spriv_cross code is all C++, and some of the compiler is implemented by subclassing its classes.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
You know what, this glsl is the output of shaderc so there probably isn't a lot of comments already.
For the sake of giving you a clear path forward, let's remove the name mangling for now and we can measure the benefit after this lands and decide if we want to do that as a followup. Let's also only do this for release engines.
|
Oh that reminds me, we are using shaderc or spirv-cross to generate the glsl code. I can't remember which. But they may already have a flag that does this minification for us already if it is generating glsl. We should pull on that thread. |
I'll take a look. |
0757678 to
5994969
Compare
zanderso
left a comment
zanderso
left a comment
There was a problem hiding this comment.
There's no 'minification' option/flag in spriv_cross, but I found a simpler way to implement the float minification by subclassing spirv_cross::CompilerGLSL. The whitespace removal is simpler as-is in this PR.
I removed the variable name shortening and that added back ~30KB or so.
| /// Registers a custom `spirv_cross::FloatFormatter` backed by `std::to_chars` | ||
| /// so floating-point literals are emitted in their shortest round-tripping | ||
| /// decimal representation instead of SPIRV-Cross's default `"%.32g"` format. | ||
| class MinifyingCompilerGLSL final : public spirv_cross::CompilerGLSL { |
There was a problem hiding this comment.
Subclassing CompilerGLSL allows overriding the methods that emit floats so that they're just emitted using the shortest representation in the first place.
To get another ~30KB of reduction, it's also possible to override methods in here to get shorter variable names.
5994969 to
f8eab92
Compare
|
@zanderso friendly reminder, when you want another review make sure to press the "re-request review" button. looks like you may be close to that point |
|
Thanks for the reminder! I was waiting for the presubs to be all green before re-requesting =) |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
f8eab92 to
24d297c
Compare
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Mostly looking good. There is one potential gotcha in the float transformation logic. We should do a followup to apply this to user defined shaders too.
24d297c to
e611b68
Compare
zanderso
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
flutter/flutter@c091098...5e805bf 2026-10-30 [email protected] Fix/shared thread merger expired lease (flutter/flutter#194200) 2026-10-30 [email protected] [FGP] Report the real error when applying kotlin-android fails (flutter/flutter#195080) 2026-10-30 [email protected] [Impeller] Minify GLSL shaders (flutter/flutter#195240) 2026-10-30 [email protected] Roll Packages from 0b72e92 to 3742a4e (12 revisions) (flutter/flutter#195686) 2026-10-30 [email protected] Roll Dart SDK from 52c908c00267 to fe609902984b (2 revisions) (flutter/flutter#195679) 2026-10-30 [email protected] Roll Skia from 0789f2ab93b8 to c0c46ee9f938 (1 revision) (flutter/flutter#195671) 2026-10-30 [email protected] Roll Fuchsia Linux SDK from SORPfa9tgFn83M1CR... to x2SqGbVUkOm4d6vk_... (flutter/flutter#195670) 2026-10-30 [email protected] Roll Skia from 4470b8abaad6 to 0789f2ab93b8 (2 revisions) (flutter/flutter#195665) 2026-10-30 [email protected] Roll Dart SDK from 89f7a35480d6 to 52c908c00267 (1 revision) (flutter/flutter#195659) 2026-10-30 [email protected] Roll Skia from 9f4e3117465e to 4470b8abaad6 (4 revisions) (flutter/flutter#195658) 2026-10-30 [email protected] Roll Skia from ec13cd547e98 to 9f4e3117465e (1 revision) (flutter/flutter#195650) 2026-10-30 [email protected] [web] Keep semantics editing state while the text input connection is open (flutter/flutter#195141) 2026-10-30 [email protected] [web_paragraph] Fix multi-span BiDi span order and region direction (flutter/flutter#194980) 2026-10-30 [email protected] [web] Unskip web tests that pass on CanvasKit and Skwasm: accessibility_inspector_test, color_filter_test (flutter/flutter#195541) 2026-10-30 [email protected] Roll Skia from 4ecd22a4a1d0 to ec13cd547e98 (10 revisions) (flutter/flutter#195647) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
For the GLSL shaders that get baked into the engine as textual source, this PR: - Removes comments and whitespace, - Removes needless precision from float literals This shrinks the GLSL shaders bundled into the engine by ~150KB, reducing the uncompressed size of libflutter.so by roughly that amount. This impacts the on-disk and in-memory size of the engine, but only reduces the in-transit apk by under 20KB since the text compresses well. Later, we can decide whether to also shorten variable names, which would save an additional ~40KB. ## 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 in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/CONTRIBUTING.ai.md [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
For the GLSL shaders that get baked into the engine as textual source, this PR:
This shrinks the GLSL shaders bundled into the engine by ~150KB, reducing the uncompressed size of libflutter.so by roughly that amount. This impacts the on-disk and in-memory size of the engine, but only reduces the in-transit apk by under 20KB since the text compresses well.
Later, we can decide whether to also shorten variable names, which would save an additional ~40KB.
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.