Skip to content

docs(android): record the measured field cost of the Impeller switch #7794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hm21 merged 2 commits into from
Aug 19, 2026
Merged

docs(android): record the measured field cost of the Impeller switch #7794

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6a52ef
docs(android): record the measured field cost of the Impeller switch
hm21 Aug 18, 2026
78a8210
docs(android): correct the stale Flutter pin in the Impeller comment
realmeylisdev Aug 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions mobile/android/app/src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,39 @@
- Skia: 13-16ms raster, 60 FPS, correct video rendering.

Nothing upstream has changed since. That measurement was
taken on Flutter 3.44.0, the same pin this branch builds on,
with both cited issues already closed - so this is not a
re-evaluation after an engine upgrade. What changed is the
taken on Flutter 3.44.0, with both cited issues already
closed - so this is not a re-evaluation after an engine
upgrade. The Flutter pin has since moved to 3.44.9 (#7202),
which the profiling above predates. What changed is the
requirement: the video editor's live chroma-key preview
needs `ui.ImageFilter.shader`, which Skia does not implement
at all. On Skia the preview degrades to the unkeyed video
(the exported file is unaffected either way), and we decided
a working preview is worth re-entering Impeller.

What the trade cost, measured in the field (#7123): across

@realmeylisdev realmeylisdev Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nine lines up, on the pre-existing text: taken on Flutter 3.44.0, the same pin this branch builds on. That is not true on this branch any more.

#7202 (2504f4d871, merged 2026-08-18) moved the pin to 3.44.9 in mobile/mise.toml, codemagic.yaml and every workflow, and git merge-base --is-ancestor 2504f4d871 22fd2320 passes, so this branch builds on 3.44.9. mobile/docs/SHOREBIRD_CODE_PUSH.md on this same branch already says so: "The current 3.44.9 pin is deliberate ... the repo and all CI workflows moved together to the refreshed 3.44.9 Shorebird engine."

It also undercuts the sentence it sits in. "Nothing upstream has changed since ... so this is not a re-evaluation after an engine upgrade" is there to argue the Skia/Impeller profiling is still current, and the engine did move after that profiling was taken. Not a routine bump either: 3.44.0 was dropped because its Shorebird engine crashed at launch in the cupertino_http async FFI path.

Since this PR is the one turning the block into a record someone can read cold, better to correct it here than leave the next reader to trip on it. Naming 3.44.0 as the version the profiling ran on, and 3.44.9 as what the branch builds on now, would cover it.

The shell.cc quote is unaffected either way, the warning sits at line 529 in both versions.

the 1.0.17 Skia -> 1.0.19 Impeller boundary, store builds
only, `_st_MainActivity` screen traces with device fixed
effects and session weighting, Android slow frames rose
2.9 points (95% CI [+0.5, +5.3], p=0.019) - about one frame
in thirty-five. iOS across the same boundary under the same
estimator is the control at +0.29 points (CI [-0.01,
+0.59]), which is what pins this on the backend rather than
the shared Dart. Frozen frames did not move (+0.23 points,
p=0.18), and the cold-start regression the issue was opened
on faded to noise as the sample grew. Larger figures were
reported for all three before sessions were weighted:
averaging per-device ratios unweighted gives a device with
five sessions the same say as one with eight hundred.

Reverting is a stopgap, not a fix. The engine logs

@realmeylisdev realmeylisdev Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this reads present-tense about the app, but there is no EnableImpeller entry in this manifest, so the warning never fires here. The tail of this same block already says as much ("which the engine only logs when Impeller is actually off"), and the engine guard is if (!settings.enable_impeller && settings.warn_on_impeller_opt_out).

Same for "this manifest entry" - the entry being deprecated is not in this file.

Something like "reverting would trip the engine's [Action Required] warning" keeps the point and drops the ambiguity.

@realmeylisdev realmeylisdev Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Withdrawing this one, I got it wrong. mobile/android/app/src/debug/AndroidManifest.xml:23-25 does set EnableImpeller to false, so every debug build really does trip the warning. "The engine logs" is literally true today and the present tense is fine. My "never fires here" was read off this file alone without checking the sibling source set.

The "this manifest entry" wording is the only bit I'd still glance at, and it is minor: the entry does exist, just in debug/ rather than this file. Not worth a change on its own.

Separately, and only in the PR description, not in the diff: "Three things changed versus the numbers in the issue, all from weighting sessions" does not hold for the third bullet. The cold-start move is the one the second reading attributes to the shared-device set growing 33 to 48, and #7123 carries no session-weighted cold-start estimate at all, only slow and frozen frames were re-run under FE-OLS. The manifest text gets this right ("faded to noise as the sample grew"), so it is just the description.

"[Action Required]: Impeller opt-out deprecated" and says
the flag and this manifest entry "are going to go away in
an upcoming Flutter release" (shell/common/shell.cc). The
feed has to be fast on Impeller before that removal lands,
whatever the chroma-key preview is worth, so the answer to
those 2.9 points is raster work, not a backend flip.

Two things that follow, both easy to get wrong:

- "Falls back to Skia" is not what happens. Below API 29 the
Expand Down
Loading