Skip to content

Commit

Permalink
Fix RasterCache LRU logic, opportunistic simplifications. (flutter#16434
Browse files Browse the repository at this point in the history 
)

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).
  • Loading branch information
@ignatz @NoamDev
ignatz authored and NoamDev committed Feb 27, 2020
1 parent ed95cfa commit e9558d4
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 73 deletions.
61 changes: 28 additions & 33 deletions flow/raster_cache.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,6 @@

namespace flutter {

RasterCacheResult::RasterCacheResult() {}

RasterCacheResult::RasterCacheResult(const RasterCacheResult& other) = default;

RasterCacheResult::~RasterCacheResult() = default;

RasterCacheResult::RasterCacheResult(sk_sp<SkImage> image,
const SkRect& logical_rect)
: image_(std::move(image)), logical_rect_(logical_rect) {}
Expand All @@ -41,10 +35,7 @@ RasterCache::RasterCache(size_t access_threshold,
size_t picture_cache_limit_per_frame)
: access_threshold_(access_threshold),
picture_cache_limit_per_frame_(picture_cache_limit_per_frame),
checkerboard_images_(false),
weak_factory_(this) {}

RasterCache::~RasterCache() = default;
checkerboard_images_(false) {}

static bool CanRasterizePicture(SkPicture* picture) {
if (picture == nullptr) {
Expand Down Expand Up @@ -138,24 +129,12 @@ RasterCacheResult RasterizePicture(SkPicture* picture,
[=](SkCanvas* canvas) { canvas->drawPicture(picture); });
}

static inline size_t ClampSize(size_t value, size_t min, size_t max) {
if (value > max) {
return max;
}

if (value < min) {
return min;
}

return value;
}

void RasterCache::Prepare(PrerollContext* context,
Layer* layer,
const SkMatrix& ctm) {
LayerRasterCacheKey cache_key(layer->unique_id(), ctm);
Entry& entry = layer_cache_[cache_key];
entry.access_count = ClampSize(entry.access_count + 1, 0, access_threshold_);
entry.access_count++;
entry.used_this_frame = true;
if (!entry.image.is_valid()) {
entry.image = Rasterize(
Expand Down Expand Up @@ -191,6 +170,10 @@ bool RasterCache::Prepare(GrContext* context,
SkColorSpace* dst_color_space,
bool is_complex,
bool will_change) {
// Disabling caching when access_threshold is zero is historic behavior.
if (access_threshold_ == 0) {
return false;
}
if (picture_cached_this_frame_ >= picture_cache_limit_per_frame_) {
return false;
}
Expand All @@ -210,11 +193,9 @@ bool RasterCache::Prepare(GrContext* context,

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

// Creates an entry, if not present prior.
Entry& entry = picture_cache_[cache_key];
entry.access_count = ClampSize(entry.access_count + 1, 0, access_threshold_);
entry.used_this_frame = true;

if (entry.access_count < access_threshold_ || access_threshold_ == 0) {
if (entry.access_count < access_threshold_) {
// Frame threshold has not yet been reached.
return false;
}
Expand All @@ -231,20 +212,34 @@ RasterCacheResult RasterCache::Get(const SkPicture& picture,
const SkMatrix& ctm) const {
PictureRasterCacheKey cache_key(picture.uniqueID(), ctm);
auto it = picture_cache_.find(cache_key);
return it == picture_cache_.end() ? RasterCacheResult() : it->second.image;
if (it == picture_cache_.end()) {
return RasterCacheResult();
}

Entry& entry = it->second;
entry.access_count++;
entry.used_this_frame = true;

return entry.image;
}

RasterCacheResult RasterCache::Get(Layer* layer, const SkMatrix& ctm) const {
LayerRasterCacheKey cache_key(layer->unique_id(), ctm);
auto it = layer_cache_.find(cache_key);
return it == layer_cache_.end() ? RasterCacheResult() : it->second.image;
if (it == layer_cache_.end()) {
return RasterCacheResult();
}

Entry& entry = it->second;
entry.access_count++;
entry.used_this_frame = true;

return entry.image;
}

void RasterCache::SweepAfterFrame() {
using PictureCache = PictureRasterCacheKey::Map<Entry>;
using LayerCache = LayerRasterCacheKey::Map<Entry>;
SweepOneCacheAfterFrame<PictureCache, PictureCache::iterator>(picture_cache_);
SweepOneCacheAfterFrame<LayerCache, LayerCache::iterator>(layer_cache_);
SweepOneCacheAfterFrame(picture_cache_);
SweepOneCacheAfterFrame(layer_cache_);
picture_cached_this_frame_ = 0;
TraceStatsToTimeline();
}
Expand Down
17 changes: 6 additions & 11 deletions flow/raster_cache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,9 @@ namespace flutter {

class RasterCacheResult {
public:
RasterCacheResult();
RasterCacheResult() = default;

RasterCacheResult(const RasterCacheResult& other);

~RasterCacheResult();
RasterCacheResult(const RasterCacheResult& other) = default;

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

Expand Down Expand Up @@ -56,8 +54,6 @@ class RasterCache {
size_t access_threshold = 3,
size_t picture_cache_limit_per_frame = kDefaultPictureCacheLimitPerFrame);

~RasterCache();

static SkIRect GetDeviceBounds(const SkRect& rect, const SkMatrix& ctm) {
SkRect device_rect;
ctm.mapRect(&device_rect, rect);
Expand Down Expand Up @@ -109,9 +105,9 @@ class RasterCache {
RasterCacheResult image;
};

template <class Cache, class Iterator>
template <class Cache>
static void SweepOneCacheAfterFrame(Cache& cache) {
std::vector<Iterator> dead;
std::vector<typename Cache::iterator> dead;

for (auto it = cache.begin(); it != cache.end(); ++it) {
Entry& entry = it->second;
Expand All @@ -129,10 +125,9 @@ class RasterCache {
const size_t access_threshold_;
const size_t picture_cache_limit_per_frame_;
size_t picture_cached_this_frame_ = 0;
PictureRasterCacheKey::Map<Entry> picture_cache_;
LayerRasterCacheKey::Map<Entry> layer_cache_;
mutable PictureRasterCacheKey::Map<Entry> picture_cache_;
mutable LayerRasterCacheKey::Map<Entry> layer_cache_;
bool checkerboard_images_;
fml::WeakPtrFactory<RasterCache> weak_factory_;

void TraceStatsToTimeline() const;

Expand Down
79 changes: 50 additions & 29 deletions flow/raster_cache_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ TEST(RasterCache, SimpleInitialization) {
}

TEST(RasterCache, ThresholdIsRespected) {
size_t threshold = 3;
size_t threshold = 2;
flutter::RasterCache cache(threshold);

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

sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 1
cache.SweepAfterFrame();
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 2
ASSERT_FALSE(
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));
// 1st access.
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());

cache.SweepAfterFrame();
ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 3

ASSERT_FALSE(
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));

// 2st access.
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());

cache.SweepAfterFrame();

// Now Prepare should cache it.
ASSERT_TRUE(
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));
ASSERT_TRUE(cache.Get(*picture, matrix).is_valid());
}

TEST(RasterCache, ThresholdIsRespectedWhenZero) {
TEST(RasterCache, AccessThresholdOfZeroDisablesCaching) {
size_t threshold = 0;
flutter::RasterCache cache(threshold);

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

sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 1
cache.SweepAfterFrame();
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 2
cache.SweepAfterFrame();
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 3
cache.SweepAfterFrame();
ASSERT_FALSE(
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));

ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
}

TEST(RasterCache, PictureCacheLimitPerFrameIsRespectedWhenZero) {
size_t picture_cache_limit_per_frame = 0;
flutter::RasterCache cache(3, picture_cache_limit_per_frame);

SkMatrix matrix = SkMatrix::I();

auto picture = GetSamplePicture();

sk_sp<SkImage> image;

sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(
cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, false));

ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
}

TEST(RasterCache, SweepsRemoveUnusedFrames) {
size_t threshold = 3;
size_t threshold = 1;
flutter::RasterCache cache(threshold);

SkMatrix matrix = SkMatrix::I();
Expand All @@ -86,19 +108,18 @@ TEST(RasterCache, SweepsRemoveUnusedFrames) {
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 1
ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());

cache.SweepAfterFrame();
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 2
cache.SweepAfterFrame();
ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 3
cache.SweepAfterFrame();

ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 4
false)); // 2
ASSERT_TRUE(cache.Get(*picture, matrix).is_valid());

cache.SweepAfterFrame();
cache.SweepAfterFrame(); // Extra frame without a preroll image access.
ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true,
false)); // 5
cache.SweepAfterFrame(); // Extra frame without a Get image access.

ASSERT_FALSE(cache.Get(*picture, matrix).is_valid());
}

} // namespace testing
Expand Down

0 comments on commit e9558d4

Please sign in to comment.