Remove limit on number of resources in cc

Remove limit on number of resources in cc

This ability was added for Android WebView to limit the number of
gralloc buffers, which has a low limit on the number of allocations.
Android WebView no longer uses gralloc buffers, so no need to keep this
ability.

Use memory limit to limit the staging resource pool for 1-copy
rasterizer.

BUG=426548
Committed: https://crrev.com/77de0216df77874d43b21233fcb675671cfd0c55
Cr-Commit-Position: refs/heads/master@{#301559}

[boliu, 2014-10-24 23:31:56]

boliu@chromium.org changed reviewers:
+ hush@chromium.org, reveman@chromium.org, yfriedman@chromium.org

[boliu, 2014-10-24 23:31:57]

PTAL!

hush - android_webview
yfriedman - content
reveman - cc

[hush (inactive), 2014-10-24 23:36:37]

https://codereview.chromium.org/643993005/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/643993005/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:122:
compositor_->SetMemoryPolicy(0u);
do you need to enforce it immediately right here?
what if the next DrawGL never comes?

[boliu, 2014-10-24 23:37:53]

https://codereview.chromium.org/643993005/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/643993005/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:122:
compositor_->SetMemoryPolicy(0u);
On 2014/10/24 23:36:37, hush wrote:
> do you need to enforce it immediately right here?
> what if the next DrawGL never comes?

It is enforce it once line below. There's just a huge gap..

[hush (inactive), 2014-10-24 23:49:59]

android_webview/ lgtm

https://codereview.chromium.org/643993005/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/643993005/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:122:
compositor_->SetMemoryPolicy(0u);
On 2014/10/24 23:37:53, boliu wrote:
> On 2014/10/24 23:36:37, hush wrote:
> > do you need to enforce it immediately right here?
> > what if the next DrawGL never comes?
> 
> It is enforce it once line below. There's just a huge gap..

oops. Yes. Thanks!

[reveman, 2014-10-27 13:39:00]

https://codereview.chromium.org/643993005/diff/60001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/643993005/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.h:214: int64 BytesLimitFromConfig(const gfx::Size&
size, ResourceFormat format);
MemoryUsageFromConfig?

https://codereview.chromium.org/643993005/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.h:215: int64 BytesLimitFromTile(const Tile* tile);
MemoryUsageFromTile?

https://codereview.chromium.org/643993005/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/643993005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1251: staging_bytes_limit);
Where did the visible_ check go?

The actual limit used is not so important as a more strict limit will be
enforced by the raster worker pool implementation. This is mostly to minimize
the memory usage when invisible. Please change GetMaxStagingResourceCount() into
GetMaxStagingResourceBytes() and remove the bytes_per_tile calculation.
GetMaxStagingResourceBytes() can simply return "32 * 512 * 512 * 4" for now.

[boliu, 2014-10-27 15:35:58]

https://codereview.chromium.org/643993005/diff/60001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/643993005/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.h:214: int64 BytesLimitFromConfig(const gfx::Size&
size, ResourceFormat format);
On 2014/10/27 13:39:00, reveman wrote:
> MemoryUsageFromConfig?

Done.

https://codereview.chromium.org/643993005/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.h:215: int64 BytesLimitFromTile(const Tile* tile);
On 2014/10/27 13:39:00, reveman wrote:
> MemoryUsageFromTile?

Done.

https://codereview.chromium.org/643993005/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/643993005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1251: staging_bytes_limit);
On 2014/10/27 13:39:00, reveman wrote:
> Where did the visible_ check go?

Oops my bad! Added back now.

> 
> The actual limit used is not so important as a more strict limit will be
> enforced by the raster worker pool implementation. This is mostly to minimize
> the memory usage when invisible. Please change GetMaxStagingResourceCount()
into
> GetMaxStagingResourceBytes() and remove the bytes_per_tile calculation.
> GetMaxStagingResourceBytes() can simply return "32 * 512 * 512 * 4" for now.

Done.

[reveman, 2014-10-27 16:57:55]

reveman@chromium.org changed reviewers:
+ vmpstr@chromium.org

[reveman, 2014-10-27 16:57:56]

cc/ lgtm but vmpstr should take a look at tile manager changes before this land

[Yaron, 2014-10-27 17:02:10]

content lgtm but I don't own content/renderer/gpu/compositor_output_surface.cc

[boliu, 2014-10-27 17:05:23]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org

[boliu, 2014-10-27 17:05:24]

On 2014/10/27 17:02:10, Yaron wrote:
> content lgtm but I don't own content/renderer/gpu/compositor_output_surface.cc

+sievers :)

[boliu, 2014-10-27 17:07:51]

boliu@chromium.org changed reviewers:
+ jbauman@chromium.org
- sievers@chromium.org

[boliu, 2014-10-27 17:07:52]

Oops, Daniel doesn't own that file either.

+jbauman for content/renderer/gpu/compositor_output_surface.cc

[danakj, 2014-10-27 17:16:54]

Daniel owns all of content/ now fwiw

On Mon, Oct 27, 2014 at 1:07 PM, <boliu@chromium.org> wrote:

> Oops, Daniel doesn't own that file either.
>
> +jbauman for content/renderer/gpu/compositor_output_surface.cc
>
> https://codereview.chromium.org/643993005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2014-10-27 17:20:52]

On 2014/10/27 17:16:54, danakj wrote:
> Daniel owns all of content/ now fwiw

Sorry for the churn... Whoever gets it first then? :p

[vmpstr, 2014-10-27 17:58:55]

https://codereview.chromium.org/643993005/diff/100001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/643993005/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:549: int64 hard_memory_limit =
global_state_.hard_memory_limit_in_bytes;
i would kinda weakly prefer to have something along the lines of 

typedef int64 MemoryUsage;

and keep everything as that, but lgtm either way.

[jamesr, 2014-10-27 18:03:25]

https://codereview.chromium.org/643993005/diff/100001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/643993005/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:549: int64 hard_memory_limit =
global_state_.hard_memory_limit_in_bytes;
we use int64_t, etc, nowadays not int64

[boliu, 2014-10-27 18:48:07]

reveman/vmpstr: Please take another look at tile_manager.cc/h

Went with "using MemoryUsage = int64_t;" which is recommended over typedef.

Not everything is unchanged though. Of course it's no longer possible to call
any methods on MemoryUsage. And also there is no point in passing it as const&.

[vmpstr, 2014-10-27 19:15:41]

lgtm, thanks

[no sievers, 2014-10-28 00:07:06]

lgtm

[jbauman, 2014-10-28 00:09:20]

not lgtm. We still use this for software rendering (set in
CompositorOutputSurface::BindToClient), so I'd prefer not to remove the limit.

[boliu, 2014-10-28 00:19:54]

On 2014/10/28 00:09:20, jbauman wrote:
> not lgtm. We still use this for software rendering (set in
> CompositorOutputSurface::BindToClient), so I'd prefer not to remove the limit.

Ahh ok. Looks like it's the same problem that webview had with gralloc, file
descriptor limits :)

Going to scale this change back then. Reverting all the changes in cc, but
keeping the android_webivew and synchronous comositor changes.

https://codereview.chromium.org/643993005/diff/120001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (left):

https://codereview.chromium.org/643993005/diff/120001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:114:
base::SharedMemory::GetHandleLimit() / 3));
Hmm, this kinda assumes there's at most 3 compositors (or I guess 2, since other
things in the process uses file descriptors too) in a single renderer process?

[boliu, 2014-10-28 00:26:24]

Done. jbauman: PTAL

[jbauman, 2014-10-28 00:38:53]

On 2014/10/28 00:19:54, boliu wrote:
> On 2014/10/28 00:09:20, jbauman wrote:
> > not lgtm. We still use this for software rendering (set in
> > CompositorOutputSurface::BindToClient), so I'd prefer not to remove the
limit.
> 
> Ahh ok. Looks like it's the same problem that webview had with gralloc, file
> descriptor limits :)
> 
Yep, pretty much.

> Going to scale this change back then. Reverting all the changes in cc, but
> keeping the android_webivew and synchronous comositor changes.

Thanks, lgtm.
> 
>
https://codereview.chromium.org/643993005/diff/120001/content/renderer/gpu/co...
> File content/renderer/gpu/compositor_output_surface.cc (left):
> 
>
https://codereview.chromium.org/643993005/diff/120001/content/renderer/gpu/co...
> content/renderer/gpu/compositor_output_surface.cc:114:
> base::SharedMemory::GetHandleLimit() / 3));
> Hmm, this kinda assumes there's at most 3 compositors (or I guess 2, since
other
> things in the process uses file descriptors too) in a single renderer process?

2 visible renderers (non-visible don't get tiles), which is unfortunate but
generally isn't much of an issue.

[boliu, 2014-10-28 00:54:13]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-10-28 00:55:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643993005/140001

[boliu, 2014-10-28 03:14:36]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2014-10-28 03:16:28]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-10-28 03:17:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643993005/140001

[commit-bot: I haz the power, 2014-10-28 04:06:48]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-10-28 04:07:42]

Patchset 8 (id:??) landed as
https://crrev.com/77de0216df77874d43b21233fcb675671cfd0c55
Cr-Commit-Position: refs/heads/master@{#301559}