Bug 73617 – [Chromium] Remove hard-coded memory limits from TextureManager

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Eric Penner]

[Chromium] Remove hard-coded memory limits from TextureManager

[Eric Penner]

Created attachment 117556 [details]
Patch

[Eric Penner]

This initially started here:
https://bugs.webkit.org/show_bug.cgi?id=72202
[chromium] Set texture limits as multiples of viewport size instead of hardcoded values

That CL wasn't reaching consensus, so this change is a subset of the suggestions from there. The idea of using the viewport to set memory limits was questioned, so this just moves the hard-coded values into CCSettings.

At least two other in-progress CLs require knowledge of the reclaim limit from within TextureManager, so I added both reclaim limit and low memory limit as member variables with getters/setters.

[Nat Duca]

Comment on attachment 117556 [details]
Patch

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

Word police. :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:90
> +    size_t highMemoryLimitBytes;

max?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:91
> +    // Preferred texture size limit given the viewport size.

Why not s/reclaim/preferredMemorySize/?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:93
> +    // The maximum texture memory usage when asked to release textures.

This comment doesn't work in my head.

[James Robinson]

Comment on attachment 117556 [details]
Patch

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

This feels even more complicated to me, to be honest.  I really preferred having the TextureManager only internally care about one limit at a time and having the caller be responsible for adjusting that limit at the appropriate times.  This moves complexity into the TextureManager implementation and adds a whole lot more knobs to CCSettings than seem practical without exposing the knobs that you really would want - like allowing the limit to be a multiple of the viewport size.

Let's continue to iterate on https://bugs.webkit.org/show_bug.cgi?id=72202.

> Source/WebCore/platform/graphics/chromium/TextureManager.h:66
> +    void setHighMemoryLimitBytes(size_t);
> +    size_t highMemoryLimitBytes() { return m_highMemoryLimitBytes; }
> +
> +    void setReclaimMemoryLimitBytes(size_t bytes) { m_reclaimMemoryLimitBytes = bytes; }
> +    size_t reclaimMemoryLimitBytes() { return m_reclaimMemoryLimitBytes; }
> +
> +    void setLowMemoryLimitBytes(size_t bytes) { m_lowMemoryLimitBytes = bytes; }
> +    size_t lowMemoryLimitBytes() { return m_lowMemoryLimitBytes; }

why does one setter release memory while the other two setters do not?

[Eric Penner]

Comment on attachment 117556 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TextureManager.h:67
>  

It makes sense somewhat when the setting the high (max) memory limit. The idea with have the other limits is that some allocations will fail while others will not. I can perhaps remove the low memory limit and just keep max/preferred.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:90
>> +    size_t highMemoryLimitBytes;
> 
> max?

These names and comments came from their initial definition above. I'm just removing hard coded variables and making them to be member variables. I like max more too.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:91
>> +    // Preferred texture size limit given the viewport size.
> 
> Why not s/reclaim/preferredMemorySize/?

ditto.  But I like preferred more.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:93
>> +    // The maximum texture memory usage when asked to release textures.
> 
> This comment doesn't work in my head.

ditto above.

[Eric Penner]

(In reply to comment #4)
> (From update of attachment 117556 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117556&action=review
> 
> This feels even more complicated to me, to be honest.  I really preferred having the TextureManager only internally care about one limit at a time and having the caller be responsible for adjusting that limit at the appropriate times.  This moves complexity into the TextureManager implementation and adds a whole lot more knobs to CCSettings than seem practical without exposing the knobs that you really would want - like allowing the limit to be a multiple of the viewport size.
> 

Unfortunately, we are going to need to know about the reclaim/preferred memory limit in any event. 

For example, when protecting textures for a large pre-paint rect, we will need to know the reclaim/preferred limit, so we can stop pre-painting before we hit the max limit. 

Another in-progress CL involves recycling textures, which also needs to know about the preferred limit.

> Let's continue to iterate on https://bugs.webkit.org/show_bug.cgi?id=72202.
> 

There doesn't seem to be agreement on how we will calculate the memory limits, or whether it is even appropriate to use the viewport (and if so, only the viewport and not many other additional heuristics). Minimally we need non-hard-coded values so I thought this would be a good first step.

> > Source/WebCore/platform/graphics/chromium/TextureManager.h:66
> > +    void setHighMemoryLimitBytes(size_t);
> > +    size_t highMemoryLimitBytes() { return m_highMemoryLimitBytes; }
> > +
> > +    void setReclaimMemoryLimitBytes(size_t bytes) { m_reclaimMemoryLimitBytes = bytes; }
> > +    size_t reclaimMemoryLimitBytes() { return m_reclaimMemoryLimitBytes; }
> > +
> > +    void setLowMemoryLimitBytes(size_t bytes) { m_lowMemoryLimitBytes = bytes; }
> > +    size_t lowMemoryLimitBytes() { return m_lowMemoryLimitBytes; }
> 
> why does one setter release memory while the other two setters do not?

[James Robinson]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 117556 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117556&action=review
> > 
> > This feels even more complicated to me, to be honest.  I really preferred having the TextureManager only internally care about one limit at a time and having the caller be responsible for adjusting that limit at the appropriate times.  This moves complexity into the TextureManager implementation and adds a whole lot more knobs to CCSettings than seem practical without exposing the knobs that you really would want - like allowing the limit to be a multiple of the viewport size.
> > 
> 
> Unfortunately, we are going to need to know about the reclaim/preferred memory limit in any event. 
> 
> For example, when protecting textures for a large pre-paint rect, we will need to know the reclaim/preferred limit, so we can stop pre-painting before we hit the max limit. 

The different limits aren't a concern of the TextureManager itself though, are they? Aren't they a concern of the prepainting system?  Enne's more caught up on this than I am.

One thing that we *do* want to make a concern of the TextureManager is the eviction policy - on top of the LRU policy it has now we probably want it to evict prepainted stuff before normal tiles. But that's not a matter of different limits, I think, but different attributes on some textures.

> 
> Another in-progress CL involves recycling textures, which also needs to know about the preferred limit.

No it doesn't - it already landed without any of this.

[Eric Penner]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (From update of attachment 117556 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=117556&action=review
> > > 
> > > This feels even more complicated to me, to be honest.  I really preferred having the TextureManager only internally care about one limit at a time and having the caller be responsible for adjusting that limit at the appropriate times.  This moves complexity into the TextureManager implementation and adds a whole lot more knobs to CCSettings than seem practical without exposing the knobs that you really would want - like allowing the limit to be a multiple of the viewport size.
> > > 
> > 
> > Unfortunately, we are going to need to know about the reclaim/preferred memory limit in any event. 
> > 
> > For example, when protecting textures for a large pre-paint rect, we will need to know the reclaim/preferred limit, so we can stop pre-painting before we hit the max limit. 
> 
> The different limits aren't a concern of the TextureManager itself though, are they? Aren't they a concern of the prepainting system?  Enne's more caught up on this than I am.
> 

Well, the lower priority textures also need a reserve/protect mechanism to avoid endlessly thrashing pre-painting. That should ideally fail when we hit the reclaim limit.

Anyway, if the other CL didn't end up needing it, maybe pre-painting can suffice without it too.

> One thing that we *do* want to make a concern of the TextureManager is the eviction policy - on top of the LRU policy it has now we probably want it to evict prepainted stuff before normal tiles. But that's not a matter of different limits, I think, but different attributes on some textures.
> 
> > 
> > Another in-progress CL involves recycling textures, which also needs to know about the preferred limit.
> 
> No it doesn't - it already landed without any of this.