Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RasterCache LRU logic + opportunistic simplifications. #16434

Merged
merged 3 commits into from
Feb 6, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Redefine access threshold semantics to reflect the LRU cache semantic…
…s 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.
Sebastian Jeltsch committed Feb 6, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit f0c48ab2b7c9188dff8679ca8f2b3fd2de8b6101
4 changes: 1 addition & 3 deletions flow/raster_cache.cc
Original file line number Diff line number Diff line change
@@ -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_) {
ignatz marked this conversation as resolved.
Show resolved Hide resolved
// Frame threshold has not yet been reached.
return false;
2 changes: 1 addition & 1 deletion flow/raster_cache.h
Original file line number Diff line number Diff line change
@@ -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) {
64 changes: 31 additions & 33 deletions flow/raster_cache_unittests.cc
Original file line number Diff line number Diff line change
@@ -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<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) {
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<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, 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<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_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