From 40261ac0f0ae77d7a21eda842fa53ef443de0192 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Fri, 8 Jul 2022 18:03:54 -0700 Subject: [PATCH 1/4] simplify logic around when to increment DisplayList cache access count --- flow/layers/display_list_raster_cache_item.cc | 17 ++++------- flow/layers/layer_raster_cache_item.cc | 7 ++--- flow/layers/layer_raster_cache_item.h | 2 -- flow/raster_cache.cc | 30 +++++++------------ flow/raster_cache.h | 22 +++++++------- flow/testing/mock_raster_cache.cc | 4 +-- 6 files changed, 32 insertions(+), 50 deletions(-) diff --git a/flow/layers/display_list_raster_cache_item.cc b/flow/layers/display_list_raster_cache_item.cc index 75a79bcf33c14..152ea5ca2bfb8 100644 --- a/flow/layers/display_list_raster_cache_item.cc +++ b/flow/layers/display_list_raster_cache_item.cc @@ -108,16 +108,12 @@ void DisplayListRasterCacheItem::PrerollFinalize(PrerollContext* context, // if the rect is intersect we will get the entry access_count to confirm if // it great than the threshold. Otherwise we only increase the entry // access_count. - if (context->cull_rect.intersect(bounds)) { - if (raster_cache->MarkSeen(key_id_, transformation_matrix_) < - raster_cache->access_threshold()) { - cache_state_ = CacheState::kNone; - return; - } - context->subtree_can_inherit_opacity = true; - cache_state_ = CacheState::kCurrent; + bool visible = context->cull_rect.intersect(bounds); + int accesses = raster_cache->MarkSeen(key_id_, matrix, visible); + if (!visible || accesses < raster_cache->access_threshold()) { + cache_state_ = kNone; } else { - raster_cache->Touch(key_id_, matrix); + cache_state_ = kCurrent; } return; } @@ -136,9 +132,6 @@ bool DisplayListRasterCacheItem::Draw(const PaintContext& context, if (cache_state_ == CacheState::kCurrent) { return context.raster_cache->Draw(key_id_, *canvas, paint); } - // This display_list doesn't cache itself, this only increase the entry - // access_count; - context.raster_cache->Touch(key_id_, canvas->getTotalMatrix()); return false; } diff --git a/flow/layers/layer_raster_cache_item.cc b/flow/layers/layer_raster_cache_item.cc index d0d987fcaa793..b0e29405ccc11 100644 --- a/flow/layers/layer_raster_cache_item.cc +++ b/flow/layers/layer_raster_cache_item.cc @@ -50,12 +50,11 @@ void LayerRasterCacheItem::PrerollFinalize(PrerollContext* context, return; } child_items_ = context->raster_cached_entries->size() - child_items_; - if (num_cache_attempts_ >= layer_cached_threshold_) { + int attempts = context->raster_cache->MarkSeen(key_id_, matrix_, true); + if (attempts >= layer_cached_threshold_) { // the layer can be cached cache_state_ = CacheState::kCurrent; - context->raster_cache->MarkSeen(key_id_, matrix_); } else { - num_cache_attempts_++; // access current layer if (can_cache_children_) { if (!layer_children_id_.has_value()) { @@ -67,7 +66,7 @@ void LayerRasterCacheItem::PrerollFinalize(PrerollContext* context, RasterCacheKeyType::kLayerChildren); } cache_state_ = CacheState::kChildren; - context->raster_cache->MarkSeen(layer_children_id_.value(), matrix_); + context->raster_cache->MarkSeen(layer_children_id_.value(), matrix_, true); } } } diff --git a/flow/layers/layer_raster_cache_item.h b/flow/layers/layer_raster_cache_item.h index 3588abdf3b627..ae3e4f2840ae2 100644 --- a/flow/layers/layer_raster_cache_item.h +++ b/flow/layers/layer_raster_cache_item.h @@ -63,8 +63,6 @@ class LayerRasterCacheItem : public RasterCacheItem { // if the layer's children can be directly cache, set the param is true; bool can_cache_children_ = false; - - mutable int num_cache_attempts_ = 1; }; } // namespace flutter diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index 720efea1866ff..0862f408f467f 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -92,7 +92,6 @@ bool RasterCache::UpdateCacheEntry( const std::function& render_function) const { RasterCacheKey key = RasterCacheKey(id, raster_cache_context.matrix); Entry& entry = cache_[key]; - entry.used_this_frame = true; if (!entry.image) { entry.image = Rasterize(raster_cache_context, render_function); if (entry.image != nullptr) { @@ -110,24 +109,17 @@ bool RasterCache::UpdateCacheEntry( return entry.image != nullptr; } -bool RasterCache::Touch(const RasterCacheKeyID& id, - const SkMatrix& matrix) const { - RasterCacheKey cache_key = RasterCacheKey(id, matrix); - auto it = cache_.find(cache_key); - if (it != cache_.end()) { - it->second.access_count++; - it->second.used_this_frame = true; - return true; - } - return false; -} - int RasterCache::MarkSeen(const RasterCacheKeyID& id, - const SkMatrix& matrix) const { + const SkMatrix& matrix, + bool visible) const { RasterCacheKey key = RasterCacheKey(id, matrix); Entry& entry = cache_[key]; - entry.used_this_frame = true; - return entry.access_count; + entry.encountered_this_frame = true; + entry.visible_this_frame = visible; + if (visible || entry.accesses_since_visible > 0) { + entry.accesses_since_visible++; + } + return entry.accesses_since_visible; } bool RasterCache::HasEntry(const RasterCacheKeyID& id, @@ -148,8 +140,6 @@ bool RasterCache::Draw(const RasterCacheKeyID& id, } Entry& entry = it->second; - entry.access_count++; - entry.used_this_frame = true; if (entry.image) { entry.image->draw(canvas, paint); @@ -171,7 +161,7 @@ void RasterCache::SweepOneCacheAfterFrame(RasterCacheKey::Map& cache, for (auto it = cache.begin(); it != cache.end(); ++it) { Entry& entry = it->second; - if (!entry.used_this_frame) { + if (!entry.encountered_this_frame) { dead.push_back(it); } else if (entry.image) { RasterCacheKeyKind kind = it->first.kind(); @@ -186,7 +176,7 @@ void RasterCache::SweepOneCacheAfterFrame(RasterCacheKey::Map& cache, break; } } - entry.used_this_frame = false; + entry.encountered_this_frame = false; } for (auto it : dead) { diff --git a/flow/raster_cache.h b/flow/raster_cache.h index 95feec68dbf0c..f7d8682e7bb0b 100644 --- a/flow/raster_cache.h +++ b/flow/raster_cache.h @@ -123,8 +123,6 @@ class RasterCache { SkCanvas& canvas, const SkPaint* paint) const; - bool Touch(const RasterCacheKeyID& id, const SkMatrix& matrix) const; - bool HasEntry(const RasterCacheKeyID& id, const SkMatrix&) const; void PrepareNewFrame(); @@ -191,13 +189,16 @@ class RasterCache { } /** - * @brief The entry which RasterCacheId is generated by RasterCacheKeyID and - * matrix, that will be used by the current frame. If current frame doesn't - * have this entry we will create a new entry. - * @return the number of times the entry has been hit since it was created. If - * a new entry that will be 1. + * @brief The entry whose RasterCacheKey is generated by RasterCacheKeyID + * and matrix is marked as encountered by the current frame. The entry + * will be created if it does not exist. Optionally the entry will be marked + * as visible in the current frame if the caller determines that it + * intersects the cull rect. The access_count of the entry will be + * increased if it is visible, or if it was ever visible. + * @return the number of times the entry has been hit since it was created. For + * a new entry that will be 1 if it is visible, or zero if non-visible. */ - int MarkSeen(const RasterCacheKeyID& id, const SkMatrix& matrix) const; + int MarkSeen(const RasterCacheKeyID& id, const SkMatrix& matrix, bool visible) const; bool UpdateCacheEntry( const RasterCacheKeyID& id, @@ -206,8 +207,9 @@ class RasterCache { private: struct Entry { - bool used_this_frame = false; - size_t access_count = 0; + bool encountered_this_frame = false; + bool visible_this_frame = false; + size_t accesses_since_visible = 0; std::unique_ptr image; }; diff --git a/flow/testing/mock_raster_cache.cc b/flow/testing/mock_raster_cache.cc index 9b69eb39103c2..8f4aec37685b1 100644 --- a/flow/testing/mock_raster_cache.cc +++ b/flow/testing/mock_raster_cache.cc @@ -61,10 +61,10 @@ void MockRasterCache::AddMockPicture(int width, int height) { DisplayListRasterCacheItem display_list_item(display_list.get(), SkPoint(), true, false); for (int i = 0; i < access_threshold(); i++) { - MarkSeen(display_list_item.GetId().value(), ctm); + MarkSeen(display_list_item.GetId().value(), ctm, true); Draw(display_list_item.GetId().value(), mock_canvas_, nullptr); } - MarkSeen(display_list_item.GetId().value(), ctm); + MarkSeen(display_list_item.GetId().value(), ctm, true); RasterCache::Context r_context = { // clang-format off .gr_context = preroll_context_.gr_context, From 33f9c306a9473e36c0a48b0f5c30e4c866d4f807 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Fri, 8 Jul 2022 18:05:52 -0700 Subject: [PATCH 2/4] formatting --- flow/layers/layer_raster_cache_item.cc | 3 ++- flow/raster_cache.h | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/flow/layers/layer_raster_cache_item.cc b/flow/layers/layer_raster_cache_item.cc index b0e29405ccc11..dd7b47936c14c 100644 --- a/flow/layers/layer_raster_cache_item.cc +++ b/flow/layers/layer_raster_cache_item.cc @@ -66,7 +66,8 @@ void LayerRasterCacheItem::PrerollFinalize(PrerollContext* context, RasterCacheKeyType::kLayerChildren); } cache_state_ = CacheState::kChildren; - context->raster_cache->MarkSeen(layer_children_id_.value(), matrix_, true); + context->raster_cache->MarkSeen(layer_children_id_.value(), matrix_, + true); } } } diff --git a/flow/raster_cache.h b/flow/raster_cache.h index f7d8682e7bb0b..298dfea8dfda8 100644 --- a/flow/raster_cache.h +++ b/flow/raster_cache.h @@ -195,10 +195,12 @@ class RasterCache { * as visible in the current frame if the caller determines that it * intersects the cull rect. The access_count of the entry will be * increased if it is visible, or if it was ever visible. - * @return the number of times the entry has been hit since it was created. For - * a new entry that will be 1 if it is visible, or zero if non-visible. + * @return the number of times the entry has been hit since it was created. + * For a new entry that will be 1 if it is visible, or zero if non-visible. */ - int MarkSeen(const RasterCacheKeyID& id, const SkMatrix& matrix, bool visible) const; + int MarkSeen(const RasterCacheKeyID& id, + const SkMatrix& matrix, + bool visible) const; bool UpdateCacheEntry( const RasterCacheKeyID& id, From 7e81105a451f83f81276cf04081ba16902b367d1 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Sat, 9 Jul 2022 14:28:22 -0700 Subject: [PATCH 3/4] restore layer logic, fix unit tests and DL threshold logic --- flow/layers/display_list_layer_unittests.cc | 6 +++--- flow/layers/display_list_raster_cache_item.cc | 3 ++- flow/layers/layer_raster_cache_item.cc | 5 +++-- flow/layers/layer_raster_cache_item.h | 2 ++ 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/flow/layers/display_list_layer_unittests.cc b/flow/layers/display_list_layer_unittests.cc index 941c20702368b..33105670b96aa 100644 --- a/flow/layers/display_list_layer_unittests.cc +++ b/flow/layers/display_list_layer_unittests.cc @@ -238,9 +238,9 @@ TEST_F(DisplayListLayerTest, CachedIncompatibleDisplayListOpacityInheritance) { EXPECT_FALSE(context->subtree_can_inherit_opacity); // Pump the DisplayListLayer until it is ready to cache its DL - display_list_layer->Paint(paint_context()); - display_list_layer->Paint(paint_context()); - display_list_layer->Paint(paint_context()); + display_list_layer->Preroll(preroll_context(), SkMatrix()); + display_list_layer->Preroll(preroll_context(), SkMatrix()); + display_list_layer->Preroll(preroll_context(), SkMatrix()); int opacity_alpha = 0x7F; SkPoint opacity_offset = SkPoint::Make(10, 10); diff --git a/flow/layers/display_list_raster_cache_item.cc b/flow/layers/display_list_raster_cache_item.cc index 152ea5ca2bfb8..4a5adef3bde02 100644 --- a/flow/layers/display_list_raster_cache_item.cc +++ b/flow/layers/display_list_raster_cache_item.cc @@ -110,9 +110,10 @@ void DisplayListRasterCacheItem::PrerollFinalize(PrerollContext* context, // access_count. bool visible = context->cull_rect.intersect(bounds); int accesses = raster_cache->MarkSeen(key_id_, matrix, visible); - if (!visible || accesses < raster_cache->access_threshold()) { + if (!visible || accesses <= raster_cache->access_threshold()) { cache_state_ = kNone; } else { + context->subtree_can_inherit_opacity = true; cache_state_ = kCurrent; } return; diff --git a/flow/layers/layer_raster_cache_item.cc b/flow/layers/layer_raster_cache_item.cc index dd7b47936c14c..0d2b753283e5d 100644 --- a/flow/layers/layer_raster_cache_item.cc +++ b/flow/layers/layer_raster_cache_item.cc @@ -50,11 +50,12 @@ void LayerRasterCacheItem::PrerollFinalize(PrerollContext* context, return; } child_items_ = context->raster_cached_entries->size() - child_items_; - int attempts = context->raster_cache->MarkSeen(key_id_, matrix_, true); - if (attempts >= layer_cached_threshold_) { + if (num_cache_attempts_ >= layer_cached_threshold_) { // the layer can be cached cache_state_ = CacheState::kCurrent; + context->raster_cache->MarkSeen(key_id_, matrix_, true); } else { + num_cache_attempts_++; // access current layer if (can_cache_children_) { if (!layer_children_id_.has_value()) { diff --git a/flow/layers/layer_raster_cache_item.h b/flow/layers/layer_raster_cache_item.h index ae3e4f2840ae2..3588abdf3b627 100644 --- a/flow/layers/layer_raster_cache_item.h +++ b/flow/layers/layer_raster_cache_item.h @@ -63,6 +63,8 @@ class LayerRasterCacheItem : public RasterCacheItem { // if the layer's children can be directly cache, set the param is true; bool can_cache_children_ = false; + + mutable int num_cache_attempts_ = 1; }; } // namespace flutter From e84646f4d8f18864382eede0e827d45b8138a7b8 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Sun, 10 Jul 2022 14:37:57 -0700 Subject: [PATCH 4/4] add test case for DL caching interaction with cull rect --- flow/layers/display_list_layer_unittests.cc | 92 +++++++++++++++++++++ flow/raster_cache.cc | 10 +++ flow/raster_cache.h | 6 ++ 3 files changed, 108 insertions(+) diff --git a/flow/layers/display_list_layer_unittests.cc b/flow/layers/display_list_layer_unittests.cc index 33105670b96aa..ca88583539144 100644 --- a/flow/layers/display_list_layer_unittests.cc +++ b/flow/layers/display_list_layer_unittests.cc @@ -398,5 +398,97 @@ TEST_F(DisplayListLayerTest, NoLayerTreeSnapshotsWhenDisabledByDefault) { EXPECT_EQ(0u, snapshot_store.Size()); } +TEST_F(DisplayListLayerTest, DisplayListAccessCountDependsOnVisibility) { + const SkPoint layer_offset = SkPoint::Make(1.5f, -0.5f); + const SkRect picture_bounds = SkRect::MakeLTRB(5.0f, 6.0f, 20.5f, 21.5f); + const SkRect missed_cull_rect = SkRect::MakeLTRB(100, 100, 200, 200); + const SkRect hit_cull_rect = SkRect::MakeLTRB(0, 0, 200, 200); + DisplayListBuilder builder; + builder.drawRect(picture_bounds); + auto display_list = builder.Build(); + auto layer = std::make_shared( + layer_offset, SkiaGPUObject(display_list, unref_queue()), true, false); + + auto raster_cache_item = layer->raster_cache_item(); + use_mock_raster_cache(); + + // First Preroll the DisplayListLayer a few times where it does not intersect + // the cull rect. No caching progress should occur during this time, the + // access_count should remain 0 because the DisplayList was never "visible". + preroll_context()->cull_rect = missed_cull_rect; + for (int i = 0; i < 10; i++) { + preroll_context()->raster_cached_entries->clear(); + layer->Preroll(preroll_context(), SkMatrix::I()); + ASSERT_EQ(raster_cache_item->cache_state(), RasterCacheItem::kNone); + ASSERT_TRUE(raster_cache_item->GetId().has_value()); + ASSERT_EQ(preroll_context()->raster_cache->GetAccessCount( + raster_cache_item->GetId().value(), SkMatrix::I()), + 0); + ASSERT_EQ(preroll_context()->raster_cached_entries->size(), size_t(1)); + ASSERT_EQ(preroll_context()->raster_cache->EstimatePictureCacheByteSize(), + size_t(0)); + ASSERT_FALSE(raster_cache_item->TryToPrepareRasterCache(paint_context())); + ASSERT_FALSE(raster_cache_item->Draw(paint_context(), nullptr)); + } + + // Next Preroll the DisplayListLayer once where it does intersect + // the cull rect. No caching progress should occur during this time + // since this is the first frame in which it was visible, but the + // count should start incrementing. + preroll_context()->cull_rect = hit_cull_rect; + preroll_context()->raster_cached_entries->clear(); + layer->Preroll(preroll_context(), SkMatrix()); + ASSERT_EQ(raster_cache_item->cache_state(), RasterCacheItem::kNone); + ASSERT_TRUE(raster_cache_item->GetId().has_value()); + ASSERT_EQ(preroll_context()->raster_cache->GetAccessCount( + raster_cache_item->GetId().value(), SkMatrix::I()), + 1); + ASSERT_EQ(preroll_context()->raster_cached_entries->size(), size_t(1)); + ASSERT_EQ(preroll_context()->raster_cache->EstimatePictureCacheByteSize(), + size_t(0)); + ASSERT_FALSE(raster_cache_item->TryToPrepareRasterCache(paint_context())); + ASSERT_FALSE(raster_cache_item->Draw(paint_context(), nullptr)); + + // Now we can Preroll the DisplayListLayer again with a cull rect that + // it does not intersect and it should continue to count these operations + // even though it is not visible. No actual caching should occur yet, + // even though we will surpass its threshold. + preroll_context()->cull_rect = missed_cull_rect; + for (int i = 0; i < 10; i++) { + preroll_context()->raster_cached_entries->clear(); + layer->Preroll(preroll_context(), SkMatrix()); + ASSERT_EQ(raster_cache_item->cache_state(), RasterCacheItem::kNone); + ASSERT_TRUE(raster_cache_item->GetId().has_value()); + ASSERT_EQ(preroll_context()->raster_cache->GetAccessCount( + raster_cache_item->GetId().value(), SkMatrix::I()), + i + 2); + ASSERT_EQ(preroll_context()->raster_cached_entries->size(), size_t(1)); + ASSERT_EQ(preroll_context()->raster_cache->EstimatePictureCacheByteSize(), + size_t(0)); + ASSERT_FALSE(raster_cache_item->TryToPrepareRasterCache(paint_context())); + ASSERT_FALSE(raster_cache_item->Draw(paint_context(), nullptr)); + } + + // Finally Preroll the DisplayListLayer again where it does intersect + // the cull rect. Since we should have exhausted our access count + // threshold in the loop above, these operations should result in the + // DisplayList being cached. + preroll_context()->cull_rect = hit_cull_rect; + preroll_context()->raster_cached_entries->clear(); + layer->Preroll(preroll_context(), SkMatrix()); + ASSERT_EQ(raster_cache_item->cache_state(), RasterCacheItem::kCurrent); + ASSERT_TRUE(raster_cache_item->GetId().has_value()); + ASSERT_EQ(preroll_context()->raster_cache->GetAccessCount( + raster_cache_item->GetId().value(), SkMatrix::I()), + 12); + ASSERT_EQ(preroll_context()->raster_cached_entries->size(), size_t(1)); + ASSERT_EQ(preroll_context()->raster_cache->EstimatePictureCacheByteSize(), + size_t(0)); + ASSERT_TRUE(raster_cache_item->TryToPrepareRasterCache(paint_context())); + ASSERT_GT(preroll_context()->raster_cache->EstimatePictureCacheByteSize(), + size_t(0)); + ASSERT_TRUE(raster_cache_item->Draw(paint_context(), nullptr)); +} + } // namespace testing } // namespace flutter diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc index 0862f408f467f..2284cd238067d 100644 --- a/flow/raster_cache.cc +++ b/flow/raster_cache.cc @@ -122,6 +122,16 @@ int RasterCache::MarkSeen(const RasterCacheKeyID& id, return entry.accesses_since_visible; } +int RasterCache::GetAccessCount(const RasterCacheKeyID& id, + const SkMatrix& matrix) const { + RasterCacheKey key = RasterCacheKey(id, matrix); + auto entry = cache_.find(key); + if (entry != cache_.cend()) { + return entry->second.accesses_since_visible; + } + return -1; +} + bool RasterCache::HasEntry(const RasterCacheKeyID& id, const SkMatrix& matrix) const { RasterCacheKey key = RasterCacheKey(id, matrix); diff --git a/flow/raster_cache.h b/flow/raster_cache.h index 298dfea8dfda8..01f08e12cf90c 100644 --- a/flow/raster_cache.h +++ b/flow/raster_cache.h @@ -202,6 +202,12 @@ class RasterCache { const SkMatrix& matrix, bool visible) const; + /** + * Returns the access count (i.e. accesses_since_visible) for the given + * entry in the cache, or -1 if no such entry exists. + */ + int GetAccessCount(const RasterCacheKeyID& id, const SkMatrix& matrix) const; + bool UpdateCacheEntry( const RasterCacheKeyID& id, const Context& raster_cache_context,