Skip to content

Commit e9558d4

Browse files
ignatzNoamDev
ignatz
authored and
NoamDev
committed
Fix RasterCache LRU logic, opportunistic simplifications. (flutter#16434)
RasterCache::Get() methods were not updating the RasterCache::Entry access_count and used_this_frame fields, as is done in RasterCache::Prepare(). This can result in onscreen images being evicted from the cache as new entries are created (e.g. as new elements scroll onscreen).
1 parent ed95cfa commit e9558d4

File tree

3 files changed

+84
-73
lines changed

3 files changed

+84
-73
lines changed

‎flow/raster_cache.cc‎

Lines changed: 28 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,6 @@
1717

1818
namespace flutter {
1919

20-
RasterCacheResult::RasterCacheResult() {}
21-
22-
RasterCacheResult::RasterCacheResult(const RasterCacheResult& other) = default;
23-
24-
RasterCacheResult::~RasterCacheResult() = default;
25-
2620
RasterCacheResult::RasterCacheResult(sk_sp<SkImage> image,
2721
const SkRect& logical_rect)
2822
: image_(std::move(image)), logical_rect_(logical_rect) {}
@@ -41,10 +35,7 @@ RasterCache::RasterCache(size_t access_threshold,
4135
size_t picture_cache_limit_per_frame)
4236
: access_threshold_(access_threshold),
4337
picture_cache_limit_per_frame_(picture_cache_limit_per_frame),
44-
checkerboard_images_(false),
45-
weak_factory_(this) {}
46-
47-
RasterCache::~RasterCache() = default;
38+
checkerboard_images_(false) {}
4839

4940
static bool CanRasterizePicture(SkPicture* picture) {
5041
if (picture == nullptr) {
@@ -138,24 +129,12 @@ RasterCacheResult RasterizePicture(SkPicture* picture,
138129
[=](SkCanvas* canvas) { canvas->drawPicture(picture); });
139130
}
140131

141-
static inline size_t ClampSize(size_t value, size_t min, size_t max) {
142-
if (value > max) {
143-
return max;
144-
}
145-
146-
if (value < min) {
147-
return min;
148-
}
149-
150-
return value;
151-
}
152-
153132
void RasterCache::Prepare(PrerollContext* context,
154133
Layer* layer,
155134
const SkMatrix& ctm) {
156135
LayerRasterCacheKey cache_key(layer->unique_id(), ctm);
157136
Entry& entry = layer_cache_[cache_key];
158-
entry.access_count = ClampSize(entry.access_count + 1, 0, access_threshold_);
137+
entry.access_count++;
159138
entry.used_this_frame = true;
160139
if (!entry.image.is_valid()) {
161140
entry.image = Rasterize(
@@ -191,6 +170,10 @@ bool RasterCache::Prepare(GrContext* context,
191170
SkColorSpace* dst_color_space,
192171
bool is_complex,
193172
bool will_change) {
173+
// Disabling caching when access_threshold is zero is historic behavior.
174+
if (access_threshold_ == 0) {
175+
return false;
176+
}
194177
if (picture_cached_this_frame_ >= picture_cache_limit_per_frame_) {
195178
return false;
196179
}
@@ -210,11 +193,9 @@ bool RasterCache::Prepare(GrContext* context,
210193

211194
PictureRasterCacheKey cache_key(picture->uniqueID(), transformation_matrix);
212195

196+
// Creates an entry, if not present prior.
213197
Entry& entry = picture_cache_[cache_key];
214-
entry.access_count = ClampSize(entry.access_count + 1, 0, access_threshold_);
215-
entry.used_this_frame = true;
216-
217-
if (entry.access_count < access_threshold_ || access_threshold_ == 0) {
198+
if (entry.access_count < access_threshold_) {
218199
// Frame threshold has not yet been reached.
219200
return false;
220201
}
@@ -231,20 +212,34 @@ RasterCacheResult RasterCache::Get(const SkPicture& picture,
231212
const SkMatrix& ctm) const {
232213
PictureRasterCacheKey cache_key(picture.uniqueID(), ctm);
233214
auto it = picture_cache_.find(cache_key);
234-
return it == picture_cache_.end() ? RasterCacheResult() : it->second.image;
215+
if (it == picture_cache_.end()) {
216+
return RasterCacheResult();
217+
}
218+
219+
Entry& entry = it->second;
220+
entry.access_count++;
221+
entry.used_this_frame = true;
222+
223+
return entry.image;
235224
}
236225

237226
RasterCacheResult RasterCache::Get(Layer* layer, const SkMatrix& ctm) const {
238227
LayerRasterCacheKey cache_key(layer->unique_id(), ctm);
239228
auto it = layer_cache_.find(cache_key);
240-
return it == layer_cache_.end() ? RasterCacheResult() : it->second.image;
229+
if (it == layer_cache_.end()) {
230+
return RasterCacheResult();
231+
}
232+
233+
Entry& entry = it->second;
234+
entry.access_count++;
235+
entry.used_this_frame = true;
236+
237+
return entry.image;
241238
}
242239

243240
void RasterCache::SweepAfterFrame() {
244-
using PictureCache = PictureRasterCacheKey::Map<Entry>;
245-
using LayerCache = LayerRasterCacheKey::Map<Entry>;
246-
SweepOneCacheAfterFrame<PictureCache, PictureCache::iterator>(picture_cache_);
247-
SweepOneCacheAfterFrame<LayerCache, LayerCache::iterator>(layer_cache_);
241+
SweepOneCacheAfterFrame(picture_cache_);
242+
SweepOneCacheAfterFrame(layer_cache_);
248243
picture_cached_this_frame_ = 0;
249244
TraceStatsToTimeline();
250245
}

‎flow/raster_cache.h‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,9 @@ namespace flutter {
1919

2020
class RasterCacheResult {
2121
public:
22-
RasterCacheResult();
22+
RasterCacheResult() = default;
2323

24-
RasterCacheResult(const RasterCacheResult& other);
25-
26-
~RasterCacheResult();
24+
RasterCacheResult(const RasterCacheResult& other) = default;
2725

2826
RasterCacheResult(sk_sp<SkImage> image, const SkRect& logical_rect);
2927

@@ -56,8 +54,6 @@ class RasterCache {
5654
size_t access_threshold = 3,
5755
size_t picture_cache_limit_per_frame = kDefaultPictureCacheLimitPerFrame);
5856

59-
~RasterCache();
60-
6157
static SkIRect GetDeviceBounds(const SkRect& rect, const SkMatrix& ctm) {
6258
SkRect device_rect;
6359
ctm.mapRect(&device_rect, rect);
@@ -109,9 +105,9 @@ class RasterCache {
109105
RasterCacheResult image;
110106
};
111107

112-
template <class Cache, class Iterator>
108+
template <class Cache>
113109
static void SweepOneCacheAfterFrame(Cache& cache) {
114-
std::vector<Iterator> dead;
110+
std::vector<typename Cache::iterator> dead;
115111

116112
for (auto it = cache.begin(); it != cache.end(); ++it) {
117113
Entry& entry = it->second;
@@ -129,10 +125,9 @@ class RasterCache {
129125
const size_t access_threshold_;
130126
const size_t picture_cache_limit_per_frame_;
131127
size_t picture_cached_this_frame_ = 0;
132-
PictureRasterCacheKey::Map<Entry> picture_cache_;
133-
LayerRasterCacheKey::Map<Entry> layer_cache_;
128+
mutable PictureRasterCacheKey::Map<Entry> picture_cache_;
129+
mutable LayerRasterCacheKey::Map<Entry> layer_cache_;
134130
bool checkerboard_images_;
135-
fml::WeakPtrFactory<RasterCache> weak_factory_;
136131

137132
void TraceStatsToTimeline() const;
138133

‎flow/raster_cache_unittests.cc‎

Lines changed: 50 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ TEST(RasterCache, SimpleInitialization) {
3030
}
3131

3232
TEST(RasterCache, ThresholdIsRespected) {
33-
size_t threshold = 3;
33+
size_t threshold = 2;
3434
flutter::RasterCache cache(threshold);
3535

3636
SkMatrix matrix = SkMatrix::I();
@@ -40,18 +40,28 @@ TEST(RasterCache, ThresholdIsRespected) {
4040
sk_sp<SkImage> image;
4141

4242
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
43-
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
44-
false)); // 1
45-
cache.SweepAfterFrame();
46-
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
47-
false)); // 2
43+
ASSERT_FALSE(
44+
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));
45+
// 1st access.
46+
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
47+
4848
cache.SweepAfterFrame();
49-
ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
50-
false)); // 3
49+
50+
ASSERT_FALSE(
51+
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));
52+
53+
// 2st access.
54+
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
55+
5156
cache.SweepAfterFrame();
57+
58+
// Now Prepare should cache it.
59+
ASSERT_TRUE(
60+
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));
61+
ASSERT_TRUE(cache.Get(*picture, matrix).is_valid());
5262
}
5363

54-
TEST(RasterCache, ThresholdIsRespectedWhenZero) {
64+
TEST(RasterCache, AccessThresholdOfZeroDisablesCaching) {
5565
size_t threshold = 0;
5666
flutter::RasterCache cache(threshold);
5767

@@ -62,19 +72,31 @@ TEST(RasterCache, ThresholdIsRespectedWhenZero) {
6272
sk_sp<SkImage> image;
6373

6474
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
65-
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
66-
false)); // 1
67-
cache.SweepAfterFrame();
68-
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
69-
false)); // 2
70-
cache.SweepAfterFrame();
71-
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
72-
false)); // 3
73-
cache.SweepAfterFrame();
75+
ASSERT_FALSE(
76+
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));
77+
78+
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
79+
}
80+
81+
TEST(RasterCache, PictureCacheLimitPerFrameIsRespectedWhenZero) {
82+
size_t picture_cache_limit_per_frame = 0;
83+
flutter::RasterCache cache(3, picture_cache_limit_per_frame);
84+
85+
SkMatrix matrix = SkMatrix::I();
86+
87+
auto picture = GetSamplePicture();
88+
89+
sk_sp<SkImage> image;
90+
91+
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
92+
ASSERT_FALSE(
93+
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));
94+
95+
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
7496
}
7597

7698
TEST(RasterCache, SweepsRemoveUnusedFrames) {
77-
size_t threshold = 3;
99+
size_t threshold = 1;
78100
flutter::RasterCache cache(threshold);
79101

80102
SkMatrix matrix = SkMatrix::I();
@@ -86,19 +108,18 @@ TEST(RasterCache, SweepsRemoveUnusedFrames) {
86108
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
87109
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
88110
false)); // 1
111+
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
112+
89113
cache.SweepAfterFrame();
90-
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
91-
false)); // 2
92-
cache.SweepAfterFrame();
93-
ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
94-
false)); // 3
95-
cache.SweepAfterFrame();
114+
96115
ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
97-
false)); // 4
116+
false)); // 2
117+
ASSERT_TRUE(cache.Get(*picture, matrix).is_valid());
118+
98119
cache.SweepAfterFrame();
99-
cache.SweepAfterFrame(); // Extra frame without a preroll image access.
100-
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
101-
false)); // 5
120+
cache.SweepAfterFrame(); // Extra frame without a Get image access.
121+
122+
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
102123
}
103124

104125
} // namespace testing

0 commit comments

Comments
 (0)