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

Picture caching fixes and clean ups. #3413

Merged
merged 4 commits into from Dec 14, 2018

Conversation

Projects
None yet
4 participants
@gw3583
Copy link
Collaborator

gw3583 commented Dec 13, 2018

From the commits:

Store primitive instance origins in the tile descriptors.

This is necessary to uniquely identify a primitive instance, now
that we store primitive templates in a primitive-local space.
Switch default tile size to 1024 x 256.

This seems better suited for most pages I've tested on so far.

It also ensures that cache tiles are allocated as standalone
textures, rather than layers inside the texture cache. The current
texture cache grow / shrink logic was not handling the picture
cache allocation strategy very well, resulting in a lot of GPU
spikes as the texture cache arrays were being resized.

In future, we might consider a specialized cache texture for these
tiles that is, for example, 4096 x 4096. This would allow better
batching of draw calls for tiles when drawing the final scene.
Tidy up how tile rect coords are calculated. 

This change is Reviewable

gw3583 added some commits Dec 13, 2018

Store primitive instance origins in the tile descriptors.
This is necessary to uniquely identify a primitive instance, now
that we store primitive templates in a primitive-local space.
Switch default tile size to 1024 x 256.
This seems better suited for most pages I've tested on so far.

It also ensures that cache tiles are allocated as standalone
textures, rather than layers inside the texture cache. The current
texture cache grow / shrink logic was not handling the picture
cache allocation strategy very well, resulting in a lot of GPU
spikes as the texture cache arrays were being resized.

In future, we might consider a specialized cache texture for these
tiles that is, for example, 4096 x 4096. This would allow better
batching of draw calls for tiles when drawing the final scene.
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 13, 2018

r? @kvark or anyone

Improve how display list flattening defines tile cache pictures.
This is both a bug fix (when opening devtools with picture caching
enabled) and also a much less hacky way to define where tile
cache picture(s) exist (although still not ideal).

Now, we only create a tile cache for top level (content) iframes.

When creating the tile cache for an iframe, skip any preceding
or trailing primitives that are not part of the main scroll root.
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 14, 2018

Added a follow up commit:

Improve how display list flattening defines tile cache pictures.

This is both a bug fix (when opening devtools with picture caching
enabled) and also a much less hacky way to define where tile
cache picture(s) exist (although still not ideal).

Now, we only create a tile cache for top level (content) iframes.

When creating the tile cache for an iframe, skip any preceding
or trailing primitives that are not part of the main scroll root.
@kvark

kvark approved these changes Dec 14, 2018

Copy link
Member

kvark left a comment

:lgtm:

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


webrender/src/display_list_flattener.rs, line 264 at r2 (raw file):

        let mut main_scroll_root = None;

        let first_index = primitives.iter().position(|instance| {

nit: using find_map here on an enumerated iterator could make this a bit cleaner


webrender/src/picture.rs, line 259 at r2 (raw file):

    origin: PointKey,
    /// The first clip in the clip_uids array of clips that affect this tile.
    first_clip: u16,

nit: use Range?

@gw3583
Copy link
Collaborator

gw3583 left a comment

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


webrender/src/display_list_flattener.rs, line 264 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: using find_map here on an enumerated iterator could make this a bit cleaner

I have some other patches coming to this code shortly, so I'll switch to using that locally.


webrender/src/picture.rs, line 259 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: use Range?

I was going to, but that is a slight extra cost (adding the first + 1 to get the end) so it didn't seem worth it in this case.

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 14, 2018

(merging now, assuming that the tick of approval means you're not too concerned about those nits).

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 14, 2018

📌 Commit 3628139 has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 14, 2018

⌛️ Testing commit 3628139 with merge d2c673a...

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

Auto merge of #3413 - gw3583:more-cache-fixes, r=kvark
Picture caching fixes and clean ups.

From the commits:

```
Store primitive instance origins in the tile descriptors.

This is necessary to uniquely identify a primitive instance, now
that we store primitive templates in a primitive-local space.
```

```
Switch default tile size to 1024 x 256.

This seems better suited for most pages I've tested on so far.

It also ensures that cache tiles are allocated as standalone
textures, rather than layers inside the texture cache. The current
texture cache grow / shrink logic was not handling the picture
cache allocation strategy very well, resulting in a lot of GPU
spikes as the texture cache arrays were being resized.

In future, we might consider a specialized cache texture for these
tiles that is, for example, 4096 x 4096. This would allow better
batching of draw calls for tiles when drawing the final scene.
```

```
Tidy up how tile rect coords are calculated.
```

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 14, 2018

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

@bors-servo bors-servo merged commit 3628139 into servo:master Dec 14, 2018

3 of 4 checks passed

code-review/reviewable 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 Dec 14, 2018

Bug 1514180 - Update webrender to commit d2c673ada607f29846c3d1ac8ca7…
…d2b272ba1b2d (WR PR #3413). r=kats

servo/webrender#3413

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

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

escapewindow pushed a commit to escapewindow/gecko that referenced this pull request Dec 14, 2018

@bholley

This comment has been minimized.

Copy link
Contributor

bholley commented Dec 17, 2018

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