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

Only cache tiles when they have had the same content > 2 frames. #3487

Merged
merged 3 commits into from Jan 9, 2019

Conversation

Projects
None yet
4 participants
@gw3583
Copy link
Collaborator

gw3583 commented Jan 8, 2019

This prevents tile caching doing redundant blits if the content
is changing every frame. This fixes a lot of the performance
regression on pages / benchmarks that have content which is
animated / changing every frame.

It still allows caching of tiles within those pages that are
static, giving the best of both worlds.


This change is Reviewable

gw3583 added some commits Jan 8, 2019

Fix handling of world clip rects when APZ is occurring.
During APZ, it's possible to receive a new display with effectively
the same content, but with display items at different local
positions.

Handling this with world clip rects, while not invalidating tiles
unnecessarily as they change relative to scrolling tiles is
quite subtle. This patch fixes these by maintaining the region
of the primitive in its primitive-relative space that affects
the tile (similar to how primitive interning stores rects as
primitive relative).
Only cache tiles when they have had the same content > 2 frames.
This prevents tile caching doing redundant blits if the content
is changing every frame. This fixes a lot of the performance
regression on pages / benchmarks that have content which is
animated / changing every frame.

It still allows caching of tiles within those pages that are
static, giving the best of both worlds.
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 8, 2019

r? @kvark or @bholley

This includes #3482, which can be reviewed separately or as part of this PR.

This is a potential fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1518406, https://bugzilla.mozilla.org/show_bug.cgi?id=1514539 and https://bugzilla.mozilla.org/show_bug.cgi?id=1518407, and is just generally a sensible thing to do. Having said that, the reported performance regressions in those bugs are severe, and don't match what I see with picture caching, so still warrant further investigation.

Pending try (pic caching enabled):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdca53d341eb9b2afd88b1f06f9db9091a74647c

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 8, 2019

Try run looks good.

@kvark

kvark approved these changes Jan 8, 2019

Copy link
Member

kvark left a comment

no serious concerns here 👍

/// Return true if the content of the tile is the same
/// as last frame. This doesn't check validity of the
/// tile based on the currently valid regions.
fn is_same_content(&self) -> bool {

This comment has been minimized.

@kvark

kvark Jan 8, 2019

Member

hm, this doesn't appear to contain any actual comparisons, does it?

// Only cache tiles that have had the same content for at least two
// frames. This skips caching on pages / benchmarks that are changing
// every frame, which is wasteful.
if tile.same_frames > 2 {

This comment has been minimized.

@kvark

kvark Jan 8, 2019

Member

would be good to move this constant up and define separately

@gw3583
Copy link
Collaborator

gw3583 left a comment

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @kvark)


webrender/src/picture.rs, line 288 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

hm, this doesn't appear to contain any actual comparisons, does it?

The is_valid method returns the current comparison state based on items being pushed into the comparable vec.


webrender/src/picture.rs, line 1070 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would be good to move this constant up and define separately

Done

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 9, 2019

Pushed a couple of fixes and addressed the comments in each PR.

Thanks for the review!

@staktrace

This comment has been minimized.

Copy link
Contributor

staktrace commented Jan 9, 2019

@gw3583 is this good to land now?

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 9, 2019

@staktrace I think so - there are a couple of comments from #3482 that need are not completely resolved, but I think it's fine to do them as follow ups.

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

📌 Commit eab071d has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

⌛️ Testing commit eab071d with merge fb4b934...

bors-servo added a commit that referenced this pull request Jan 9, 2019

Auto merge of #3487 - gw3583:less-caching, r=kvark
 Only cache tiles when they have had the same content > 2 frames.

This prevents tile caching doing redundant blits if the content
is changing every frame. This fixes a lot of the performance
regression on pages / benchmarks that have content which is
animated / changing every frame.

It still allows caching of tiles within those pages that are
static, giving the best of both worlds.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3487)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing fb4b934 to master...

@bors-servo bors-servo merged commit eab071d into servo:master Jan 9, 2019

3 of 4 checks passed

code-review/reviewable 1 file, 2 discussions left (kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 9, 2019

Bug 1518708 - Update webrender to commit fb4b9342aa1b047ac46b89cb7a70…
…987816bee686 (WR PR #3487). r=kats

servo/webrender#3487

Differential Revision: https://phabricator.services.mozilla.com/D16009

--HG--
extra : moz-landing-system : lando

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Jan 9, 2019

@kvark
Copy link
Member

kvark left a comment

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

staktrace added a commit to staktrace/webrender that referenced this pull request Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment