Repository navigation
perf(images): size the image cache by RAM and free GPU memory after clearing it - #10308
Merged
Merged
Conversation
…ring it A full 256 MB decoded-image cache costs about 500 MB of graphics memory under Impeller (#10301), on every device. The budget is now RAM / 48, clamped to 96-256 MB (12 GB: 256, 8 GB: 170, 6 GB: 128), read once at startup through a new divine/device_memory channel, with 128 MB when RAM is unknown. Clearing the cache only drops references: Impeller frees the textures when it next draws a frame, and a static screen draws none. Every explicit clear now schedules a frame. On Android this also makes the trim-memory clear on backgrounding return the memory, because it lands while the app is still inactive.
hm21
marked this pull request as ready for review
October 19, 2026 10:15
hm21
requested review from
rabble and
realmeylisdev
and removed request for
a team
October 19, 2026 10:15
roborev: Combined Review (
|
This comment has been minimized.
This comment has been minimized.
…nored The test fed a 5.6 GiB reading, whose budget (6 GiB / 48 = 128 MiB) is exactly the unknown-RAM fallback, so a configureImageCacheBudget that ignored the reading still passed. Use the 8 GB reading instead: its budget is neither the fallback nor a clamp bound.
The first clearDecodedImageCache() call already scheduled a frame and nothing drew it, so the hasScheduledFrame check after the includeLiveImages: true call passed whether or not that call scheduled one. Draw the first frame before the second clear. That call is the only form production uses.
clearDecodedImageCache() uses scheduleFrame() rather than scheduleForcedFrame() so a clear while the app is paused draws nothing until it returns, as its dartdoc and the PR description say. No test pinned that: with scheduleForcedFrame() every existing test stayed green. Clear while paused and expect no scheduled frame.
Every caller in the suite fakes it (openVineImageCacheClearProvider is overridden with a no-op, the age-verification test injects a counter), so dropping includeLiveImages: true, the frame, or the disk clear kept every test green. includeLiveImages is what stops revoked adult media from being served again from a live image. The function reads the global disk cache, and the tests keep that cache out of the shared test isolate (see debugImageCacheOverride): its JSON index debounces writes on a 3 s timer that would outlive the test. So it takes an optional @VisibleForTesting disk cache. Production calls, including the Future<void> Function() tear-off in social_providers, are unchanged.
The bootstrap call is now the only place the RAM-sized budget takes effect, and nothing ran it: configureImageCacheBudget's tests build their own ImageCache, and no test runs startOpenVineApp. Removing the call, pointing it at another cache or passing a reader that returns null kept every test green while every device fell back to a fixed budget. Follows error_widget_builder_wiring_test.dart, which pins a startup wiring line the same way for the same reason (#8647).
The PR's measured background release (625-632 MB of graphics memory on main against 254-270 MB) comes from this one callback, and no test ran it: memory_pressure_handler_test injects its own clearImageCache, and no test mounts DivineApp. Reverting it to the old clear()..clearLiveImages() kept every test green.
…t a frame The dartdoc said ImageCache.clear() only drops references and Impeller frees the textures on the next frame. In the pinned SDK, clear() and clearLiveImages() dispose each entry, and the entry releases its image handle in a SchedulerBinding post-frame callback (painting/image_cache.dart:601-611). So no frame means nothing is freed on any renderer, not only Impeller. That matters for anyone deciding whether a bare clear() is safe on Skia, desktop or the web.
…easured on The dartdoc said that on Android the memory-pressure clear on backgrounding arrives while the app is still inactive, so its frame is drawn. That ordering was measured on one Galaxy S26. Android sends TRIM_MEMORY_UI_HIDDEN when the process stops being important, so it can land after the app is paused, for example while a publish holds a foreground service. scheduleFrame() then returns without a frame, and nothing is freed until the app returns.
The rule agents load said Flutter's default ImageCache holds 100 images (it holds 1000 entries and 100 MiB) and told them to fix low-end OOMs by assigning maximumSizeBytes directly. After this PR that assignment would silently override the RAM-sized budget on every device. Name the one place the budget is set, and the helper to clear through.
This comment has been minimized.
This comment has been minimized.
realmeylisdev
approved these changes
Oct 19, 2026
realmeylisdev
left a comment
realmeylisdev
left a comment
Contributor
There was a problem hiding this comment.
@hm21 I reviewed this at 955f4e0 and pushed nine small commits to the branch, one per finding, so you can revert any of them on its own. This approval covers the new head da7e954.
The change holds up. The budget math matches the table, and both native channels return sensible values. The startup read also can't race the channel registration. On iOS the implicit-engine callback that registers the channel runs before Dart starts (Flutter 3.47.2 engine: FlutterViewController.mm:307-313, with the launch at :744). On Android, configureFlutterEngine runs in onAttach, before the Dart entrypoint starts in onStart. That ordering is what shows the iOS channel works. The iPhone 12 Pro run can't show it alone: 6 GB maps to 128 MB, which is also the fallback. Most of what I found was tests that couldn't fail.
Pushed
| Commit | What and why |
|---|---|
c425b57 |
The configure test fed 5.6 GiB. That reading's budget is exactly the 128 MB fallback, so the test passed even if the reading was ignored. It now feeds 7.4 GiB, whose budget is the 8 GB one. |
4294292 |
In the live-images test, the second frame check couldn't fail, because the first clear had already scheduled a frame. The test now draws that frame first. |
f026513 |
Nothing checked that the helper uses scheduleFrame() rather than scheduleForcedFrame(). A new test clears while the app is paused and expects no frame. |
c669088 |
No test ran clearOpenVineImageCache(); every caller in the suite is faked. Two tests now cover includeLiveImages: true (what keeps revoked adult media out of memory), the frame and the disk clear. This touches production code: it adds an optional @visibleForTesting diskCache parameter. The real disk cache can't run in the shared test isolate, because its index writes on a 3-second timer that outlives the test. Production calls are unchanged. |
de7c7c7 |
No test ran the startup call, which is now the only place the budget takes effect. Added a wiring test, like error_widget_builder_wiring_test.dart. |
e27a7a1 |
Added the same wiring test for the memory-pressure clear in DivineApp. That clear is where the measured background release comes from. |
d58b889 |
The dartdoc said Impeller holds the memory until the next frame. In fact ImageCache itself releases each dropped image in a post-frame callback (Flutter 3.47.2 painting/image_cache.dart:601-611). So without a frame nothing is freed, on any renderer. The PR description has the same explanation and is worth fixing too. |
880ea2b |
The doc presented the S26's event order as Android behaviour. Android doesn't guarantee that order (see 3 below). |
da7e954 |
.claude/rules/performance.md told agents to set maximumSizeBytes directly, which would now override the RAM-based budget on every device. It also said Flutter's default holds 100 images; it holds 1000. The rule now points at image_cache_budget.dart and the clear helper. |
I checked each test fix by mutation. With the behaviour deliberately broken, the old test still passed and the new one fails.
Yours to decide (nothing pushed)
- Stop-motion playback on 4-8 GB phones.
StopMotionPlayerprecaches every still once and shows only the current one. So a loop plays from memory only while all of its stills fit in the cache. The preview screens decode stills at screen height, which is about 14 MB per 9:16 still on an iPhone 12 Pro. About 18 stills fit in 256 MB and about 9 fit in 128 MB. A sequence between those sizes used to loop from memory. Now every tick misses the cache and decodes the next still from disk again. That's the stuttercacheHeightwas added to prevent (stop_motion_player.dart:54-58). Longer sequences already did this on main. One playback check on a 6 GB phone would settle it. If it stutters, the player could hold its stills withkeepAlive()handles while it's mounted, or decode them at the player's own size. - Web and desktop drop from 256 MB to 128 MB. They report no RAM, so they get the fallback. The table says this is intended. I'm only flagging that the 2× GPU-cost reasoning was measured on two phones. macOS could read
ProcessInfo.physicalMemorythe same way iOS does. - A background clear can still free nothing. The background release depends on Android's trim signal arriving while the app is still
inactive. Android sends it later while the process stays important, for example while a publish holds its foreground service. ThenscheduleFrame()does nothing, and the memory isn't freed until the app returns. This is no worse than main. - Nothing makes callers use the helper. In #10306,
ReleaseCachedImagesOnDisposestill callsimageCache.clear()directly, and its doc namesimageCacheMaxBytesas the budget. Both need updating once these two PRs meet. A zero-baseline guard against directimageCache.clear()calls would make the convention stick.
Declined
- Running the memory read alongside the two native calls before it: it saves about a millisecond.
- Rounding to the nearest standard RAM size: a phone that reports more than 1 GiB below its installed RAM drops one budget step, which errs toward less GPU memory.
- Removing
DeviceMemoryService's unusedchannelparameter: it mirrorsInstallSourceServiceexactly.
Validation: flutter analyze lib test integration_test is clean. 218 tests pass across test/startup, test/app, both cache-helper tests, the device-memory and memory-pressure tests, social_providers_cleanup_test and moderation_providers_test. scripts/ci/run_guards.sh: all 65 guards pass. CI on da7e954: 29 checks pass, including iOS Native Tests, and the daily service-test publish is skipped as usual.
Independent review: the description doesn't record which model wrote the original change. This review and the pushed fixes are by Claude, so independent review of this head needs a reviewer from another provider. I added my identity block to the description.
@rabble, the head has moved since you were requested.
Agent-Model: anthropic/claude-opus-5-5
Agent-Effort: max
Agent-Harness: claude-code/2.1.303
Agent-Context: fresh
Initiated-By: @realmeylisdev
Both tests read a Dart file as text and match it with a regex, so they check how the call is written rather than what the app does: a harmless refactor (extracting the argument, renaming the service) turns them red, while a reviewer sees a deleted call in the diff anyway. The behaviour stays covered by the image_cache_budget, decoded_image_cache and open_vine_image_cache tests.
Member
Author
|
Thanks @realmeylisdev, the review commits helped: the post-frame-callback explanation is the real mechanism ( I removed two of them in 1e0ca09: |
hm21
enabled auto-merge
October 19, 2026 13:35
Mobile PR PreviewPreview refreshed for Last refresh:
|
1 of 7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-ups from the image cache budget study in #10301. Two changes, plus one investigated idea that turned out not to be needed.
1. The decoded-image cache budget is sized by RAM. It was a fixed 256 MB on every device. Under Impeller every MB of full cache costs about 2 MB of graphics memory (S26 and iPhone 12 Pro), so the fixed budget cost ~500 MB on a 6 GB phone just as on a 12 GB one. The budget is now RAM / 48, clamped to 96–256 MB, which keeps its real cost near 4 % of RAM:
The OS reports a bit less than the installed RAM (the S26 reports 11.4 GiB), so the reading is rounded up to whole GiB before dividing.
device_info_plus10.1.2 has no RAM getter, so a newdivine/device_memorymethod channel returnsActivityManager.MemoryInfo.totalMemon Android andProcessInfo.physicalMemoryon iOS. I looked for an existing channel first: the app's own channels are install source, ProofMode, Zendesk, navigation and the app badge, anddivine_video_player's diagnostics channel is Apple-only and lives in the player package, so none of them fits without mixing concerns. The budget is read once in the bootstrap, after the plugin calls, before anything decodes.2. Every explicit cache clear schedules a frame.
ImageCache.clear()only drops references: Impeller gives the textures back when it next draws a frame, and a static screen draws none.clearDecodedImageCache()clears and callsscheduleFrame(). The memory-pressure handler andclearOpenVineImageCache()use it. It usesscheduleFrame()rather thanscheduleForcedFrame(), so a clear in the background draws nothing until the app returns.3. Investigated, not shipped: clearing on Android before the surface goes away. On
mainthe backgrounded S26 kept ~630 MB of GL memory because nothing drew a frame after the trim-memory clear. With change 2 that clear already returns it: on the S26 the trim-memory signal arrives while the app is stillinactive, so the frame it schedules is drawn before the surface goes. Clearing again oninactivewas worse, presumably because the visible thumbnails are decoded again during the transition (10–12 extra frames), andinactivealso fires when the notification shade is pulled down, which would empty the cache. Clearing onhiddenwith a forced frame added nothing. Numbers below.Related Issue: Closes #none · Relates to #10301
Device measurements
Profile builds of this branch with an uncommitted in-app driver: home feed, Explore → New grid scrolled until the cache reached its budget, then the steps below. S26 GL mtrack from
dumpsys meminfo(two runs), iPhone physical footprint read in-app (one run).Galaxy S26 (12 GB, Android 16): budget 256 MB.
clear()without a frame (whatmaindoes): GL for 24 s, 0 frames drawnclearDecodedImageCache(): before → within 1 smain(study runs, before → background)inactive+ frame (not shipped)hidden+ forced frame (not shipped)iPhone 12 Pro (6 GB, iOS 18.5): budget 128 MB (was 256 MB). From the study, 128 MB saves ~210 MB of footprint on this phone at a full cache and keeps ~15 grid screens of instant scroll-back instead of ~30.
clear()without a frame: for 24 s, 0 frames drawnclearDecodedImageCache(): before → within 2 sOut of Scope
ReleaseCachedImagesOnDisposefrom perf(video-editor): give back the memory that camera and editor sessions kept #10306 is not merged yet and callsimageCache.clear()directly. Once both are in, it should callclearDecodedImageCache()so the camera and editor clear also returns memory on a static screen.*.divine.videoload throughImage.networkwith no disk cache, so a memory miss is an HTTP re-fetch. Unchanged here.Verification
flutter analyze lib test integration_test: no issues.flutter testfor the new tests (test/startup/image_cache_budget_test.dart,test/services/device_memory_service_test.dart,test/utils/decoded_image_cache_test.dart) plustest/startup,test/app, the memory-pressure handler andsocial_providers_cleanup_test.dart: 211 passed.bash scripts/ci/run_guards.sh: 65 passed.Type of Change
Review fixes were pushed to this branch by an agent run that @realmeylisdev started. The review on this PR lists each commit.