Bug 74152 – [chromium] Exposing required/preferred texture priorities from TextureManager

• Status: CLOSED
• Resolution: INVALID
• Component: New Bugs

[Eric Penner]

[chromium] Exposing required/preferred texture priorities from TextureManager/ManagedTexture.

[Eric Penner]

Created attachment 119067 [details]
Patch

[Eric Penner]

This patch makes it possible to expand the pre-paint region to be arbitrarily large. There are two main issues with that: memory consumption, and LRU behavior. The memory problem is relatively simple to solve. 

Unfortunately, since the pre-paint textures (especially in a large pre-paint region) are not 'used' they don't fit in the LRU anywhere.  This change puts them at the front of the LRU, and relies on the textures being protected to stay alive.

There is one issue this change doesn't address, which is how layers share with each other. Given the current scheme, the only solution I can think of to share memory fairly is iteratively expanding the pre-paint region (ie. two or more passes of idlePaintContents, expanding the region each time).

[Eric Penner]

Comment on attachment 119067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119067&action=review

> Source/WebCore/platform/graphics/chromium/TextureManager.h:89
> +    void setPriority(TexturePriority priority) { m_priority = priority; }

Note: I could expose this externally via protectTexture and ManagedTexture::reserve, but that effects all call sites that reserve textures, so I thought having two modes was better.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:369
> +    m_contentsTextureManager->setMaxMemoryLimitBytes(preferredLimitBytes);

Alternatively, I could have TextureManager use the preferred memory limit as the max, when the priority is set to PreferredPriority.

[Adrienne Walker]

I had a conversation with jamesr yesterday offline, and my thought is that the TextureManager has a few different concerns, such as texture budget and limits, preventing reuse of in-use textures, and eviction policy.  It's a little bit awkward to conflate all of those via the reserve() command.  These concerns also apply differently to render surface and content texture managers too--render surface texture managers don't need any sense of priority or reclamation limit.

Thinking out loud, for content textures, maybe we want a smarter eviction policy with spatial knowledge.  If you prepaint a tile that's just offscreen, shouldn't a tile that was previously onscreen (not prepainted) but is spatially further away from that prepainted tile get evicted first? For render surface textures, LRU is probably still a reasonable policy.

It would be nice to abstract these concerns out into different classes so that we could more easily test them and not have this ever-growing overloaded texture manager class.  I am not sure that I want to add this priority wrinkle to the texture manager in such a direct way right now.

To talk overall strategy, even though it'd be nice, this patch isn't a high priority for prepainting, right? For one, we can't increase the prepainting region arbitrarily until SkPicture-painting becomes the default so that we don't pay for extra rasterization.  Still, we can still prepaint at a fast enough rate for scrolling, right?

[Eric Penner]

(In reply to comment #4)
> I had a conversation with jamesr yesterday offline, and my thought is that the TextureManager has a few different concerns, such as texture budget and limits, preventing reuse of in-use textures, and eviction policy.  It's a little bit awkward to conflate all of those via the reserve() command.  These concerns also apply differently to render surface and content texture managers too--render surface texture managers don't need any sense of priority or reclamation limit.
> 
> Thinking out loud, for content textures, maybe we want a smarter eviction policy with spatial knowledge.  If you prepaint a tile that's just offscreen, shouldn't a tile that was previously onscreen (not prepainted) but is spatially further away from that prepainted tile get evicted first? For render surface textures, LRU is probably still a reasonable policy.
> 
> It would be nice to abstract these concerns out into different classes so that we could more easily test them and not have this ever-growing overloaded texture manager class.  I am not sure that I want to add this priority wrinkle to the texture manager in such a direct way right now.
> 
> To talk overall strategy, even though it'd be nice, this patch isn't a high priority for prepainting, right? For one, we can't increase the prepainting region arbitrarily until SkPicture-painting becomes the default so that we don't pay for extra rasterization.  Still, we can still prepaint at a fast enough rate for scrolling, right?


One correction: it is possible to turn on a large pre-paint region with only this patch (we shouldn't need the SkPicture changes). The additional paints are always small and preempted by visible paints.

I was definitely also questioning whether an LRU was appropriate any more, given the added constraints. The worst possible case is that you would need to redo all the pre-painting in order to resolve the correct priority (based on protecting all the pre-paint tiles, rather than LRU).

Since I gave some thought to a direct priority based on distance, here are some thoughts on that: The current system is completely implicit, so we can't guarantee a tile will get it's priority updated each frame. If we did enforce that, ideally all priorities would be updated before we even start to reserve new memory, so we get the eviction order correct. An alternative might be to use the last frame's priority for one frame. 

One issue is that even one tile, or pre-painting can take significant memory on pages with many layers, which results in pushing to the high memory limit. A subset of this patch would be to simply set the memory limit before pre-painting, and forget about LRU order.  That would leave texture manager pretty much untouched. What do you think?

[Eric Penner]

(In reply to comment #5)
> (In reply to comment #4)
> > I had a conversation with jamesr yesterday offline, and my thought is that the TextureManager has a few different concerns, such as texture budget and limits, preventing reuse of in-use textures, and eviction policy.  It's a little bit awkward to conflate all of those via the reserve() command.  These concerns also apply differently to render surface and content texture managers too--render surface texture managers don't need any sense of priority or reclamation limit.
> > 
> > Thinking out loud, for content textures, maybe we want a smarter eviction policy with spatial knowledge.  If you prepaint a tile that's just offscreen, shouldn't a tile that was previously onscreen (not prepainted) but is spatially further away from that prepainted tile get evicted first? For render surface textures, LRU is probably still a reasonable policy.
> > 
> > It would be nice to abstract these concerns out into different classes so that we could more easily test them and not have this ever-growing overloaded texture manager class.  I am not sure that I want to add this priority wrinkle to the texture manager in such a direct way right now.
> > 
> > To talk overall strategy, even though it'd be nice, this patch isn't a high priority for prepainting, right? For one, we can't increase the prepainting region arbitrarily until SkPicture-painting becomes the default so that we don't pay for extra rasterization.  Still, we can still prepaint at a fast enough rate for scrolling, right?
> 
> 
> One correction: it is possible to turn on a large pre-paint region with only this patch (we shouldn't need the SkPicture changes). The additional paints are always small and preempted by visible paints.
> 
> I was definitely also questioning whether an LRU was appropriate any more, given the added constraints. The worst possible case is that you would need to redo all the pre-painting in order to resolve the correct priority (based on protecting all the pre-paint tiles, rather than LRU).
> 
> Since I gave some thought to a direct priority based on distance, here are some thoughts on that: The current system is completely implicit, so we can't guarantee a tile will get it's priority updated each frame. If we did enforce that, ideally all priorities would be updated before we even start to reserve new memory, so we get the eviction order correct. An alternative might be to use the last frame's priority for one frame. 
> 
> One issue is that even one tile, or pre-painting can take significant memory on pages with many layers, which results in pushing to the high memory limit. A subset of this patch would be to simply set the memory limit before pre-painting, and forget about LRU order.  That would leave texture manager pretty much untouched. What do you think?

If you are okay with just adjusting the memory limit before/after pre-paint, I will role that into the original CL. It turns out James was waiting on me to make a commit-queue request, so that change still hasn't landed.

[Adrienne Walker]

(In reply to comment #5)

> One issue is that even one tile, or pre-painting can take significant memory on pages with many layers, which results in pushing to the high memory limit. A subset of this patch would be to simply set the memory limit before pre-painting, and forget about LRU order.  That would leave texture manager pretty much untouched. What do you think?

That seems like a reasonable approach to fix the memory issues.

[James Robinson]

What's the status of this patch?

[Eric Penner]

Closing. We change the maximum memory limit during pre-painting instead.