Bug 95758 – [chromium] Adjust texture priorities in preparation of impl-thread eviction of only some textures

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Christopher Cameron]

[chromium] Adjust texture priorities in preparation of impl-thread eviction of only some textures

[Christopher Cameron]

Created attachment 162060 [details]
Patch

[Christopher Cameron]

Adding text from ChangeLog and reviewers.

Adjust texture priorities to prepare for asynchronous impl-thread eviction of textures.  Move lingering textures to be prioritized before textures that are more than half of a viewport away, so that lingering textures are not evicted before tiles that are extremely far from the viewport.

Change priority scores to be in "percentage of a viewport away from being visible" instead of "pixels away from being visible" so that priority cutoffs of "just draw X viewports of data" can be specified.

Make CCPriorityCalculator functions that were effectively static be static, and remove passing of unused CCPriorityCalculator classes.

Fix a bug where the priority of a CCPrioritizedTexture was stored as a size_t instead of an int.

Specify a particular priority function for small animated layers instead of using a pixel distance value.

[Dana Jansens]

(In reply to comment #2)
> Adding text from ChangeLog and reviewers.
> 
> Adjust texture priorities to prepare for asynchronous impl-thread eviction of textures.  Move lingering textures to be prioritized before textures that are more than half of a viewport away, so that lingering textures are not evicted before tiles that are extremely far from the viewport.
> 
> Change priority scores to be in "percentage of a viewport away from being visible" instead of "pixels away from being visible" so that priority cutoffs of "just draw X viewports of data" can be specified.
> 
> Make CCPriorityCalculator functions that were effectively static be static, and remove passing of unused CCPriorityCalculator classes.

I tried to remove passing around the class also, but eric has resisted that as it disallows us to add state to the calculator in the future. I think if we want to do it, that should be a separate patch from behaviour changes, anyhow.

> Fix a bug where the priority of a CCPrioritizedTexture was stored as a size_t instead of an int.
> 
> Specify a particular priority function for small animated layers instead of using a pixel distance value.

[Christopher Cameron]

Created attachment 162121 [details]
Patch

[Christopher Cameron]

Thanks!

I've removed passing around the class in the patch (but I have retained the changes that make the effectively-static functions static).

[Eric Penner]

Comment on attachment 162121 [details]
Patch

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

Regarding passing the priority calculator object around, don't let me stop you from removing it. I think I was just pushing back against removing it myself during the original patch (in order to get it landed) so feel free to clean it up as you like.  Previously it looked like we would use layer ordering which would require passing an object around, but that looks quite unlikely now.

> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:32
>  

Very much like these constants in one place :)

> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:36
> +static const int kUiDoesNotDrawToRootSurface      =       2;

I think it's okay to make all UI textures at the same priority, above everything else.

> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:41
> +// less than a half-viewport away are considered "near".

Half a viewport seems low to me. Would 2 viewports be okay?

> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:54
> +static const int kFarFromVisibleBase              =  300050;

Could we possibly just squash these into the above lingering group?  There could be many lingering textures (enough to use all remaining memory) and I would rather not have them take away memory from closer textures. Could we just start calling all textures lingering after a certain distance limit is reached?

> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:100
> +    {

Could we call the manhattanDistance function here and then normalize to viewport units? I feel it's more clear what the code is doing that way.

[Christopher Cameron]

Comment on attachment 162121 [details]
Patch

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

Thanks!

>> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:36
>> +static const int kUiDoesNotDrawToRootSurface      =       2;
> 
> I think it's okay to make all UI textures at the same priority, above everything else.

Sure -- updated adjustments.

>> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:41
>> +// less than a half-viewport away are considered "near".
> 
> Half a viewport seems low to me. Would 2 viewports be okay?

Updated to 2 viewports.

>> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:54
>> +static const int kFarFromVisibleBase              =  300050;
> 
> Could we possibly just squash these into the above lingering group?  There could be many lingering textures (enough to use all remaining memory) and I would rather not have them take away memory from closer textures. Could we just start calling all textures lingering after a certain distance limit is reached?

I've moved them to be lower than than the >2-viewports-away but still higher than <2-viewports-away.  I'd like to keep lingering to be closer than "many viewports away".  There is a pathology now where on very long pages, we will use all available memory caching very-far-away tiles, and I want to keep the lingering tiles with higher priority than the very-far-away tiles,

>> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:100
>> +    {
> 
> Could we call the manhattanDistance function here and then normalize to viewport units? I feel it's more clear what the code is doing that way.

I wanted to do this, but the normalization is different in X and Y (so I can't recover it directly from the scalar distance).  I've added a comment about L1 norms because it's you're right that it's unclear what the math does without that prior.

[Christopher Cameron]

Created attachment 162141 [details]
Patch

[Eric Penner]

Comment on attachment 162121 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:54
>>> +static const int kFarFromVisibleBase              =  300050;
>> 
>> Could we possibly just squash these into the above lingering group?  There could be many lingering textures (enough to use all remaining memory) and I would rather not have them take away memory from closer textures. Could we just start calling all textures lingering after a certain distance limit is reached?
> 
> I've moved them to be lower than than the >2-viewports-away but still higher than <2-viewports-away.  I'd like to keep lingering to be closer than "many viewports away".  There is a pathology now where on very long pages, we will use all available memory caching very-far-away tiles, and I want to keep the lingering tiles with higher priority than the very-far-away tiles,

I see the issue, but doesn't this just push the problem to the lingering tiles? There could be unbounded lingering tiles as well which would consume all memory, including the 'far' tile memory (which would more likely be closer than the lingering tiles).

What about setting the limit to be some multiple * visibleMemory, to reduce the total memory usage. This will mimic and LRU since all lingering tiles slowly decrease in priority.

In any case sorry about the 'lingering' thing! Lingering will go away soon!!

>>> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:100
>>> +    {
>> 
>> Could we call the manhattanDistance function here and then normalize to viewport units? I feel it's more clear what the code is doing that way.
> 
> I wanted to do this, but the normalization is different in X and Y (so I can't recover it directly from the scalar distance).  I've added a comment about L1 norms because it's you're right that it's unclear what the math does without that prior.

Hmm, how about reducing to one viewport scaler (average or sum?). In particular a wide viewport shouldn't bias towards caching horizontal tiles.

[Christopher Cameron]

(In reply to comment #9)
> (From update of attachment 162121 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162121&action=review
> 
> I see the issue, but doesn't this just push the problem to the lingering tiles? There could be unbounded lingering tiles as well which would consume all memory, including the 'far' tile memory (which would more likely be closer than the lingering tiles).
>
> What about setting the limit to be some multiple * visibleMemory, to reduce the total memory usage. This will mimic and LRU since all lingering tiles slowly decrease in priority.

Yes -- eventually, you'll hit a hard limit for all memory allowed to be allocated.  When we set the "priority limit", we may want to set it to be "lingering for X frames", which will allow them to be purged more aggressively.

> > I wanted to do this, but the normalization is different in X and Y (so I can't recover it directly from the scalar distance).  I've added a comment about L1 norms because it's you're right that it's unclear what the math does without that prior.
> 
> Hmm, how about reducing to one viewport scaler (average or sum?). In particular a wide viewport shouldn't bias towards caching horizontal tiles.

Oh -- the current scheme is to treat the dimensions separately, so a wide viewport is only taken into account when looking at the horizontal distance to the viewport (and a tall viewport is only taken into account when looking at the vertical distance).

[Eric Penner]

> Yes -- eventually, you'll hit a hard limit for all memory allowed to be allocated.  When we set the "priority limit", we may want to set it to be "lingering for X frames", which will allow them to be purged more aggressively.

But even if you do that, all the 'far' tiles will be completely gone right? Why not lump 'far' together with 'unknown' such that they can all linger for X frames rather than 'far' disappearing first.

> > Hmm, how about reducing to one viewport scaler (average or sum?). In particular a wide viewport shouldn't bias towards caching horizontal tiles.
> 
> Oh -- the current scheme is to treat the dimensions separately, so a wide viewport is only taken into account when looking at the horizontal distance to the viewport (and a tall viewport is only taken into account when looking at the vertical distance).

I see that but it doesn't seem optimal. The more I think about it, the less I'm convinced we need to normalize by viewport size at all. But if we do, I don't think it should be biased for x or y based on the viewport dimensions. If anything we should always bias towards vertical caching as that's where the user will most likely be scrolling and seeing blank tiles.

[Dana Jansens]

Viewport dimensions don't affect scrolling speed, so I also don't think it should matter if the viewport is tall and narrow or short and wide. Either way we should cache as far out each way that we can. And I agree with Eric about vertical. Maybe we cache faster in the vertical direction always, like 2:1 or 3:2 or something, but I don't see what the viewport size has to do with it. Did you have some particular scenario in mind?

[Christopher Cameron]

To take a step back, the goal of this change is to tweak the priorities so that I can put tabs into states of
- "only use video memory for what is on-screen" (say, for a cold backgrounded tab)
- "only use video memory for what is nearly on-screen" (say, for a warm backgrounded tab)
- "use as much video memory as you want, but don't keep around tiles that are 10 screens away, since that's just wasteful" (say, for the current tab)

I moved the lingering state to be at a higher priority here because that state is often for elements that will suddenly appear on-screen (say, on mouse-over), and so I don't want to have them matter less than tiles that are basically infinitely far away.  This is to not regress these sorts of UIs when discarding textures that are very far away.  If I don't make this change, the I cannot discard in order of priority without discarding all lingering tiles before discarding any tiles that were visible many screens ago.

I changed the units for distance from pixels to viewports so that I can specify "discard all textures that are >1 viewport away" (and things to that effect) for memory management.

(In reply to comment #12)
> Viewport dimensions don't affect scrolling speed, so I also don't think it should matter if the viewport is tall and narrow or short and wide. Either way we should cache as far out each way that we can.

The issue with caching as far as we can is that that can be unbounded.  This work came out of the following bug, where, on a long page, we consumed 500MB of video memory caching previously rendered tiles.
http://code.google.com/p/chromium/issues/detail?id=141377

Keeping around everything we every rendered is wasteful.  It also doesn't make much sense considering how un-aggressive we are with pre-painting -- in the above bug, we pre-paint half of a screen (or so), in the direction we are most likely to scroll (to content we haven't seen), while we retain hundreds of screens of content that we have already seen.

Using unbounded amounts of video memory like this causes instability and poor performance -- some systems will keel over under the pressure (or silently start texturing across PCIE, etc), and we don't have reliable ways of seeing where pressure starts.

> And I agree with Eric about vertical. Maybe we cache faster in the vertical direction always, like 2:1 or 3:2 or something, 

Unless I have the math wrong (which I may), the math that is there treats the two axes independently.  It will not put the vertical caching at a disadvantage in, for example, a very wide window.  If you'd like to add an additional bias towards caching more vertical tiles, that's fine as well.

> but I don't see what the viewport size has to do with it. Did you have some particular scenario in mind?

The goal is to be able to say "keep around X screens worth of previously-rendered content", instead of "keep around X bytes worth of previously-rendered content (no matter how far away it is)".  I was considering having a rule of "keep X pixels around", but that unit of measure is very hard to reason about because of differing screen sizes.

The true answer about how much to keep around (and pre-paint) is "however much we need to not checkerboard when scrolling", which is a function of rasterization time and scroll speed.  Also of note is that on platforms with weaker GPUs and where there are not multiple visible windows, we may want to be more aggressive about caching.

This change doesn't add any new cutoffs -- they will need to be added very carefully.

[Eric Penner]

> I moved the lingering state to be at a higher priority here because that state is often for elements that will suddenly appear on-screen (say, on mouse-over), and so I don't want to have them matter less than tiles that are basically infinitely far away.  This is to not regress these sorts of UIs when discarding textures that are very far away.  If I don't make this change, the I cannot discard in order of priority without discarding all lingering tiles before discarding any tiles that were visible many screens ago.

I think this will help but doesn't eliminate the problem of using all memory, it just moves it into the lingering category. Since this is short term I'm okay with it though. Keep in mind that on Android we do want to cache up to the max limit always, until paint speed is dramatically increased.

> I changed the units for distance from pixels to viewports so that I can specify "discard all textures that are >1 viewport away" (and things to that effect) for memory management.

I think this could be achieved by just being a bit more loose about what you call "one viewport". The viewport width and height aren't even in the layer's space (what if it's rotated for example), so scaling by them doesn't make sense when measuring distance within a layer, unless you transform them into vectors in content space, which would be overkill. Similarly the scroll velocity isn't effected by viewport size as Dana mentions, so we don't think it should effect the amount cached in each direction, which non-uniform scaling of the distance metric would do.

So I'm saying maybe "one viewport" could just be loosely defined as say "(viewport_width+viewport_height)/2 pixels". This could be done after the fact when discarding textures based on priority level, or baked it into the priority. It's just a quick way to create a 1D yard-stick instead of a 2D one. Personally I like doing it after the fact actually, and keeping pixel units for clarity and debugging.

[Christopher Cameron]

(In reply to comment #14)

> > This could be done after the fact when discarding textures based on priority level, or baked it into the priority. 

Well, this is going back to the scheme in bug 94751, where the priority is opaque, and comparisons can only be made with respect to a metric (which can change with the prioritization scheme).

This is proving to be more controversial than I'd anticipated, so I'll just check in the refactoring changes, with no functional changes.  I'll reintroduce the viewport changes in the context of their eventual use.

[Christopher Cameron]

Created attachment 162316 [details]
Patch

[Eric Penner]

Comment on attachment 162316 [details]
Patch

Nice cleanup. Really sorry if you think we are being pedantic about this distance metric thing. I didn't see its importance in your end goal, so I thought it would be easy to just use a uniform scale. Maybe it will be more clear together with the change that uses it.

[Peter Beverloo (cr-android ews)]

Comment on attachment 162316 [details]
Patch

Attachment 162316 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13757752

[Adrienne Walker]

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

> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:34
> +static const int kUiDrawsToRootSurface            =      -1;
> +static const int kVisibleDrawsToRootSurface       =       0;

These named constants are way better.

style nit: In general, WebKit style is not to use kFoo for named static variables.  Usually this would just be like "visibleDrawsToRootSurface" and not "kVisibleDrawsToRootSurface".

style nit: Please don't use whitespace to vertically align anything.

> Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:46
> +static const int kSmallAnimatedLayer              = 1000512;

nit: maybe kNotVisibleBase + 512?

[Christopher Cameron]

Created attachment 162361 [details]
Patch

[Christopher Cameron]

(In reply to comment #19)
> View in context: https://bugs.webkit.org/attachment.cgi?id=162316&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCPriorityCalculator.cpp:34
> > +static const int kUiDrawsToRootSurface            =      -1;
> > +static const int kVisibleDrawsToRootSurface       =       0;
> 
> These named constants are way better.
> 
> style nit: In general, WebKit style is not to use kFoo for named static variables.  Usually this would just be like "visibleDrawsToRootSurface" and not "kVisibleDrawsToRootSurface".

Ah -- fixed (changed them to visibleDrawsToRootSurfacePriority, etc).

> style nit: Please don't use whitespace to vertically align anything.

Fixed.

> nit: maybe kNotVisibleBase + 512?

Good point -- fixed.

[Adrienne Walker]

Comment on attachment 162361 [details]
Patch

R=me.  Looks good to me, except for fixing cr-android-ews.

[Christopher Cameron]

Created attachment 162364 [details]
Patch

[Christopher Cameron]

(In reply to comment #22)
> (From update of attachment 162361 [details])
> R=me.  Looks good to me, except for fixing cr-android-ews.

Fixed (signed/unsigned mismatch).

[Christopher Cameron]

Created attachment 162368 [details]
Patch

[Adrienne Walker]

Comment on attachment 162368 [details]
Patch

R=me, assuming it passes ews.

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=231377