cc: Remove memory state from tile management

cc: Remove memory state from tile management

After the upload is moved into its own class, the tile
can either have a resource or not. If it doesn't have
a resource, but has a raster task, the memory is unreleasable.
That makes all the memory states are implicit.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203901

[vmpstr, 2013-05-31 18:05:25]

PTAL. This depends on https://codereview.chromium.org/16190002/

[reveman, 2013-06-03 14:11:34]

https://codereview.chromium.org/15715031/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15715031/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:350: if
(!tile->managed_state().raster_task.is_null())
This is not the same. raster_task is only set while tile is being initialized.

https://codereview.chromium.org/15715031/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:415: if
(!tile->managed_state().raster_task.is_null())
It's a bit weird that we examine |raster_task| to determine available memory.
Maybe we should compute bytes_allocatable as the sum of all releasable memory +
max(0, memory_limit_in_bytes - resource_pool_->memory_usage_bytes()).

https://codereview.chromium.org/15715031/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:446: tile_bytes = 0;
Maybe we should reverse this. Make tile_bytes=0 by default and increase it based
on the needs for the tile. ie. maintaining a resource requires
tile->bytes_consumed_if_allocated(), creating a RasterTask for tile requires
another tile->bytes_consumed_if_allocated(). Wdyt?

Comment above needs to be updated anyhow. We shouldn't refer to unreleasable
memory anymore.

[vmpstr, 2013-06-03 17:08:26]

PTAL.

https://codereview.chromium.org/15715031/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15715031/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:350: if
(!tile->managed_state().raster_task.is_null())
On 2013/06/03 14:11:34, David Reveman wrote:
> This is not the same. raster_task is only set while tile is being initialized.

Done.

https://codereview.chromium.org/15715031/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:415: if
(!tile->managed_state().raster_task.is_null())
On 2013/06/03 14:11:34, David Reveman wrote:
> It's a bit weird that we examine |raster_task| to determine available memory.
> Maybe we should compute bytes_allocatable as the sum of all releasable memory
+
> max(0, memory_limit_in_bytes - resource_pool_->memory_usage_bytes()).

Done.

https://codereview.chromium.org/15715031/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:446: tile_bytes = 0;
On 2013/06/03 14:11:34, David Reveman wrote:
> Maybe we should reverse this. Make tile_bytes=0 by default and increase it
based
> on the needs for the tile. ie. maintaining a resource requires
> tile->bytes_consumed_if_allocated(), creating a RasterTask for tile requires
> another tile->bytes_consumed_if_allocated(). Wdyt?
> 
> Comment above needs to be updated anyhow. We shouldn't refer to unreleasable
> memory anymore.

Done.

[reveman, 2013-06-03 18:07:23]

almost there

https://codereview.chromium.org/15715031/diff/5001/cc/resources/managed_tile_...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/15715031/diff/5001/cc/resources/managed_tile_...
cc/resources/managed_tile_state.h:43: return (resource_ ? resource_->id() :
resource_id_);
nit: resource_->id() should be the same as resource_id_ at the moment. I'd
prefer if we just returned resource_id_ but maybe we should add a DCHECK above
to make sure they are the same.

https://codereview.chromium.org/15715031/diff/5001/cc/resources/resource_pool.h
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/15715031/diff/5001/cc/resources/resource_pool...
cc/resources/resource_pool.h:49: size_t memory_acquired_bytes() const {
nit: acquired_memory_usage_bytes() would be a bit more consistent with current
naming.

https://codereview.chromium.org/15715031/diff/5001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15715031/diff/5001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:351: *memory_used_bytes +=
resource_pool_->memory_acquired_bytes();
nit: s/+=/=/ and move it to the top.

https://codereview.chromium.org/15715031/diff/5001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:415: bytes_releasable +
global_state_.memory_limit_in_bytes;
This line alone is confusing. Can you change this so it's a bit easier to
understand what's going on. Use a temporary int64 if that makes it easier.

[vmpstr, 2013-06-03 18:25:48]

PTAL.

https://codereview.chromium.org/15715031/diff/5001/cc/resources/managed_tile_...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/15715031/diff/5001/cc/resources/managed_tile_...
cc/resources/managed_tile_state.h:43: return (resource_ ? resource_->id() :
resource_id_);
On 2013/06/03 18:07:23, David Reveman wrote:
> nit: resource_->id() should be the same as resource_id_ at the moment. I'd
> prefer if we just returned resource_id_ but maybe we should add a DCHECK above
> to make sure they are the same.

Done.

https://codereview.chromium.org/15715031/diff/5001/cc/resources/resource_pool.h
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/15715031/diff/5001/cc/resources/resource_pool...
cc/resources/resource_pool.h:49: size_t memory_acquired_bytes() const {
On 2013/06/03 18:07:23, David Reveman wrote:
> nit: acquired_memory_usage_bytes() would be a bit more consistent with current
> naming.

Done.

https://codereview.chromium.org/15715031/diff/5001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15715031/diff/5001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:351: *memory_used_bytes +=
resource_pool_->memory_acquired_bytes();
On 2013/06/03 18:07:23, David Reveman wrote:
> nit: s/+=/=/ and move it to the top.

Done.

https://codereview.chromium.org/15715031/diff/5001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:415: bytes_releasable +
global_state_.memory_limit_in_bytes;
On 2013/06/03 18:07:23, David Reveman wrote:
> This line alone is confusing. Can you change this so it's a bit easier to
> understand what's going on. Use a temporary int64 if that makes it easier.

Done-ish. Please see if this is better.

[reveman, 2013-06-03 19:48:36]

lgtm

https://codereview.chromium.org/15715031/diff/10002/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/15715031/diff/10002/cc/resources/tile_manager...
cc/resources/tile_manager.cc:416:
static_cast<int64>(resource_pool_->acquired_memory_usage_bytes());
consider using:
int64 bytes_available = memory_limit_in_bytes - acquired_memory_usage_bytes;

[commit-bot: I haz the power, 2013-06-03 21:33:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/15715031/1009

[commit-bot: I haz the power, 2013-06-04 07:39:54]

Change committed as 203901