Skip to content

Commit 9169fc1

Browse files
NoamDevNoamDev
authored
Revert "Fix RasterCache LRU logic, opportunistic simplifications. (flutter-team-archive#16434)"
This reverts commit e9558d4.
1 parent 3ee9466 commit 9169fc1

3 files changed

Lines changed: 73 additions & 84 deletions

File tree

‎flow/raster_cache.cc‎

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

1818
namespace flutter {
1919

20+
RasterCacheResult::RasterCacheResult() {}
21+
22+
RasterCacheResult::RasterCacheResult(const RasterCacheResult& other) = default;
23+
24+
RasterCacheResult::~RasterCacheResult() = default;
25+
2026
RasterCacheResult::RasterCacheResult(sk_sp<SkImage> image,
2127
const SkRect& logical_rect)
2228
: image_(std::move(image)), logical_rect_(logical_rect) {}
@@ -35,7 +41,10 @@ RasterCache::RasterCache(size_t access_threshold,
3541
size_t picture_cache_limit_per_frame)
3642
: access_threshold_(access_threshold),
3743
picture_cache_limit_per_frame_(picture_cache_limit_per_frame),
38-
checkerboard_images_(false) {}
44+
checkerboard_images_(false),
45+
weak_factory_(this) {}
46+
47+
RasterCache::~RasterCache() = default;
3948

4049
static bool CanRasterizePicture(SkPicture* picture) {
4150
if (picture == nullptr) {
@@ -129,12 +138,24 @@ RasterCacheResult RasterizePicture(SkPicture* picture,
129138
[=](SkCanvas* canvas) { canvas->drawPicture(picture); });
130139
}
131140

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+
132153
void RasterCache::Prepare(PrerollContext* context,
133154
Layer* layer,
134155
const SkMatrix& ctm) {
135156
LayerRasterCacheKey cache_key(layer->unique_id(), ctm);
136157
Entry& entry = layer_cache_[cache_key];
137-
entry.access_count++;
158+
entry.access_count = ClampSize(entry.access_count + 1, 0, access_threshold_);
138159
entry.used_this_frame = true;
139160
if (!entry.image.is_valid()) {
140161
entry.image = Rasterize(
@@ -170,10 +191,6 @@ bool RasterCache::Prepare(GrContext* context,
170191
SkColorSpace* dst_color_space,
171192
bool is_complex,
172193
bool will_change) {
173-
// Disabling caching when access_threshold is zero is historic behavior.
174-
if (access_threshold_ == 0) {
175-
return false;
176-
}
177194
if (picture_cached_this_frame_ >= picture_cache_limit_per_frame_) {
178195
return false;
179196
}
@@ -193,9 +210,11 @@ bool RasterCache::Prepare(GrContext* context,
193210

194211
PictureRasterCacheKey cache_key(picture->uniqueID(), transformation_matrix);
195212

196-
// Creates an entry, if not present prior.
197213
Entry& entry = picture_cache_[cache_key];
198-
if (entry.access_count < access_threshold_) {
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) {
199218
// Frame threshold has not yet been reached.
200219
return false;
201220
}
@@ -212,34 +231,20 @@ RasterCacheResult RasterCache::Get(const SkPicture& picture,
212231
const SkMatrix& ctm) const {
213232
PictureRasterCacheKey cache_key(picture.uniqueID(), ctm);
214233
auto it = picture_cache_.find(cache_key);
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;
234+
return it == picture_cache_.end() ? RasterCacheResult() : it->second.image;
224235
}
225236

226237
RasterCacheResult RasterCache::Get(Layer* layer, const SkMatrix& ctm) const {
227238
LayerRasterCacheKey cache_key(layer->unique_id(), ctm);
228239
auto it = layer_cache_.find(cache_key);
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;
240+
return it == layer_cache_.end() ? RasterCacheResult() : it->second.image;
238241
}
239242

240243
void RasterCache::SweepAfterFrame() {
241-
SweepOneCacheAfterFrame(picture_cache_);
242-
SweepOneCacheAfterFrame(layer_cache_);
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_);
243248
picture_cached_this_frame_ = 0;
244249
TraceStatsToTimeline();
245250
}

‎flow/raster_cache.h‎

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

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

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

2628
RasterCacheResult(sk_sp<SkImage> image, const SkRect& logical_rect);
2729

@@ -54,6 +56,8 @@ class RasterCache {
5456
size_t access_threshold = 3,
5557
size_t picture_cache_limit_per_frame = kDefaultPictureCacheLimitPerFrame);
5658

59+
~RasterCache();
60+
5761
static SkIRect GetDeviceBounds(const SkRect& rect, const SkMatrix& ctm) {
5862
SkRect device_rect;
5963
ctm.mapRect(&device_rect, rect);
@@ -105,9 +109,9 @@ class RasterCache {
105109
RasterCacheResult image;
106110
};
107111

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

112116
for (auto it = cache.begin(); it != cache.end(); ++it) {
113117
Entry& entry = it->second;
@@ -125,9 +129,10 @@ class RasterCache {
125129
const size_t access_threshold_;
126130
const size_t picture_cache_limit_per_frame_;
127131
size_t picture_cached_this_frame_ = 0;
128-
mutable PictureRasterCacheKey::Map<Entry> picture_cache_;
129-
mutable LayerRasterCacheKey::Map<Entry> layer_cache_;
132+
PictureRasterCacheKey::Map<Entry> picture_cache_;
133+
LayerRasterCacheKey::Map<Entry> layer_cache_;
130134
bool checkerboard_images_;
135+
fml::WeakPtrFactory<RasterCache> weak_factory_;
131136

132137
void TraceStatsToTimeline() const;
133138

‎flow/raster_cache_unittests.cc‎

Lines changed: 29 additions & 50 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 = 2;
33+
size_t threshold = 3;
3434
flutter::RasterCache cache(threshold);
3535

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

4242
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
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-
43+
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
44+
false)); // 1
4845
cache.SweepAfterFrame();
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-
46+
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
47+
false)); // 2
48+
cache.SweepAfterFrame();
49+
ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
50+
false)); // 3
5651
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());
6252
}
6353

64-
TEST(RasterCache, AccessThresholdOfZeroDisablesCaching) {
54+
TEST(RasterCache, ThresholdIsRespectedWhenZero) {
6555
size_t threshold = 0;
6656
flutter::RasterCache cache(threshold);
6757

@@ -72,31 +62,19 @@ TEST(RasterCache, AccessThresholdOfZeroDisablesCaching) {
7262
sk_sp<SkImage> image;
7363

7464
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
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());
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();
9674
}
9775

9876
TEST(RasterCache, SweepsRemoveUnusedFrames) {
99-
size_t threshold = 1;
77+
size_t threshold = 3;
10078
flutter::RasterCache cache(threshold);
10179

10280
SkMatrix matrix = SkMatrix::I();
@@ -108,18 +86,19 @@ TEST(RasterCache, SweepsRemoveUnusedFrames) {
10886
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
10987
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
11088
false)); // 1
111-
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
112-
11389
cache.SweepAfterFrame();
114-
90+
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
91+
false)); // 2
92+
cache.SweepAfterFrame();
11593
ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
116-
false)); // 2
117-
ASSERT_TRUE(cache.Get(*picture, matrix).is_valid());
118-
94+
false)); // 3
11995
cache.SweepAfterFrame();
120-
cache.SweepAfterFrame(); // Extra frame without a Get image access.
121-
122-
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
96+
ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
97+
false)); // 4
98+
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
123102
}
124103

125104
} // namespace testing

0 commit comments

Comments
 (0)