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

Eagerly clean removed image out of the texture cache. #2941

Merged
merged 2 commits into from Aug 3, 2018

Conversation

Projects
None yet
4 participants
@nical
Collaborator

nical commented Jul 31, 2018

This is the first step towards improving the use of dirty rects with async blobs.

This PR adds the concept of manual and automatic eviction strategies. Automatically evicted texture cache entries correspond to what we currently do where the texture cache evicts items that haven't been request for a number of frames, while manually evicted items never get evicted until the resource cache explicitly marks them as unused with TextureCache::mark_unused.

The manual eviction policy will be needed for blob images so that we get the guarantees that what's in the cache doesn't get evicted while rasterization is happening asynchronously, which we need for dirty rects to stay valid.

Non-blob items will remain automatically evicted. This PR also makes us evict removed image keys faster for all types of texture cache entries which is good since an existing texture cache entry for a removed image key will never be used again (might as well free space up in the cache as soon we know we won't use it again).


This change is Reviewable

@nical nical requested a review from kvark Jul 31, 2018

Show outdated Hide outdated webrender/src/texture_cache.rs
// Set a very low last accessed frame to make it very likely that this entry
// will get cleaned up next time we try to expire entries.
entry.last_access = FrameId(0);
entry.eviction = Eviction::Auto;

This comment has been minimized.

@kvark

kvark Jul 31, 2018

Member

given that last_access doesn't make sense for Manual anyway, can we make it a field of Eviction::Auto?

@kvark

kvark Jul 31, 2018

Member

given that last_access doesn't make sense for Manual anyway, can we make it a field of Eviction::Auto?

This comment has been minimized.

@nical

nical Jul 31, 2018

Collaborator

The Eviction enum was primarily meant to avoid adding a boolean parameter to the update function so I'd rather keep it this way.

@nical

nical Jul 31, 2018

Collaborator

The Eviction enum was primarily meant to avoid adding a boolean parameter to the update function so I'd rather keep it this way.

This comment has been minimized.

@kvark

kvark Jul 31, 2018

Member

I don't understand. If we can get a better semantics, better use of types, then why neglect it?

@kvark

kvark Jul 31, 2018

Member

I don't understand. If we can get a better semantics, better use of types, then why neglect it?

This comment has been minimized.

@nical

nical Jul 31, 2018

Collaborator

If you insist I can add another enum for the thing stored in here that has a last access time and keep this enum for the signature fo TextureCache::update. Where Eviction shows up most right now is as an argument in the various calls to TextureCache::update where the last access time member doesn't make sense (I think).

@nical

nical Jul 31, 2018

Collaborator

If you insist I can add another enum for the thing stored in here that has a last access time and keep this enum for the signature fo TextureCache::update. Where Eviction shows up most right now is as an argument in the various calls to TextureCache::update where the last access time member doesn't make sense (I think).

This comment has been minimized.

@kvark

kvark Aug 1, 2018

Member

ok, fair enough. I can follow up with this myself if I see it feasible

@kvark

kvark Aug 1, 2018

Member

ok, fair enough. I can follow up with this myself if I see it feasible

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 1, 2018

Contributor

I haven't read through this PR, but I'm a little unsure about it from the description.

One of the principles of the texture cache is that it's truly a cache, and thus it's expected that we can evict the whole thing at any time and things will "just work" correctly the next frame.

For example, if we experience memory pressure, or a device reset, we can just recreate the texture cache and everything will correctly be re-uploaded on the next frame transparently.

It sounds like these proposed changes may break that assumption? And if so, would that complicate things such as device reset etc?

Contributor

gw3583 commented Aug 1, 2018

I haven't read through this PR, but I'm a little unsure about it from the description.

One of the principles of the texture cache is that it's truly a cache, and thus it's expected that we can evict the whole thing at any time and things will "just work" correctly the next frame.

For example, if we experience memory pressure, or a device reset, we can just recreate the texture cache and everything will correctly be re-uploaded on the next frame transparently.

It sounds like these proposed changes may break that assumption? And if so, would that complicate things such as device reset etc?

@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Aug 1, 2018

Collaborator

It sounds like these proposed changes may break that assumption? And if so, would that complicate things such as device reset etc?

Not necessarily: for memory pressures or device loss we can detect it and throw away the rendered blobs that only have a sub-rect available (that would fall back to synchronous rasterization on the next frame but look correct). That way we could still throw away manually evicted items if we want to.

If we don't have a way to maintain blobs in the cache, we can't rasterize sub-rects asynchronously, since doing so relies on the fact that the rest of the blob is in the cache and will still be there by the time we are done rasterizing.

Collaborator

nical commented Aug 1, 2018

It sounds like these proposed changes may break that assumption? And if so, would that complicate things such as device reset etc?

Not necessarily: for memory pressures or device loss we can detect it and throw away the rendered blobs that only have a sub-rect available (that would fall back to synchronous rasterization on the next frame but look correct). That way we could still throw away manually evicted items if we want to.

If we don't have a way to maintain blobs in the cache, we can't rasterize sub-rects asynchronously, since doing so relies on the fact that the rest of the blob is in the cache and will still be there by the time we are done rasterizing.

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Aug 1, 2018

Member

Given the concern raised, leaving for @gw3583 to merge.

Member

kvark commented Aug 1, 2018

Given the concern raised, leaving for @gw3583 to merge.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 2, 2018

Contributor

OK, I think that sounds reasonable, as long as it's easy to detect those cases. I guess we'll just need to add a bit of code to the message that clears the caches to ensure that we hook up this logic to the blob code?

Contributor

gw3583 commented Aug 2, 2018

OK, I think that sounds reasonable, as long as it's easy to detect those cases. I guess we'll just need to add a bit of code to the message that clears the caches to ensure that we hook up this logic to the blob code?

@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Aug 2, 2018

Collaborator

All we need is to throw away the rasterized blobs and the code that queries images during frame building will fallback to rasterizing the blobs it can't find synchronously on demand.

Collaborator

nical commented Aug 2, 2018

All we need is to throw away the rasterized blobs and the code that queries images during frame building will fallback to rasterizing the blobs it can't find synchronously on demand.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 2, 2018

Contributor

@nical Sounds good to me then!

Contributor

gw3583 commented Aug 2, 2018

@nical Sounds good to me then!

@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Aug 3, 2018

Collaborator

@bors-servo r=gw3583

Collaborator

nical commented Aug 3, 2018

@bors-servo r=gw3583

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 3, 2018

Contributor

📌 Commit 2cca059 has been approved by gw3583

Contributor

bors-servo commented Aug 3, 2018

📌 Commit 2cca059 has been approved by gw3583

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 3, 2018

Contributor

⌛️ Testing commit 2cca059 with merge 81005df...

Contributor

bors-servo commented Aug 3, 2018

⌛️ Testing commit 2cca059 with merge 81005df...

bors-servo added a commit that referenced this pull request Aug 3, 2018

Auto merge of #2941 - nical:tex-cache-eviction, r=gw3583
Eagerly clean removed image out of the texture cache.

This is the first step towards improving the use of dirty rects with async blobs.

This PR adds the concept of manual and automatic eviction strategies. Automatically evicted texture cache entries correspond to what we currently do where the texture cache evicts items that haven't been request for a number of frames, while manually evicted items never get evicted until the resource cache explicitly marks them as unused with `TextureCache::mark_unused`.

The manual eviction policy will be needed for blob images so that we get the guarantees that what's in the cache doesn't get evicted while rasterization is happening asynchronously, which we need for dirty rects to stay valid.

Non-blob items will remain automatically evicted. This PR also makes us evict removed image keys faster for all types of texture cache entries which is good since an existing texture cache entry for a removed image key will never be used again (might as well free space up in the cache as soon we know we won't use it again).

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

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 3, 2018

Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing 81005df to master...

Contributor

bors-servo commented Aug 3, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing 81005df to master...

@bors-servo bors-servo merged commit 2cca059 into servo:master Aug 3, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

bors-servo added a commit that referenced this pull request Aug 3, 2018

Auto merge of #2948 - nical:blob-dirty-rect, r=kvark
Only paint the dirty rect of non-tiled blobs.

Unless we settle with a small tile size we should follow up with the same optimization for tiled blobs.
This is built on top of #2941.

<!-- 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/2948)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment