Repository navigation
docs(android): record the measured field cost of the Impeller switch - #7794
Conversation
This comment has been minimized.
This comment has been minimized.
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
went through every number against the third reading in #7123 and they all match, and the shell.cc quote is exact (line 529, and it is there in both 3.44.0 and 3.44.9). manifest still parses and the element tree is byte-identical to main, so nothing ships.
one thing to fix first: line 110 of this same block still says 3.44.0 is "the same pin this branch builds on", but #7202 moved mise.toml and codemagic to 3.44.9 on 18 Aug and this branch sits on top of it. details inline, plus a small nit on the engine-logs wording.
| (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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
This comment has been minimized.
This comment has been minimized.
fc657f8 to
2508825
Compare
This comment has been minimized.
This comment has been minimized.
The manifest justifies re-entering Impeller as a traded risk but never says what the trade actually cost, so the next reader has no way to judge it except by re-running the analysis. Field measurement across the 1.0.17 -> 1.0.19 boundary puts it at +2.9 points of slow frames (95% CI [+0.5, +5.3]), with iOS flat under the same estimator. Also records why reverting is not the answer: the engine already logs the opt-out as deprecated and slated for removal, so the feed has to be fast on Impeller regardless of what the chroma-key preview is worth.
The block said the Skia/Impeller profiling was taken on 3.44.0, "the same pin this branch builds on". #7202 moved mise.toml, codemagic and every workflow to 3.44.9 on 2026-08-18, before this branch was cut, so that clause was false and the surrounding "not a re-evaluation after an engine upgrade" argument read stronger than the facts support. States the pin move instead, and that the profiling predates it. Both cited upstream issues are still closed, so the rest of the paragraph stands. Comment-only: the comment-stripped element tree is unchanged.
2508825 to
78a8210
Compare
Mobile PR PreviewPreview refreshed for Last refresh:
|
3 participants
Description
The Impeller re-entry in #6483 is documented in
AndroidManifest.xmlas a deliberate trade — the video editor's live chroma-key preview needsui.ImageFilter.shader, which Skia does not implement — but the comment never says what the trade cost. #7123 measured it, twice, with the estimate moving both times. This writes the settled numbers into the manifest so the next reader can judge the trade without re-running the analysis, and records why a revert is not the answer.What the field data says, across the 1.0.17 (Skia) → 1.0.19 (Impeller) boundary, store builds only,
_st_MainActivityscreen traces, device fixed effects with session weighting:Three things changed versus the numbers in the issue, all from weighting sessions rather than averaging per-device ratios unweighted — which gives a device with 5 sessions the same say as one with 805:
iOS crossing the same boundary with the same shared Dart is flat under the identical estimator, which is what pins the remainder on the rendering backend rather than on anything in the release.
Why reverting is off the table, and the reason this closes as documentation rather than a code change: the pinned Flutter 3.44.9 engine already logs the opt-out as deprecated and says the flag and the
EnableImpellermanifest entry "are going to go away in an upcoming Flutter release" (engine/src/flutter/shell/common/shell.cc:529). A revert buys a release or two and then the feed has to be fast on Impeller anyway — so the answer to those 2.9 points is raster work on its own merits, not a backend flip, whatever the chroma-key preview turns out to be worth.Related Issue: Closes #7123
Out of Scope
The raster work itself. The remaining Impeller-specific cost in the feed is most likely the
BackdropFilterpasses that sit over the live video texture (center_playback_control.dart,feed_playback_toggles_pill.dart,content_warning_helpers.dart) — the action-button shadow blurs are alreadyRepaintBoundary-isolated and the fullscreen backdrop blur was replaced by the blurhash path in #5957. Confirming that needs on-device profiling and belongs in its own change.Verification
Comment-only change to
AndroidManifest.xml; no Dart, no resources, no manifest elements touched. Parsed the file to confirm it is still well-formed XML. Numbers re-derived fromopenvine-co.firebase_performance.co_openvine_app_{ANDROID,IOS}over the full export through 2026-08-16 (1.0.19 store build 692 now has 6,240 screen traces, up from 1,338 when the issue's second reading was written).Type of Change