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

Allow picture caching to work across scenes (display lists). #3359

Merged
merged 2 commits into from Nov 28, 2018

Conversation

@gw3583
Copy link
Collaborator

commented Nov 27, 2018

This patch hooks up the primitive interning work with the tile
caching work.

Since we have a uid that guarantees uniqueness of the content of
primitives and clip nodes, we can use that to construct a
(reasonably) efficient hash key for the content of a tile.

Along with some additional information (such as the state of the
transforms used by the tile), we can construct a hash key for
the content of a tile that is stable across new display lists,
even if the shape of the clip-scroll tree changes in ways that
don't affect the tile.

This patch takes advantage of that to retain tiles when a new
scene is built, where the content of a tile would result in
the same output.


This change is Reviewable

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 27, 2018

r? @kvark or @nical

This gives us the ability to cache picture tiles between both frames and scenes.

This is a somewhat more interesting patch than all the previous primitive interning changes :)

Note that it's still not actually hooked up to create pictures with tile caches enabled (in master), so this shouldn't have any effect on current gecko.

Nonetheless, pending try run to be safe:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0432779ca9a8f9ddd7fb6b5c932b7f72dba6a2b8

Copy link
Member

left a comment

This is a somewhat more interesting patch than all the previous primitive interning changes :)

indeed! :)

a few concerns below

Reviewed 6 of 7 files at r1.
Reviewable status: 6 of 7 files reviewed, 10 unresolved discussions (waiting on @gw3583)


webrender/src/batch.rs, line 1004 at r1 (raw file):

                                        // Check if the tile is visible.
                                        if !tile.is_visible || !tile.in_use {

how is it possible for a tile to be visible but not used?


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

    /// if the content of the tile is the same, but the shape of the
    /// clip-scroll tree changes between scenes in other areas.
    tile_transform_map: FastHashMap<SpatialNodeIndex, TileTransformIndex>,

It's not clear to me why this is needed. My basic understanding was that when a tile is cached for a particular surface, it doesn't depend on the spatial node of this surface, but it depends on all the children spatial nodes that affect primitives intersecting with the tile. Is there ever a case where just checking for is_child() is insufficient or inefficient w.r.t. caching?


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

        resource_cache: &ResourceCache,
    ) -> Option<(TileDescriptor, TextureCacheHandle)> {
        if self.is_valid && resource_cache.texture_cache.is_allocated(&self.handle) {

is it possible for a tile to be valid but not allocated in the texture cache?


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

                None => true,
            };
            self.opacity_bindings.insert(*id, OpacityBindingInfo {

this hashmap is getting grown from zero every time, which is unfortunate. We might want to re-use the existing allocation somehow


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

                if let TransformKey::ScaleOffset(ref mut key) = key {
                    if raster_spatial_node.coordinate_system_id == surface_spatial_node.coordinate_system_id {

We shouldn't compare those and even access the raster_spatial_node and surface_spatial_node directly here. The returned ScaleOffset guarantees that the coordinate systems are matching


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

                    // exact content of this tile.
                    if let Some(handle) = retained_tiles.remove(&tile.descriptor) {
                        // Only use if not evicted from texture cache in the meantime.

if it was evicted, do we still want to request it? I thought we are going to re-render it instead


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

        //           some cases?
        for (_, handle) in retained_tiles.drain() {
            if resource_cache.texture_cache.is_allocated(&handle) {

sounds like mark_unused could be doing the is_allocated check internally


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

            debug_assert!(tile_cache.old_tiles.is_empty());
            for tile in tile_cache.tiles {
                if let Some((descriptor, texture_handle)) = tile.destroy(resource_cache) {

nit: could be retained_tiles.extend(tile.destroy(resource_cache))


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

                                        None,
                                        UvRectKind::Rect,
                                        Eviction::Auto,

is this changed because we are now manually tracking the tile cache lifetimes?


webrender/src/prim_store.rs, line 2618 at r1 (raw file):

    /// Destroy an existing primitive store. This is called just before
    /// a primitive store is replaced with a newly built scene.
    pub fn destroy(

for each object with a similar destroy we should consider having some sort of a safeguard check in drop() to ensure that it was destroyed properly

gw3583 added 2 commits Nov 27, 2018
This patch hooks up the primitive interning work with the tile
caching work.

Since we have a uid that guarantees uniqueness of the content of
primitives and clip nodes, we can use that to construct a
(reasonably) efficient hash key for the content of a tile.

Along with some additional information (such as the state of the
transforms used by the tile), we can construct a hash key for
the content of a tile that is stable across new display lists,
even if the shape of the clip-scroll tree changes in ways that
don't affect the tile.

This patch takes advantage of that to retain tiles when a new
scene is built, where the content of a tile would result in
the same output.
Copy link
Collaborator Author

left a comment

Reviewable status: 6 of 7 files reviewed, 10 unresolved discussions (waiting on @kvark)


webrender/src/batch.rs, line 1004 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

how is it possible for a tile to be visible but not used?

If the tile is within the 2d grid boundaries and is visible, but doesn't currently have any primitives in it.


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

Previously, kvark (Dzmitry Malyshau) wrote…

It's not clear to me why this is needed. My basic understanding was that when a tile is cached for a particular surface, it doesn't depend on the spatial node of this surface, but it depends on all the children spatial nodes that affect primitives intersecting with the tile. Is there ever a case where just checking for is_child() is insufficient or inefficient w.r.t. caching?

The goal here is to ensure that the node indices for a given tile are stable even if the clip-scroll tree itself looks quite different between display lists, so that the hash key is the same. It's quite subtle, and quite possible I'm missing something here too - perhaps worth discussing on IRC.


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

Previously, kvark (Dzmitry Malyshau) wrote…

is it possible for a tile to be valid but not allocated in the texture cache?

No, that shouldn't be possible. Fixed.


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

Previously, kvark (Dzmitry Malyshau) wrote…

this hashmap is getting grown from zero every time, which is unfortunate. We might want to re-use the existing allocation somehow

It is unfortunate. On the plus side, opacity bindings are very rare and I think we can remove them completely once picture caching is in place.


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

Previously, kvark (Dzmitry Malyshau) wrote…

We shouldn't compare those and even access the raster_spatial_node and surface_spatial_node directly here. The returned ScaleOffset guarantees that the coordinate systems are matching

Good point! Fixed.


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

Previously, kvark (Dzmitry Malyshau) wrote…

if it was evicted, do we still want to request it? I thought we are going to re-render it instead

The request (perhaps badly named) just updates the epoch and timestamp fields if it is allocated.


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

Previously, kvark (Dzmitry Malyshau) wrote…

sounds like mark_unused could be doing the is_allocated check internally

Ah, it does already check for a valid handle - so I removed the is_allocated test.


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

Previously, kvark (Dzmitry Malyshau) wrote…

nit: could be retained_tiles.extend(tile.destroy(resource_cache))

Fixed.


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

Previously, kvark (Dzmitry Malyshau) wrote…

is this changed because we are now manually tracking the tile cache lifetimes?

Having it as eager makes it easier to see in the texture cache debugger that tiles are being dropped, but it's overkill. Leaving it as Auto just means that the texture cache can evict when it wants to, but doesn't need to evict as soon as the tile is unused, if there's no GPU memory pressure.


webrender/src/prim_store.rs, line 2618 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

for each object with a similar destroy we should consider having some sort of a safeguard check in drop() to ensure that it was destroyed properly

That sounds like a good idea - it could be similar to the drop for GL resources in device.

@gw3583 gw3583 force-pushed the gw3583:c8 branch from 5e6314f to d1e1976 Nov 27, 2018
@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 27, 2018

@kvark Thanks for the review! Addressed those comments, and rebased on current master. Try run looks good (the failures are expected given previous PRs pending update in m-c).

@kvark
kvark approved these changes Nov 28, 2018
Copy link
Member

left a comment

:lgtm:

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


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

Previously, gw3583 (Glenn Watson) wrote…

The goal here is to ensure that the node indices for a given tile are stable even if the clip-scroll tree itself looks quite different between display lists, so that the hash key is the same. It's quite subtle, and quite possible I'm missing something here too - perhaps worth discussing on IRC.

yeah, I'm just having a hard time imagining that use case


webrender/src/prim_store.rs, line 2618 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

That sounds like a good idea - it could be similar to the drop for GL resources in device.

added to #3357 (comment)

@kvark

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

📌 Commit d1e1976 has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

⌛️ Testing commit d1e1976 with merge 3d73e38...

bors-servo added a commit that referenced this pull request Nov 28, 2018
Allow picture caching to work across scenes (display lists).

This patch hooks up the primitive interning work with the tile
caching work.

Since we have a uid that guarantees uniqueness of the content of
primitives and clip nodes, we can use that to construct a
(reasonably) efficient hash key for the content of a tile.

Along with some additional information (such as the state of the
transforms used by the tile), we can construct a hash key for
the content of a tile that is stable across new display lists,
even if the shape of the clip-scroll tree changes in ways that
don't affect the tile.

This patch takes advantage of that to retain tiles when a new
scene is built, where the content of a tile would result in
the same output.

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

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

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

@bors-servo bors-servo merged commit d1e1976 into servo:master Nov 28, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 2 discussions left (gw3583)
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 Nov 29, 2018
…73abdb59e76d (WR PR #3359). r=kats

servo/webrender#3359

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

--HG--
extra : moz-landing-system : lando
jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Nov 29, 2018
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…73abdb59e76d (WR PR #3359). r=kats

servo/webrender#3359

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

UltraBlame original commit: 8692990356a7e39a67ee65909ff01f621d0830fb
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…73abdb59e76d (WR PR #3359). r=kats

servo/webrender#3359

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

UltraBlame original commit: 8692990356a7e39a67ee65909ff01f621d0830fb
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…73abdb59e76d (WR PR #3359). r=kats

servo/webrender#3359

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

UltraBlame original commit: 8692990356a7e39a67ee65909ff01f621d0830fb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.