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

Skip evicting retained tiles that are currently not visible. #3465

Merged
merged 1 commit into from Jan 4, 2019

Conversation

Projects
None yet
3 participants
@gw3583
Copy link
Collaborator

gw3583 commented Jan 4, 2019

The code in pre_update selects a desired tile grid, based on the
currently visible world rect.

Any tiles retained after this point should try to retain their backing
texture, even if they are currently off-screen, since it's possible
they will be scrolled back into the visible rect soon.

Once it's determined they are far enough away from the visible
rect during pre_update, they will be dropped and evicted eagerly.


This change is Reviewable

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 4, 2019

r? @bholley or anyone

@bholley
Copy link
Contributor

bholley left a comment

r=me with those things fixed.

None => Tile::new(),
Some(tile) => {
tile
}

This comment has been minimized.

@bholley

bholley Jan 4, 2019

Contributor

This indentation is weird - does rustfmt require it? I'd think that this could still just be => tile, without braces.

This comment has been minimized.

@gw3583

gw3583 Jan 4, 2019

Collaborator

Fixed.

@@ -118,11 +122,15 @@ pub struct Tile {
/// cache handle can be used. Tiles are invalidated during the
/// build_dirty_regions method.
is_valid: bool,
/// The tile id is stable between display lists and / or frames,
/// if the tile is retained. Useful for debugging tile evictions.
id: TileId,

This comment has been minimized.

@bholley

bholley Jan 4, 2019

Contributor

I assume there are few enough tiles in the system and they're moved infrequently enough that an extra word for debugging has negligible overhead?

This comment has been minimized.

@gw3583

gw3583 Jan 4, 2019

Collaborator

Yea, typically 10 - 20 tiles ever active.

None => {
let id = TileId(self.next_id);
self.next_id += 1;
Tile::new(id)

This comment has been minimized.

@bholley

bholley Jan 4, 2019

Contributor

I think it would be a bit cleaner to make next_id a method and then this could just be Tile::new(self.next_id()).

This comment has been minimized.

@gw3583

gw3583 Jan 4, 2019

Collaborator

Done

// during pre_update. If a tile still exists after that, we are
// assuming that it's either visible or we want to retain it for
// a while in case it gets scrolled back onto screen soon.
resource_cache.texture_cache.request(&tile.handle, gpu_cache);

This comment has been minimized.

@bholley

bholley Jan 4, 2019

Contributor

I think it would be clearer if this were in an else block, since it's a no-op in the already-evicted case (handled above), but that's not obvious to readers.

This comment has been minimized.

@bholley

bholley Jan 4, 2019

Contributor

Also, it seems like what we really want here is EvictionPolicy::Manual, which would let us skip this work, and trigger faster eviction of the tiles we don't care about.

I won't insist you do this now, but add a TODO?

This comment has been minimized.

@gw3583

gw3583 Jan 4, 2019

Collaborator

Made it an if/else block, and added a TODO. I'm not sure about manual though - it seems like eager is good enough, and also gives the texture cache more flexibility to evict stuff in low memory situations etc.

Skip evicting retained tiles that are currently not visible.
The code in pre_update selects a desired tile grid, based on the
currently visible world rect.

Any tiles retained after this point should try to retain their backing
texture, even if they are currently off-screen, since it's possible
they will be scrolled back into the visible rect soon.

Once it's determined they are far enough away from the visible
rect during pre_update, they will be dropped and evicted eagerly.
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 4, 2019

@bors-servo r=bholley (via email)

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 4, 2019

📌 Commit ebcce70 has been approved by bholley

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 4, 2019

⌛️ Testing commit ebcce70 with merge 8a7212b...

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

Auto merge of #3465 - gw3583:less-evicts, r=bholley
Skip evicting retained tiles that are currently not visible.

The code in pre_update selects a desired tile grid, based on the
currently visible world rect.

Any tiles retained after this point should try to retain their backing
texture, even if they are currently off-screen, since it's possible
they will be scrolled back into the visible rect soon.

Once it's determined they are far enough away from the visible
rect during pre_update, they will be dropped and evicted eagerly.

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 4, 2019

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: bholley
Pushing 8a7212b to master...

@bors-servo bors-servo merged commit ebcce70 into servo:master Jan 4, 2019

3 checks passed

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 4, 2019

Bug 1517722 - Update webrender to commit 8a7212b628ae39e7251201b0a976…
…1c74bab42c5d (WR PR #3465). r=kats

servo/webrender#3465

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

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

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

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