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

Add picture caching support. #3379

Merged
merged 3 commits into from Dec 1, 2018

Conversation

Projects
None yet
4 participants
@gw3583
Copy link
Collaborator

gw3583 commented Dec 1, 2018

This change is Reviewable

gw3583 added some commits Dec 1, 2018

Fix a number of bugs related to the tile cache.
* Fix panic where update_transforms would try to calculate transforms with invalid child/parent relationship.
* Fix extra invalidation where retained tiles didn't have the right tile size / raster fields in descriptors.
* Fix panic where primitive rect was incorrectly calculated outside the bounds of the tile cache.
* Fix invalidation of unused tiles that exist in the grid.
* Fix dirty rect calculation including unused tiles.
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 1, 2018

r? @kvark

This patch has 3 commits - the first two are simple bug fixes and functionality, and shouldn't be controversial.

The last commit has a couple of horrible hacks in it to enable the first version of picture caching (when the renderer option is enabled, which is off by default), where the code relies on knowledge of what the Gecko display list looks like.

I don't like this change at all, but I think that getting it merged while we work out a better solution (during the all-hands) has a number of benefits:

  • It's off by default, so shouldn't affect anyone.
  • It lets us dogfood the first iteration of picture caching internally by flipping a preference, which allows us to find bugs, compare performance etc.
@kvark

kvark approved these changes Dec 1, 2018

Copy link
Member

kvark left a comment

hopefully, this is temporary :)

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gw3583)


webrender/src/display_list_flattener.rs, line 252 at r3 (raw file):

        }

        // This method is basically a hack to set up picture caching in a minimal

I can see how all of this would be removed if Gecko could just tell us what to cache


webrender/src/display_list_flattener.rs, line 377 at r3 (raw file):

        while node_index != ROOT_SPATIAL_NODE_INDEX {
            let node = &self.clip_scroll_tree.spatial_nodes[node_index.0];

if we are to keep this hack, might want to move it in sode the clip_scroll_tree


webrender/src/display_list_flattener.rs, line 387 at r3 (raw file):

                    // and keep looking up the tree.
                    if let ScrollFrameKind::Explicit = info.frame_kind {
                        scroll_root = node_index;

don't we need to break here?

@gw3583
Copy link
Collaborator

gw3583 left a comment

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kvark)


webrender/src/display_list_flattener.rs, line 252 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I can see how all of this would be removed if Gecko could just tell us what to cache

Yup, it's fairly isolated at least :)


webrender/src/display_list_flattener.rs, line 377 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if we are to keep this hack, might want to move it in sode the clip_scroll_tree

Yea, that'd be a good idea. If we keep it, we should also add caching since the vast majority of the time the spatial node doesn't change between primitives.


webrender/src/display_list_flattener.rs, line 387 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

don't we need to break here?

No, we want to keep walking up the tree to find the closest scroll root to the root node (since we're not creating caches for sub-scroll roots at this point). It won't pick the scroll root in the root pipeline, since that's not an Explicit scroll frame.

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 1, 2018

@kvark Thank you for the quick review! 👍

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Dec 1, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 1, 2018

📌 Commit 2c89d39 has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 1, 2018

⌛️ Testing commit 2c89d39 with merge 6ca6341...

bors-servo added a commit that referenced this pull request Dec 1, 2018

Auto merge of #3379 - gw3583:c30, r=kvark
Add picture caching support.

<!-- 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/3379)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 1, 2018

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

@bors-servo bors-servo merged commit 2c89d39 into servo:master Dec 1, 2018

3 of 4 checks passed

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

@gw3583 gw3583 deleted the gw3583:c30 branch Dec 1, 2018

@pcwalton

This comment has been minimized.

Copy link
Collaborator

pcwalton commented Dec 2, 2018

@gw3583 Did you get a chance to profile this by any chance? I'm curious what kinds of perf improvements we saw, if any. (I wouldn't expect much on most sites, based on our experience, but it'd be good to confirm.)

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 2, 2018

@pcwalton Nothing detailed yet - anecdotally it does seem like quite a win on some sites, but I need to do some proper profiling next week.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 2, 2018

Bug 1511664 - Update webrender to commit 6ca634197cfe26738194f8704202…
…0fe838c0047a (WR PR #3379). r=kats

servo/webrender#3379

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

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

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 2, 2018

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