Bug 83316 – [chromium] Remove viewport memory restrictions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Remove viewport memory restrictions and remove duplicate memory limit reductions on setVisible(false)

[Dana Jansens]

Created attachment 135909 [details]
Patch

[WebKit Review Bot]

Attachment 135909 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dana Jansens]

Created attachment 135955 [details]
Patch

[Dana Jansens]

Proper changelog with explanation :)

[Nat Duca]

Can you explain the rationale on some of this? I've gotten a little stale on this material lately...

[Nat Duca]

Oh look. Nice long changelog on the second patch. Whommmp!

[Nat Duca]

Comment on attachment 135955 [details]
Patch

So two questions:
1) does this regress android in any way? Ping epenner to see if the loss of a viewport multiplier makes sense to him
2) should we be mucking around with this code at all? In particular, I'm wondering if we should focus on adding memory request APIs to the GpuMemoryManager and then having those limits drive the texture manager limits.

[Dana Jansens]

(In reply to comment #7)
> (From update of attachment 135955 [details])
> So two questions:
> 1) does this regress android in any way? Ping epenner to see if the loss of a viewport multiplier makes sense to him

Will discuss.

> 2) should we be mucking around with this code at all? In particular, I'm wondering if we should focus on adding memory request APIs to the GpuMemoryManager and then having those limits drive the texture manager limits.

I think this is potentially backportable to M19, where we are seeing problems already, especially since Accelerated Canvas is turning on the compositor more often. I agree GpuMemoryManager is the way to go to make this better.

[Eric Penner]

Yeah we need the viewport multiplier to keep the limit lower on low resolution devices. The original patch had logic to prevent the some of the problems described (issues with small viewports), but it was removed before landing.

If a short term fix is needed why not just increase the viewport size or multiplier?

[Vangelis Kokkevis]

(In reply to comment #9)
> Yeah we need the viewport multiplier to keep the limit lower on low resolution devices. The original patch had logic to prevent the some of the problems described (issues with small viewports), but it was removed before landing.
> 
> If a short term fix is needed why not just increase the viewport size or multiplier?

The problem is that the viewport size isn't necessarily a good indicator of how much vram is available, at least not for desktop platforms. Maybe a more flexible solution would be to set the min and max values once, at TextureManager creation time and let each platform decide what it wants to set them to.  For example on Android we can use the viewport size (which will most likely be constant) and on the desktop use some other reasonable fixed value.

[Nat Duca]

Is there a crbug that justifies M19-related flailing?

[Dana Jansens]

(In reply to comment #11)
> Is there a crbug that justifies M19-related flailing?

http://code.google.com/p/chromium/issues/detail?id=121605 : There're are a few problems caused there due to OOM. But no OOM would avoid them. 

http://code.google.com/p/chromium/issues/detail?id=121557 : Also causes OOM.

[Dana Jansens]

Created attachment 136044 [details]
Patch

This keeps the multipliers for android as requested by Eric. I updated the android multipliers/values to what they are actually using downstream.

[Eric Penner]

(In reply to comment #10)
> (In reply to comment #9)
> > Yeah we need the viewport multiplier to keep the limit lower on low resolution devices. The original patch had logic to prevent the some of the problems described (issues with small viewports), but it was removed before landing.
> > 
> > If a short term fix is needed why not just increase the viewport size or multiplier?
> 
> The problem is that the viewport size isn't necessarily a good indicator of how much vram is available, at least not for desktop platforms. Maybe a more flexible solution would be to set the min and max values once, at TextureManager creation time and let each platform decide what it wants to set them to.  For example on Android we can use the viewport size (which will most likely be constant) and on the desktop use some other reasonable fixed value.

Yeah makes sense. The point is to prevent hitting a real OOM (vram limit). We do set the max limit at one other place (to prevent pre-painting from consuming the entire max limit), so I like Dana's new patch which just ignores the viewport on Desktop.

[Nat Duca]

Comment on attachment 136044 [details]
Patch

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

I would prefer if the refactoring "foremost, make no change" was separate from behavior changing.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:176
> +    if (m_contentsTextureManager && m_layerRendererInitialized)

why?

[Adrienne Walker]

Comment on attachment 136044 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [chromium] Remove viewport memory restrictions and remove duplicate memory limit reductions on setVisible(false)

I think you should separate these two changes, especially if you want to merge one of them.

[Dana Jansens]

Created attachment 137014 [details]
Patch

[Dana Jansens]

I've split this patch up. This CL removes the viewport restriction on the preferred/max memory limits. And removes the min memory limit entirely.

This means instead of possibly preserving some number of prepainted tiles, we evict all non-visible tiles when we background a tab (if we aren't deleting all textures anyways).

[Vangelis Kokkevis]

Comment on attachment 137014 [details]
Patch

Why not set these limits via API calls to the TextureManager rather than baking them into the class? We'll need that functionality soon anyway, when gpu memory manager starts telling the renderers how much memory they can use.

[Dana Jansens]

(In reply to comment #19)
> (From update of attachment 137014 [details])
> Why not set these limits via API calls to the TextureManager rather than baking them into the class? We'll need that functionality soon anyway, when gpu memory manager starts telling the renderers how much memory they can use.

I am not sure I get what you mean. What sort of API calls are you thinking? They are currently baked into the TextureManager, but we'd like to get them from GpuMemManager. I think we'll likely deal with the GpuMemManager in a separate PriorityTextureManager implementation, and we can probably just leave this one as fixed values anyhow as it'll be replaced by the new one.

For this patch, though, I'm approaching it as a minimal thing to make us not use the viewport, so that we can easily merge it back to M19.

[Vangelis Kokkevis]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 137014 [details] [details])
> > Why not set these limits via API calls to the TextureManager rather than baking them into the class? We'll need that functionality soon anyway, when gpu memory manager starts telling the renderers how much memory they can use.
> 
> I am not sure I get what you mean. What sort of API calls are you thinking? They are currently baked into the TextureManager, but we'd like to get them from GpuMemManager. I think we'll likely deal with the GpuMemManager in a separate PriorityTextureManager implementation, and we can probably just leave this one as fixed values anyhow as it'll be replaced by the new one.
> 
> For this patch, though, I'm approaching it as a minimal thing to make us not use the viewport, so that we can easily merge it back to M19.

What I had in mind was defining a method like:

TextureManager::SetLimits(min, max, viewportMultiplier)

so that we can set different limits depending on what the TextureManager is used for (e.g. is this the layer or the rendersurface TM) and we can also reset them as needed.

However, if we are going to throw away the TM class once we implement the GpuMemManager then maybe it's not worth it.

[Dana Jansens]

(In reply to comment #21)
> What I had in mind was defining a method like:
> 
> TextureManager::SetLimits(min, max, viewportMultiplier)
> 
> so that we can set different limits depending on what the TextureManager is used for (e.g. is this the layer or the rendersurface TM) and we can also reset them as needed.

Oh ok. So basically dropping the static methods and setMax/setPreferred from the class and just calling the setLimits() instead.

> However, if we are going to throw away the TM class once we implement the GpuMemManager then maybe it's not worth it.

I think it's something we could do separately from this CL either way. I think that would be a nicer API for the current TM as well to even just say setLimits(viewportSize), but maybe not worth it as you say.

[Adrienne Walker]

Comment on attachment 137014 [details]
Patch

R=me.  Thanks for chopping this up somewhat.

[WebKit Review Bot]

Comment on attachment 137014 [details]
Patch

Clearing flags on attachment: 137014

Committed r114191: <http://trac.webkit.org/changeset/114191>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.