From 9eb589a30e18b5bf7c8c28b51ac89ae281091571 Mon Sep 17 00:00:00 2001 From: Sebastian Jeltsch Date: Thu, 6 Feb 2020 01:01:11 +0100 Subject: [PATCH 1/3] Fix RasterCache LRU logic + opportunistic simplifications. --- flow/raster_cache.cc | 55 +++++++++++++++++++------------------------- flow/raster_cache.h | 17 +++++--------- 2 files changed, 30 insertions(+), 42 deletions(-) diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index a3bbb20b9b05a..092226676c207 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -17,12 +17,6 @@ namespace flutter { -RasterCacheResult::RasterCacheResult() {} - -RasterCacheResult::RasterCacheResult(const RasterCacheResult& other) = default; - -RasterCacheResult::~RasterCacheResult() = default; - RasterCacheResult::RasterCacheResult(sk_sp image, const SkRect& logical_rect) : image_(std::move(image)), logical_rect_(logical_rect) {} @@ -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) { @@ -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( @@ -211,10 +190,10 @@ bool RasterCache::Prepare(GrContext* context, PictureRasterCacheKey cache_key(picture->uniqueID(), transformation_matrix); Entry& entry = picture_cache_[cache_key]; - entry.access_count = ClampSize(entry.access_count + 1, 0, access_threshold_); + entry.access_count++; 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; } @@ -231,20 +210,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; - using LayerCache = LayerRasterCacheKey::Map; - SweepOneCacheAfterFrame(picture_cache_); - SweepOneCacheAfterFrame(layer_cache_); + SweepOneCacheAfterFrame(picture_cache_); + SweepOneCacheAfterFrame(layer_cache_); picture_cached_this_frame_ = 0; TraceStatsToTimeline(); } diff --git a/flow/raster_cache.h b/flow/raster_cache.h index 3ea92588e1fd7..b51382a5c8d2d 100644 --- a/flow/raster_cache.h +++ b/flow/raster_cache.h @@ -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 image, const SkRect& logical_rect); @@ -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); @@ -109,9 +105,9 @@ class RasterCache { RasterCacheResult image; }; - template + template static void SweepOneCacheAfterFrame(Cache& cache) { - std::vector dead; + std::vector dead; for (auto it = cache.begin(); it != cache.end(); ++it) { Entry& entry = it->second; @@ -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 picture_cache_; - LayerRasterCacheKey::Map layer_cache_; + mutable PictureRasterCacheKey::Map picture_cache_; + mutable LayerRasterCacheKey::Map layer_cache_; bool checkerboard_images_; - fml::WeakPtrFactory weak_factory_; void TraceStatsToTimeline() const; From f0c48ab2b7c9188dff8679ca8f2b3fd2de8b6101 Mon Sep 17 00:00:00 2001 From: Sebastian Jeltsch Date: Thu, 6 Feb 2020 18:09:54 +0100 Subject: [PATCH 2/3] Redefine access threshold semantics to reflect the LRU cache semantics more meaningfully and fix tests. Previously, access threshold wouldn't count the accesses but the number of Prepare calls, which was part of the issue. With moving the LRU updating to the Getter(), we were "over-counting" access (Get + Prepare), which shifted the effective threshold. I updated the default to cache everything after it's "Prepared" for the second time (which itself is silly, it would make way more sense to move the rasterization into the Get()). I did explicitly not preserve the undocumented semantics of "access_threshold == 0" meaning that the cache is disabled. Images should be cached after accessing them more than "threshold" times, zero clearly means that everything should be cached always. |picture_cache_limit_per_frame| to zero to disable the cache seems more sensible. Besides, none of these parameters is user-adjustable. --- flow/raster_cache.cc | 4 +-- flow/raster_cache.h | 2 +- flow/raster_cache_unittests.cc | 64 ++++++++++++++++------------------ 3 files changed, 33 insertions(+), 37 deletions(-) diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index 092226676c207..24366deffb912 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -189,10 +189,8 @@ 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++; - entry.used_this_frame = true; - if (entry.access_count < access_threshold_) { // Frame threshold has not yet been reached. return false; diff --git a/flow/raster_cache.h b/flow/raster_cache.h index b51382a5c8d2d..59ae0e74a6c9e 100644 --- a/flow/raster_cache.h +++ b/flow/raster_cache.h @@ -51,7 +51,7 @@ class RasterCache { static constexpr int kDefaultPictureCacheLimitPerFrame = 3; explicit RasterCache( - size_t access_threshold = 3, + size_t access_threshold = 1, size_t picture_cache_limit_per_frame = kDefaultPictureCacheLimitPerFrame); static SkIRect GetDeviceBounds(const SkRect& rect, const SkMatrix& ctm) { diff --git a/flow/raster_cache_unittests.cc b/flow/raster_cache_unittests.cc index bd83d807875f2..7a8a7d638cae0 100644 --- a/flow/raster_cache_unittests.cc +++ b/flow/raster_cache_unittests.cc @@ -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(); @@ -40,20 +40,30 @@ TEST(RasterCache, ThresholdIsRespected) { sk_sp image; sk_sp 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) { - size_t threshold = 0; - flutter::RasterCache cache(threshold); +TEST(RasterCache, PictureCacheLimitPerFrameIsRespectedWhenZero) { + size_t picture_cache_limit_per_frame = 0; + flutter::RasterCache cache(3, picture_cache_limit_per_frame); SkMatrix matrix = SkMatrix::I(); @@ -62,19 +72,14 @@ TEST(RasterCache, ThresholdIsRespectedWhenZero) { sk_sp image; sk_sp 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, SweepsRemoveUnusedFrames) { - size_t threshold = 3; + size_t threshold = 0; flutter::RasterCache cache(threshold); SkMatrix matrix = SkMatrix::I(); @@ -84,21 +89,14 @@ TEST(RasterCache, SweepsRemoveUnusedFrames) { sk_sp image; sk_sp 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_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, - false)); // 3 + false)); // 1 + ASSERT_TRUE(cache.Get(*picture, matrix).is_valid()); cache.SweepAfterFrame(); - ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, - false)); // 4 + 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 From 24a271e9de0b47bf1b8db1e961dec75a6fb447f5 Mon Sep 17 00:00:00 2001 From: Sebastian Jeltsch Date: Thu, 6 Feb 2020 19:40:06 +0100 Subject: [PATCH 3/3] Restore original semantics: value of access_threshold and that 0 means the cache is disabled. --- flow/raster_cache.cc | 4 ++++ flow/raster_cache.h | 2 +- flow/raster_cache_unittests.cc | 31 +++++++++++++++++++++++++++---- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index 24366deffb912..f179905ed8e53 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -170,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; } diff --git a/flow/raster_cache.h b/flow/raster_cache.h index 59ae0e74a6c9e..b51382a5c8d2d 100644 --- a/flow/raster_cache.h +++ b/flow/raster_cache.h @@ -51,7 +51,7 @@ class RasterCache { static constexpr int kDefaultPictureCacheLimitPerFrame = 3; explicit RasterCache( - size_t access_threshold = 1, + size_t access_threshold = 3, size_t picture_cache_limit_per_frame = kDefaultPictureCacheLimitPerFrame); static SkIRect GetDeviceBounds(const SkRect& rect, const SkMatrix& ctm) { diff --git a/flow/raster_cache_unittests.cc b/flow/raster_cache_unittests.cc index 7a8a7d638cae0..03e3b3879c469 100644 --- a/flow/raster_cache_unittests.cc +++ b/flow/raster_cache_unittests.cc @@ -61,6 +61,23 @@ TEST(RasterCache, ThresholdIsRespected) { ASSERT_TRUE(cache.Get(*picture, matrix).is_valid()); } +TEST(RasterCache, AccessThresholdOfZeroDisablesCaching) { + size_t threshold = 0; + flutter::RasterCache cache(threshold); + + SkMatrix matrix = SkMatrix::I(); + + auto picture = GetSamplePicture(); + + sk_sp image; + + sk_sp 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, PictureCacheLimitPerFrameIsRespectedWhenZero) { size_t picture_cache_limit_per_frame = 0; flutter::RasterCache cache(3, picture_cache_limit_per_frame); @@ -79,7 +96,7 @@ TEST(RasterCache, PictureCacheLimitPerFrameIsRespectedWhenZero) { } TEST(RasterCache, SweepsRemoveUnusedFrames) { - size_t threshold = 0; + size_t threshold = 1; flutter::RasterCache cache(threshold); SkMatrix matrix = SkMatrix::I(); @@ -89,13 +106,19 @@ TEST(RasterCache, SweepsRemoveUnusedFrames) { sk_sp image; sk_sp srgb = SkColorSpace::MakeSRGB(); - ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, - false)); // 1 - ASSERT_TRUE(cache.Get(*picture, matrix).is_valid()); + ASSERT_FALSE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, + false)); // 1 + ASSERT_FALSE(cache.Get(*picture, matrix).is_valid()); + cache.SweepAfterFrame(); + + ASSERT_TRUE(cache.Prepare(NULL, picture.get(), matrix, srgb.get(), true, + false)); // 2 ASSERT_TRUE(cache.Get(*picture, matrix).is_valid()); + cache.SweepAfterFrame(); cache.SweepAfterFrame(); // Extra frame without a Get image access. + ASSERT_FALSE(cache.Get(*picture, matrix).is_valid()); }