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.

Sign up for GitHub

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

Jump to bottom

Fix RasterCache LRU logic + opportunistic simplifications. #16434

Merged
merged 3 commits into from
Feb 6, 2020
Merged

Fix RasterCache LRU logic + opportunistic simplifications. #16434

merged 3 commits into from
Feb 6, 2020

Conversation

ignatz
Copy link
Contributor

@ignatz ignatz commented Feb 6, 2020

Without this fix it's possible to sweep/invalidate cache entries of items still shown on screen.

@auto-assign auto-assign bot requested a review from franciscojma86 February 6, 2020 00:05
@ignatz
Copy link
Contributor Author

ignatz commented Feb 6, 2020

@cbracken

@ignatz ignatz requested a review from cbracken February 6, 2020 00:22
@cbracken cbracken requested a review from chinmaygarde February 6, 2020 01:25
@cbracken
Copy link
Member

cbracken commented Feb 6, 2020

/cc @chinmaygarde: replaces #16431 which covers a subset.

@ignatz
Copy link
Contributor Author

ignatz commented Feb 6, 2020 via email

@cbracken
Copy link
Member

cbracken commented Feb 6, 2020

Ah I see now. Agreed this is super common with caching, just wasn't seeing the addition of a direct non-const call on the cache itself, but yep the fetch of the entry off the (otherwise) const iterator.

liyuqian
liyuqian reviewed Feb 6, 2020
View reviewed changes
Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thank you very much for helping us fix this Flutter issue!

+1 to @chinmaygarde 's request on adding a unit test that would fail without this PR.

See sample unit tests in raster_cache_unittests.cc. I'm glad that we wrote ThresholdIsRespectedWhenZero test :)

flow/raster_cache.cc Show resolved Hide resolved
Redefine access threshold semantics to reflect the LRU cache semantic…
f0c48ab 
…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.
@ignatz
Copy link
Contributor Author

ignatz commented Feb 6, 2020

I'd also happily move the Rasterization step out of the Prepare into the Accessor to make the access_threshold perfectly semantically meaningful.

cbracken
cbracken approved these changes Feb 6, 2020
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM stamp from a Japanese personal seal

@cbracken cbracken merged commit 80be2c4 into flutter:master Feb 6, 2020
@cbracken cbracken mentioned this pull request Feb 6, 2020
Closed
@liyuqian
Copy link
Contributor

liyuqian commented Feb 7, 2020 •
edited
Loading

I'd also happily move the Rasterization step out of the Prepare into the Accessor to make the access_threshold perfectly semantically meaningful.

@ignatz : we did rasterization in Prepare instead of Get to save one render target switch and improve the performance.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 7, 2020
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 7, 2020
@engine-flutter-autoroll
80be2c4 Fix RasterCache LRU logic, opportunistic simplifications. (fl…
d3f5a6f 
…utter/engine#16434)
dnfield pushed a commit to flutter/flutter that referenced this pull request Feb 7, 2020
@engine-flutter-autoroll
Roll engine f3ce90e..580503c (24 commits) (#50363)
ef74572 
* 71ec0a1 Relax timing restrictions on WakeUpTimersAreSingletons. (flutter/engine#16446)

* 3ac1e6d Disable unit tests using --gtest-filter instead of at compile time (flutter/engine#16472)

* 80be2c4 Fix RasterCache LRU logic, opportunistic simplifications. (flutter/engine#16434)

* ca40f11 Roll src/third_party/skia 7f36405ea3ec..c0360582d211 (6 commits) (flutter/engine#16476)

* c9e7713 Fix analyzer warnings for frontend_server change (flutter/engine#16470)

* 9ada1b0 Fix elf_loader.cc on Fuchsia, add a TODO for proper fix

* 0517627 Enable runtime_unittests on Fuchsia

* 03f639e Add noexcept annotations to EnableValue moves (flutter/engine#16478)

* 00904dd Various fixes in CanvasKit (flutter/engine#16433)

* de7022b Roll src/third_party/skia c0360582d211..121750c2efff (7 commits) (flutter/engine#16479)

* d2aab27 Enable shell_unittests on Fuchsia with Vulkan dependencies. (flutter/engine#16376)

* eec73e3 Roll src/third_party/dart b3396cbdcae1..49850e6919f7 (45 commits) (flutter/engine#16480)

* 557f3a2 Run Flutter framework tests against the web engine in Cirrus (flutter/engine#16343)

* 477527b Roll src/third_party/skia 121750c2efff..046f9893b953 (4 commits) (flutter/engine#16482)

* eb8691f Code cleanup on destructors (flutter/engine#16481)

* 7397817 Roll fuchsia/sdk/core/linux-amd64 from A9STP... to g2s3c... (flutter/engine#16484)

* 313527d Roll src/third_party/dart 49850e6919f7..16782e6c171f (16 commits) (flutter/engine#16485)

* 83feaf4 Roll src/third_party/skia 046f9893b953..97bf6578796c (1 commits) (flutter/engine#16486)

* 9184058 Roll src/third_party/skia 97bf6578796c..f3560b680e35 (1 commits) (flutter/engine#16487)

* 0db017d Roll src/third_party/dart 16782e6c171f..d765d237460d (1 commits) (flutter/engine#16488)

* f9ed07c Roll fuchsia/sdk/core/linux-amd64 from g2s3c... to LvSlH... (flutter/engine#16489)

* a1b91da Roll src/third_party/skia f3560b680e35..77fdf66946d2 (1 commits) (flutter/engine#16490)

* 7edb803 Roll src/third_party/dart d765d237460d..514a8d4c8417 (7 commits) (flutter/engine#16491)

* 580503c Roll src/third_party/skia 77fdf66946d2..87e3bef6f82f (2 commits) (flutter/engine#16492)
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
@ignatz @NoamDev
Fix RasterCache LRU logic, opportunistic simplifications. (flutter#16434
e9558d4 
)

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).
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
@NoamDev
Revert "Fix RasterCache LRU logic, opportunistic simplifications. (fl…
9169fc1 
…utter#16434)"

This reverts commit e9558d4.
@cbracken cbracken mentioned this pull request Apr 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liyuqian liyuqian liyuqian left review comments

@cbracken cbracken cbracken approved these changes

@franciscojma86 franciscojma86 Awaiting requested review from franciscojma86

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ignatz @cbracken @liyuqian @googlebot