cc: Do not cleanup tiles with raster tasks.

cc: Do not cleanup tiles with raster tasks.

During cleanup, this keeps the tiles around until the 
associated raster tasks completes running on the worker thread.
We reset the priority of the tile on the tree it lies before
dropping it.

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

[sohanjg, 2014-07-03 12:16:15]

Can you please take a look.
Thanks.

[reveman, 2014-07-03 18:12:47]

The description is a bit off. The important part is not to avoid releasing
resources when we have a pending task. That's actually not a problem. The
important part is keeping the task reference around until it completes. Please
update the description.

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

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:392: CleanUpReleasedTiles();
I think you need to move this below CheckForCompletedTasks() as there's no
guarantee that all tiles can be released otherwise.

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:422: it != released_tiles_.end();) {
nit: I prefer if you use a while loop when iterating like this

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:429: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
I think it's a bit confusing that this code could release some resources but not
delete the tile. ie. first tile version without pending task but second with
one. I think it would be better to handle this pending task check in a loop
above this and not free any resources if the tile has a pending task.

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:437: }
nit: blank line here

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:449: it = released_tiles_.erase(it);
nit: move this below "delete tile;" to maintain the same order in which the tile
and the map entry are destroyed. doesn't really matter but there's no reason to
change it and I think it's a bit cleaner to update the iterator as the last
thing you do in the loop.

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:458: CleanUpReleasedTiles();
This is not the right place to call this anymore. It now needs to be called
after CheckForCompletedTasks.

[sohanjg, 2014-07-04 11:56:59]

Can you please take a look.
Thanks.

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

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:392: CleanUpReleasedTiles();
On 2014/07/03 18:12:46, reveman wrote:
> I think you need to move this below CheckForCompletedTasks() as there's no
> guarantee that all tiles can be released otherwise.

Done.

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:422: it != released_tiles_.end();) {
On 2014/07/03 18:12:47, reveman wrote:
> nit: I prefer if you use a while loop when iterating like this

Done.

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:429: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
On 2014/07/03 18:12:47, reveman wrote:
> I think it's a bit confusing that this code could release some resources but
not
> delete the tile. ie. first tile version without pending task but second with
> one. I think it would be better to handle this pending task check in a loop
> above this and not free any resources if the tile has a pending task.

Done.

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:437: }
On 2014/07/03 18:12:47, reveman wrote:
> nit: blank line here

Done.

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:449: it = released_tiles_.erase(it);
On 2014/07/03 18:12:47, reveman wrote:
> nit: move this below "delete tile;" to maintain the same order in which the
tile
> and the map entry are destroyed. doesn't really matter but there's no reason
to
> change it and I think it's a bit cleaner to update the iterator as the last
> thing you do in the loop.

Done.

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:458: CleanUpReleasedTiles();
On 2014/07/03 18:12:47, reveman wrote:
> This is not the right place to call this anymore. It now needs to be called
> after CheckForCompletedTasks.

Have moved it to ManageTiles, where it checks for completed tasks.
I dont think we should cleanup released tiles every time we check for completed
tasks ?

[vmpstr, 2014-07-05 17:42:19]

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

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:429: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
On 2014/07/04 11:56:58, sohanjg wrote:
> On 2014/07/03 18:12:47, reveman wrote:
> > I think it's a bit confusing that this code could release some resources but
> not
> > delete the tile. ie. first tile version without pending task but second with
> > one. I think it would be better to handle this pending task check in a loop
> > above this and not free any resources if the tile has a pending task.
> 
> Done.

I actually think we should always release all resources on the tile... Something
like:

for (int mode = 0; ...) {
  FreeResourcesForTile(tile, static_cast<RasterMode>(mode));
  if (tile_versions[mode].raster_task_)
    tile_has_raster_task = true;
}

That way we free up everything we can free up, but then not delete the tile if
it still has a raster task. I agree with reveman that in this version it's weird
to sometimes release a resource. However, in the new version we don't release
any resources if a tile has a task, which also seems wasteful.

[reveman, 2014-07-07 18:15:16]

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

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:429: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
On 2014/07/05 17:42:19, vmpstr wrote:
> On 2014/07/04 11:56:58, sohanjg wrote:
> > On 2014/07/03 18:12:47, reveman wrote:
> > > I think it's a bit confusing that this code could release some resources
but
> > not
> > > delete the tile. ie. first tile version without pending task but second
with
> > > one. I think it would be better to handle this pending task check in a
loop
> > > above this and not free any resources if the tile has a pending task.
> > 
> > Done.
> 
> I actually think we should always release all resources on the tile...
Something
> like:
> 
> for (int mode = 0; ...) {
>   FreeResourcesForTile(tile, static_cast<RasterMode>(mode));
>   if (tile_versions[mode].raster_task_)
>     tile_has_raster_task = true;
> }
> 
> That way we free up everything we can free up, but then not delete the tile if
> it still has a raster task. I agree with reveman that in this version it's
weird
> to sometimes release a resource. However, in the new version we don't release
> any resources if a tile has a task, which also seems wasteful.

Fine with me if actually worthwhile. Would we not free these resources under
memory pressure anyhow?

[sohanjg, 2014-07-08 06:21:15]

Have freed up resources for all tiles, but avoided tile deletion if we have
raster task associated with it.
Also, CleanUpReleasedTiles doesn't need to be after CheckForCompletedTasks with
this changes i think, so reverted it to original code.
Can you please take a look.
Thanks.

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

https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:429: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
On 2014/07/07 18:15:16, reveman wrote:
> On 2014/07/05 17:42:19, vmpstr wrote:
> > On 2014/07/04 11:56:58, sohanjg wrote:
> > > On 2014/07/03 18:12:47, reveman wrote:
> > > > I think it's a bit confusing that this code could release some resources
> but
> > > not
> > > > delete the tile. ie. first tile version without pending task but second
> with
> > > > one. I think it would be better to handle this pending task check in a
> loop
> > > > above this and not free any resources if the tile has a pending task.
> > > 
> > > Done.
> > 
> > I actually think we should always release all resources on the tile...
> Something
> > like:
> > 
> > for (int mode = 0; ...) {
> >   FreeResourcesForTile(tile, static_cast<RasterMode>(mode));
> >   if (tile_versions[mode].raster_task_)
> >     tile_has_raster_task = true;
> > }
> > 
> > That way we free up everything we can free up, but then not delete the tile
if
> > it still has a raster task. I agree with reveman that in this version it's
> weird
> > to sometimes release a resource. However, in the new version we don't
release
> > any resources if a tile has a task, which also seems wasteful.
> 
> Fine with me if actually worthwhile. Would we not free these resources under
> memory pressure anyhow?

Done.

[sohanjg, 2014-07-08 13:12:46]

On 2014/07/08 06:21:15, sohanjg wrote:
> Have freed up resources for all tiles, but avoided tile deletion if we have
> raster task associated with it.
> Also, CleanUpReleasedTiles doesn't need to be after CheckForCompletedTasks
with
> this changes i think, so reverted it to original code.
> Can you please take a look.
> Thanks.
> 
> https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/366113002/diff/1/cc/resources/tile_manager.cc...
> cc/resources/tile_manager.cc:429: FreeResourceForTile(tile,
> static_cast<RasterMode>(mode));
> On 2014/07/07 18:15:16, reveman wrote:
> > On 2014/07/05 17:42:19, vmpstr wrote:
> > > On 2014/07/04 11:56:58, sohanjg wrote:
> > > > On 2014/07/03 18:12:47, reveman wrote:
> > > > > I think it's a bit confusing that this code could release some
resources
> > but
> > > > not
> > > > > delete the tile. ie. first tile version without pending task but
second
> > with
> > > > > one. I think it would be better to handle this pending task check in a
> > loop
> > > > > above this and not free any resources if the tile has a pending task.
> > > > 
> > > > Done.
> > > 
> > > I actually think we should always release all resources on the tile...
> > Something
> > > like:
> > > 
> > > for (int mode = 0; ...) {
> > >   FreeResourcesForTile(tile, static_cast<RasterMode>(mode));
> > >   if (tile_versions[mode].raster_task_)
> > >     tile_has_raster_task = true;
> > > }
> > > 
> > > That way we free up everything we can free up, but then not delete the
tile
> if
> > > it still has a raster task. I agree with reveman that in this version it's
> > weird
> > > to sometimes release a resource. However, in the new version we don't
> release
> > > any resources if a tile has a task, which also seems wasteful.
> > 
> > Fine with me if actually worthwhile. Would we not free these resources under
> > memory pressure anyhow?
> 
> Done.

Hmm..looks like as reveman@ pointed out, we need to indeed call
CleanUpReleasedTiles after CheckForCompletedTasks to ensure tiles are released.

[sohanjg, 2014-07-09 13:48:09]

reveman@, vmpstr@, can you please have a look.
Thanks.

[reveman, 2014-07-09 14:15:31]

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

https://codereview.chromium.org/366113002/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:427: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
What's the difference between this and only releasing resources when the tile is
ready to be deleted? It's a subtle difference to the code so if it has a
critical impact on the behavior we should document it. Can you please describe a
use case where this makes a difference and what the difference is.

[sohanjg, 2014-07-09 14:32:11]

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

https://codereview.chromium.org/366113002/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:427: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
On 2014/07/09 14:15:30, reveman wrote:
> What's the difference between this and only releasing resources when the tile
is
> ready to be deleted? It's a subtle difference to the code so if it has a
> critical impact on the behavior we should document it. Can you please describe
a
> use case where this makes a difference and what the difference is.

Hmm, 1 thing could be we optimize resource usage (if at all, till we hit memory
bump). Isnt it better to release whatever is releasable, rather than holding it
back ?

You and vmpstr@ are the best judge here.

[vmpstr, 2014-07-09 18:41:21]

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

https://codereview.chromium.org/366113002/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:427: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
On 2014/07/09 14:32:11, sohanjg wrote:
> On 2014/07/09 14:15:30, reveman wrote:
> > What's the difference between this and only releasing resources when the
tile
> is
> > ready to be deleted? It's a subtle difference to the code so if it has a
> > critical impact on the behavior we should document it. Can you please
describe
> a
> > use case where this makes a difference and what the difference is.
> 
> Hmm, 1 thing could be we optimize resource usage (if at all, till we hit
memory
> bump). Isnt it better to release whatever is releasable, rather than holding
it
> back ?
> 
> You and vmpstr@ are the best judge here.

I think at this point the tile is set to be deleted at one point or another, so
the memory will definitely be released. If we keep the resources until we
actually delete, then we technically would not be able to release any of those
tile resources any other way (this tile is no longer accessible via
AssignGpuMemory).

I don't really think it's that much of a difference, since the task will be
cancelled or finish raster and then be cleaned up, but I prefer this approach as
it states a bit more explicitly that all tile resources at this point are
released (the fact that we hold on to the tile has nothing to do with the
resources, but with the task).

[reveman, 2014-07-09 20:09:43]

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

https://codereview.chromium.org/366113002/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:427: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
On 2014/07/09 18:41:21, vmpstr wrote:
> On 2014/07/09 14:32:11, sohanjg wrote:
> > On 2014/07/09 14:15:30, reveman wrote:
> > > What's the difference between this and only releasing resources when the
> tile
> > is
> > > ready to be deleted? It's a subtle difference to the code so if it has a
> > > critical impact on the behavior we should document it. Can you please
> describe
> > a
> > > use case where this makes a difference and what the difference is.
> > 
> > Hmm, 1 thing could be we optimize resource usage (if at all, till we hit
> memory
> > bump). Isnt it better to release whatever is releasable, rather than holding
> it
> > back ?
> > 
> > You and vmpstr@ are the best judge here.
> 
> I think at this point the tile is set to be deleted at one point or another,
so
> the memory will definitely be released. If we keep the resources until we
> actually delete, then we technically would not be able to release any of those
> tile resources any other way (this tile is no longer accessible via
> AssignGpuMemory).

Maybe we should make this a bit more explicit by adding another function called
something like FreeResourcesForReleasedTiles() that takes care of this. This
function would be responsible for freeing resources and the existing
CleanUpReleasedTiles() function would simply cleanup tiles and DCHECK to ensure
that all resources have already been freed.

> 
> I don't really think it's that much of a difference, since the task will be
> cancelled or finish raster and then be cleaned up, but I prefer this approach
as
> it states a bit more explicitly that all tile resources at this point are
> released (the fact that we hold on to the tile has nothing to do with the
> resources, but with the task). 

What I find a bit awkward is that we still need to make another one of these
loops that free all resources later when all pending raster tasks have finished
as you then have more resources on the tile than before. It all feels a bit too
subtle to me and while this might not be so fragile right now, I'm worried
future changes could change that.

I'd prefer if we only released resources when the tile is ready to be cleaned up
or add some properly named function that makes it more clear that we're
releasing resources as a separate step from actually cleaning up the tiles.

[vmpstr, 2014-07-09 23:15:15]

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

https://codereview.chromium.org/366113002/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:427: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
> Maybe we should make this a bit more explicit by adding another function
called
> something like FreeResourcesForReleasedTiles() that takes care of this. This
> function would be responsible for freeing resources and the existing
> CleanUpReleasedTiles() function would simply cleanup tiles and DCHECK to
ensure
> that all resources have already been freed.

That sounds good to me. 

> 
> > 
> > I don't really think it's that much of a difference, since the task will be
> > cancelled or finish raster and then be cleaned up, but I prefer this
approach
> as
> > it states a bit more explicitly that all tile resources at this point are
> > released (the fact that we hold on to the tile has nothing to do with the
> > resources, but with the task). 
> 
> What I find a bit awkward is that we still need to make another one of these
> loops that free all resources later when all pending raster tasks have
finished
> as you then have more resources on the tile than before. It all feels a bit
too
> subtle to me and while this might not be so fragile right now, I'm worried
> future changes could change that.
> 
> I'd prefer if we only released resources when the tile is ready to be cleaned
up
> or add some properly named function that makes it more clear that we're
> releasing resources as a separate step from actually cleaning up the tiles.

https://codereview.chromium.org/366113002/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:437: DCHECK(tiles_.find(tile->id()) !=
tiles_.end());
Sorry to backtrack a bit. My previous comment already has the iterators patch in
mind. In this case, the tile is still accessible in AssignGpuMem. Maybe it's
worth it to always erase it from tiles_ here, and only keep it in
released_tiles_? That way we will avoid scheduling this tile again if we run
assignmem again. When the raster task completes then, it won't find tiles_
member corresponding to the task and release the resource immediately.

[reveman, 2014-07-10 11:23:04]

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

https://codereview.chromium.org/366113002/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:437: DCHECK(tiles_.find(tile->id()) !=
tiles_.end());
On 2014/07/09 23:15:14, vmpstr wrote:
> Sorry to backtrack a bit. My previous comment already has the iterators patch
in
> mind. In this case, the tile is still accessible in AssignGpuMem. Maybe it's
> worth it to always erase it from tiles_ here, and only keep it in
> released_tiles_? That way we will avoid scheduling this tile again if we run
> assignmem again. When the raster task completes then, it won't find tiles_
> member corresponding to the task and release the resource immediately. 

I prefer to keep it in |tiles_| and change the "it == tiles_.end()" check in
OnRasterTaskCompleted to a DCHECK. It shouldn't make a difference in terms of
how quickly we release resources as OnRasterTaskCompleted will only be called
when we check for completed tasks and we should clean up released tiles after
each of those calls.

[sohanjg, 2014-07-10 13:29:45]

Can you please take a look.
Thanks.

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

https://codereview.chromium.org/366113002/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:427: FreeResourceForTile(tile,
static_cast<RasterMode>(mode));
On 2014/07/09 23:15:14, vmpstr wrote:
> > Maybe we should make this a bit more explicit by adding another function
> called
> > something like FreeResourcesForReleasedTiles() that takes care of this. This
> > function would be responsible for freeing resources and the existing
> > CleanUpReleasedTiles() function would simply cleanup tiles and DCHECK to
> ensure
> > that all resources have already been freed.
> 
> That sounds good to me. 
> 
> > 
> > > 
> > > I don't really think it's that much of a difference, since the task will
be
> > > cancelled or finish raster and then be cleaned up, but I prefer this
> approach
> > as
> > > it states a bit more explicitly that all tile resources at this point are
> > > released (the fact that we hold on to the tile has nothing to do with the
> > > resources, but with the task). 
> > 
> > What I find a bit awkward is that we still need to make another one of these
> > loops that free all resources later when all pending raster tasks have
> finished
> > as you then have more resources on the tile than before. It all feels a bit
> too
> > subtle to me and while this might not be so fragile right now, I'm worried
> > future changes could change that.
> > 
> > I'd prefer if we only released resources when the tile is ready to be
cleaned
> up
> > or add some properly named function that makes it more clear that we're
> > releasing resources as a separate step from actually cleaning up the tiles.
> 

Done.

https://codereview.chromium.org/366113002/diff/100001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:437: DCHECK(tiles_.find(tile->id()) !=
tiles_.end());
On 2014/07/10 11:23:03, reveman wrote:
> On 2014/07/09 23:15:14, vmpstr wrote:
> > Sorry to backtrack a bit. My previous comment already has the iterators
patch
> in
> > mind. In this case, the tile is still accessible in AssignGpuMem. Maybe it's
> > worth it to always erase it from tiles_ here, and only keep it in
> > released_tiles_? That way we will avoid scheduling this tile again if we run
> > assignmem again. When the raster task completes then, it won't find tiles_
> > member corresponding to the task and release the resource immediately. 
> 
> I prefer to keep it in |tiles_| and change the "it == tiles_.end()" check in
> OnRasterTaskCompleted to a DCHECK. It shouldn't make a difference in terms of
> how quickly we release resources as OnRasterTaskCompleted will only be called
> when we check for completed tasks and we should clean up released tiles after
> each of those calls.

Done.

[reveman, 2014-07-10 16:21:56]

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:1081: DCHECK(tile_version.raster_task_);
you can keep this DCHECK.

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

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:440: }
you can break out of this loop as soon as you find a raster task. I think it
would be nice to move this into a TileHasRasterTask utility function so that you
can also remove the temporary variable.

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
cc/resources/tile_manager.h:231: FreeResourcesForReleasedTiles();
please move this to a separate function or change the name of this ForTesting
function to reflect that it's now doing more than cleaning up released tiles.

[sohanjg, 2014-07-11 15:14:12]

Can you please take a look.
Thanks.

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:1081: DCHECK(tile_version.raster_task_);
On 2014/07/10 16:21:56, reveman wrote:
> you can keep this DCHECK.

Done.

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

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:440: }
On 2014/07/10 16:21:56, reveman wrote:
> you can break out of this loop as soon as you find a raster task. I think it
> would be nice to move this into a TileHasRasterTask utility function so that
you
> can also remove the temporary variable.

Done.

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/120001/cc/resources/tile_manag...
cc/resources/tile_manager.h:231: FreeResourcesForReleasedTiles();
On 2014/07/10 16:21:56, reveman wrote:
> please move this to a separate function or change the name of this ForTesting
> function to reflect that it's now doing more than cleaning up released tiles.

Done.

[reveman, 2014-07-11 17:06:37]

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:393: DCHECK_EQ(0u, tiles_.size());
why remove this DCHECK? this is very useful. please keep it.

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

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:431: DCHECK(tile);
I don't think this DCHECK is useful. please remove it.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:433: bool tile_has_raster_task = false;
you don't need this variable

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:436: tile_has_raster_task = true;
no need to iterate through all modes. just "return true" here instead.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:438: return tile_has_raster_task;
and just "return false" here.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:445: if (TileHasRasterTask(tile)) {
nit: I'd add a blank line before "if (TileHasR.." to separate that chunk of code
a bit. looks like "Tile* tile = *it;" is only useful for this check right now..

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:461: delete tile;
please add a DCHECK(!tile->HasResources()) here somewhere

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:246: bool TileHasRasterTask(Tile* tile);
does this have to be a member function? please move it to anonymous namespace in
.cc file if possible.

[sohanjg, 2014-07-12 11:41:59]

Can you please take a look.
Thanks.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:393: DCHECK_EQ(0u, tiles_.size());
On 2014/07/11 17:06:36, reveman wrote:
> why remove this DCHECK? this is very useful. please keep it.

Done.

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

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:431: DCHECK(tile);
On 2014/07/11 17:06:36, reveman wrote:
> I don't think this DCHECK is useful. please remove it.

Done.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:433: bool tile_has_raster_task = false;
On 2014/07/11 17:06:36, reveman wrote:
> you don't need this variable

Done.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:436: tile_has_raster_task = true;
On 2014/07/11 17:06:36, reveman wrote:
> no need to iterate through all modes. just "return true" here instead.

Done.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:438: return tile_has_raster_task;
On 2014/07/11 17:06:36, reveman wrote:
> and just "return false" here.

Done.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:445: if (TileHasRasterTask(tile)) {
On 2014/07/11 17:06:36, reveman wrote:
> nit: I'd add a blank line before "if (TileHasR.." to separate that chunk of
code
> a bit. looks like "Tile* tile = *it;" is only useful for this check right
now..

Done.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:461: delete tile;
On 2014/07/11 17:06:36, reveman wrote:
> please add a DCHECK(!tile->HasResources()) here somewhere

Done.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:246: bool TileHasRasterTask(Tile* tile);
On 2014/07/11 17:06:36, reveman wrote:
> does this have to be a member function? please move it to anonymous namespace
in
> .cc file if possible.

Looks like, managed_state and raster_task_ used in this func are pvt members and
cant be accessed from anonymous namespace.

[reveman, 2014-07-14 21:55:00]

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:246: bool TileHasRasterTask(Tile* tile);
On 2014/07/12 11:41:59, sohanjg wrote:
> On 2014/07/11 17:06:36, reveman wrote:
> > does this have to be a member function? please move it to anonymous
namespace
> in
> > .cc file if possible.
> 
> Looks like, managed_state and raster_task_ used in this func are pvt members
and
> cant be accessed from anonymous namespace.

Ok, just make it static and private then unless there's a good reason it's
non-static and protected.

[sohanjg, 2014-07-15 05:36:38]

Can you please take a look.
Thanks.

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:246: bool TileHasRasterTask(Tile* tile);
On 2014/07/14 21:55:00, reveman wrote:
> On 2014/07/12 11:41:59, sohanjg wrote:
> > On 2014/07/11 17:06:36, reveman wrote:
> > > does this have to be a member function? please move it to anonymous
> namespace
> > in
> > > .cc file if possible.
> > 
> > Looks like, managed_state and raster_task_ used in this func are pvt members
> and
> > cant be accessed from anonymous namespace.
> 
> Ok, just make it static and private then unless there's a good reason it's
> non-static and protected.

Done.

[reveman, 2014-07-15 05:46:47]

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

https://codereview.chromium.org/366113002/diff/180001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:1586: ManagedTileState& mts =
tile->managed_state();
const ManagedTileState& mts = ...

https://codereview.chromium.org/366113002/diff/180001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/180001/cc/resources/tile_manag...
cc/resources/tile_manager.h:295: static bool TileHasRasterTask(Tile* tile);
can you make it "const Tile*" instead?

[sohanjg, 2014-07-15 06:20:06]

Can you please take a look,
Thanks.

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

https://codereview.chromium.org/366113002/diff/180001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:1586: ManagedTileState& mts =
tile->managed_state();
On 2014/07/15 05:46:47, reveman wrote:
> const ManagedTileState& mts = ...

Done.

https://codereview.chromium.org/366113002/diff/180001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/180001/cc/resources/tile_manag...
cc/resources/tile_manager.h:295: static bool TileHasRasterTask(Tile* tile);
On 2014/07/15 05:46:47, reveman wrote:
> can you make it "const Tile*" instead?

Done.

[reveman, 2014-07-15 15:28:31]

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

https://codereview.chromium.org/366113002/diff/200001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:463: CleanUpReleasedTiles();
Is it enough to clean up released tiles here? What if a tile is released that
has a pending raster task but we never get another call to ManageTiles after
that task completes? Do we have to cleanup tiles in DidFinishRunningTasks() too?

[sohanjg, 2014-07-16 13:01:34]

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

https://codereview.chromium.org/366113002/diff/200001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:463: CleanUpReleasedTiles();
On 2014/07/15 15:28:31, reveman wrote:
> Is it enough to clean up released tiles here? What if a tile is released that
> has a pending raster task but we never get another call to ManageTiles after
> that task completes? Do we have to cleanup tiles in DidFinishRunningTasks()
too?

It sounds reasonable...1 question wont we be able clean up tiles with pending
raster tasks during TileManager Dtor, if we dont get another ManageTiles ?

But, if we add cleanup in DidFinishRunningTasks, we are seeing a crash on heavy
pinch-zoom on nytimes desktop page in N5, at OnRasterTaskCompleted, while
accessing Tiles from the TileMap.

[reveman, 2014-07-16 16:49:02]

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

https://codereview.chromium.org/366113002/diff/200001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:463: CleanUpReleasedTiles();
On 2014/07/16 13:01:34, sohanjg wrote:
> On 2014/07/15 15:28:31, reveman wrote:
> > Is it enough to clean up released tiles here? What if a tile is released
that
> > has a pending raster task but we never get another call to ManageTiles after
> > that task completes? Do we have to cleanup tiles in DidFinishRunningTasks()
> too?
> 
> It sounds reasonable...1 question wont we be able clean up tiles with pending
> raster tasks during TileManager Dtor, if we dont get another ManageTiles ?

That's not good enough as the memory limit might require us to free these
resources to stay within the budget.

> 
> But, if we add cleanup in DidFinishRunningTasks, we are seeing a crash on
heavy
> pinch-zoom on nytimes desktop page in N5, at OnRasterTaskCompleted, while
> accessing Tiles from the TileMap.

Why? Where exactly are you doing the cleanup?

[sohanjg, 2014-07-16 17:47:59]

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

https://codereview.chromium.org/366113002/diff/200001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:463: CleanUpReleasedTiles();
On 2014/07/16 16:49:02, reveman wrote:
> On 2014/07/16 13:01:34, sohanjg wrote:
> > On 2014/07/15 15:28:31, reveman wrote:
> > > Is it enough to clean up released tiles here? What if a tile is released
> that
> > > has a pending raster task but we never get another call to ManageTiles
after
> > > that task completes? Do we have to cleanup tiles in
DidFinishRunningTasks()
> > too?
> > 
> > It sounds reasonable...1 question wont we be able clean up tiles with
pending
> > raster tasks during TileManager Dtor, if we dont get another ManageTiles ?
> 
> That's not good enough as the memory limit might require us to free these
> resources to stay within the budget.
OK, right.
> 
> > 
> > But, if we add cleanup in DidFinishRunningTasks, we are seeing a crash on
> heavy
> > pinch-zoom on nytimes desktop page in N5, at OnRasterTaskCompleted, while
> > accessing Tiles from the TileMap.
> 
> Why? Where exactly are you doing the cleanup?

I am doing FreeResourcesForReleasedTiles and cleanup after
CheckForCompletedTasks and before AssignGpuMemoryToTiles, in
DidFinishRunningTasks.

[reveman, 2014-07-16 21:46:16]

On 2014/07/16 17:47:59, sohanjg wrote:
>
https://codereview.chromium.org/366113002/diff/200001/cc/resources/tile_manag...
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/366113002/diff/200001/cc/resources/tile_manag...
> cc/resources/tile_manager.cc:463: CleanUpReleasedTiles();
> On 2014/07/16 16:49:02, reveman wrote:
> > On 2014/07/16 13:01:34, sohanjg wrote:
> > > On 2014/07/15 15:28:31, reveman wrote:
> > > > Is it enough to clean up released tiles here? What if a tile is released
> > that
> > > > has a pending raster task but we never get another call to ManageTiles
> after
> > > > that task completes? Do we have to cleanup tiles in
> DidFinishRunningTasks()
> > > too?
> > > 
> > > It sounds reasonable...1 question wont we be able clean up tiles with
> pending
> > > raster tasks during TileManager Dtor, if we dont get another ManageTiles ?
> > 
> > That's not good enough as the memory limit might require us to free these
> > resources to stay within the budget.
> OK, right.
> > 
> > > 
> > > But, if we add cleanup in DidFinishRunningTasks, we are seeing a crash on
> > heavy
> > > pinch-zoom on nytimes desktop page in N5, at OnRasterTaskCompleted, while
> > > accessing Tiles from the TileMap.
> > 
> > Why? Where exactly are you doing the cleanup?
> 
> I am doing FreeResourcesForReleasedTiles and cleanup after
> CheckForCompletedTasks and before AssignGpuMemoryToTiles, in
> DidFinishRunningTasks.

I think it might be better to do this right before ReduceResourceUsage(). I'm
guessing the crash is because prioritized tile set becomes invalid whenever you
modify |tiles_|. You probably need to somehow make sure we don't use this when
invalid.

[sohanjg, 2014-07-17 07:51:17]

On 2014/07/16 21:46:16, reveman wrote:
> On 2014/07/16 17:47:59, sohanjg wrote:
> >
>
https://codereview.chromium.org/366113002/diff/200001/cc/resources/tile_manag...
> > File cc/resources/tile_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/366113002/diff/200001/cc/resources/tile_manag...
> > cc/resources/tile_manager.cc:463: CleanUpReleasedTiles();
> > On 2014/07/16 16:49:02, reveman wrote:
> > > On 2014/07/16 13:01:34, sohanjg wrote:
> > > > On 2014/07/15 15:28:31, reveman wrote:
> > > > > Is it enough to clean up released tiles here? What if a tile is
released
> > > that
> > > > > has a pending raster task but we never get another call to ManageTiles
> > after
> > > > > that task completes? Do we have to cleanup tiles in
> > DidFinishRunningTasks()
> > > > too?
> > > > 
> > > > It sounds reasonable...1 question wont we be able clean up tiles with
> > pending
> > > > raster tasks during TileManager Dtor, if we dont get another ManageTiles
?
> > > 
> > > That's not good enough as the memory limit might require us to free these
> > > resources to stay within the budget.
> > OK, right.
> > > 
> > > > 
> > > > But, if we add cleanup in DidFinishRunningTasks, we are seeing a crash
on
> > > heavy
> > > > pinch-zoom on nytimes desktop page in N5, at OnRasterTaskCompleted,
while
> > > > accessing Tiles from the TileMap.
> > > 
> > > Why? Where exactly are you doing the cleanup?
> > 
> > I am doing FreeResourcesForReleasedTiles and cleanup after
> > CheckForCompletedTasks and before AssignGpuMemoryToTiles, in
> > DidFinishRunningTasks.
> 
> I think it might be better to do this right before ReduceResourceUsage(). I'm
> guessing the crash is because prioritized tile set becomes invalid whenever
you
> modify |tiles_|. You probably need to somehow make sure we don't use this when
> invalid.

That works well.thanks.
Can you have a look.

[reveman, 2014-07-17 15:10:50]

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

https://codereview.chromium.org/366113002/diff/220001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:431: void TileManager::CleanUpReleasedTiles() {
How about adding a DCHECK(prioritized_tiles_dirty_) here to prevent this from
being called in a way that could potentially cause a crash?

https://codereview.chromium.org/366113002/diff/220001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:497: CleanUpReleasedTiles();
I don't think you should do the cleanup here as that leaves |prioritized_tiles_|
invalid and there's currently no guarantee that it will be rebuilt before being
used again afaict.

Just calling FreeResourcesForReleasedTiles() here should be enough though.
Cleaning up the actual instances can wait until
UpdatePrioritizedTileSetIfNeeded() or the dtor is called.

[sohanjg, 2014-07-17 15:58:33]

Please take a look, Thanks.

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

https://codereview.chromium.org/366113002/diff/220001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:431: void TileManager::CleanUpReleasedTiles() {
On 2014/07/17 15:10:50, reveman wrote:
> How about adding a DCHECK(prioritized_tiles_dirty_) here to prevent this from
> being called in a way that could potentially cause a crash?

Done.

https://codereview.chromium.org/366113002/diff/220001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:497: CleanUpReleasedTiles();
On 2014/07/17 15:10:50, reveman wrote:
> I don't think you should do the cleanup here as that leaves
|prioritized_tiles_|
> invalid and there's currently no guarantee that it will be rebuilt before
being
> used again afaict.
> 
> Just calling FreeResourcesForReleasedTiles() here should be enough though.
> Cleaning up the actual instances can wait until
> UpdatePrioritizedTileSetIfNeeded() or the dtor is called.

Done.

[reveman, 2014-07-17 16:03:38]

lgtm

[sohanjg, 2014-07-18 00:41:37]

On 2014/07/17 16:03:38, reveman wrote:
> lgtm
Vlad,do u have any inputs before we commit this ?

[vmpstr, 2014-07-18 18:03:53]

You need to clear the TilePriority for both the active and the pending trees so
that we don't schedule released tiles again.

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

https://codereview.chromium.org/366113002/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:426: for (int mode = 0; mode < NUM_RASTER_MODES;
++mode)
nit: This loop can be replaced with FreeResourcesForTile(tile);

https://codereview.chromium.org/366113002/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:1589: bool TileManager::TileHasRasterTask(const
Tile* tile) {
Is it better if this is a Tile member? Or are we avoiding putting raster task
knowledge into tiles?

[reveman, 2014-07-18 19:18:14]

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

https://codereview.chromium.org/366113002/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:1589: bool TileManager::TileHasRasterTask(const
Tile* tile) {
On 2014/07/18 18:03:53, vmpstr wrote:
> Is it better if this is a Tile member? Or are we avoiding putting raster task
> knowledge into tiles?

Good idea. I'd prefer it as part of the Tile class too.

[sohanjg, 2014-07-21 10:09:07]

Looks like its better to do UpdatePrioritizedTileSetIfNeeded,  in
DidFinishRunningTasks once we do CheckForCompletedTasks, this will take care of
the clean up better (and we wouldnt schedule released tiles again).
Please have a look. Thanks.

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

https://codereview.chromium.org/366113002/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:426: for (int mode = 0; mode < NUM_RASTER_MODES;
++mode)
On 2014/07/18 18:03:53, vmpstr wrote:
> nit: This loop can be replaced with FreeResourcesForTile(tile);

Done.

https://codereview.chromium.org/366113002/diff/240001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:1589: bool TileManager::TileHasRasterTask(const
Tile* tile) {
On 2014/07/18 19:18:14, reveman wrote:
> On 2014/07/18 18:03:53, vmpstr wrote:
> > Is it better if this is a Tile member? Or are we avoiding putting raster
task
> > knowledge into tiles?
> 
> Good idea. I'd prefer it as part of the Tile class too.

Done.

[reveman, 2014-07-21 12:27:50]

not lgtm

https://codereview.chromium.org/366113002/diff/260001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/260001/cc/resources/tile.h#new...
cc/resources/tile.h:177: bool HasRasterTask();
bool HasRasterTask() const;

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

https://codereview.chromium.org/366113002/diff/260001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:485: UpdatePrioritizedTileSetIfNeeded();
This doesn't work as prioritized tile set will only be updated as a result of
ManageTiles being called. Even if it worked, we do not want to update the
prioritized tile set here.

It's probably a good idea to reset the priority in ::Release(Tile* tile) but I
don't think more than that is needed. There's a small chance that
DidFinishRunningTasks() is called before ManageTiles() after a tile has been
released but it's not incorrect to schedule the tile again in that case. It's
actually more correct to allow this tile to be scheduled again as the new
priority should not take effect until ManageTiles is called.

https://codereview.chromium.org/366113002/diff/260001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/260001/cc/resources/tile_manag...
cc/resources/tile_manager.h:295: static bool TileHasRasterTask(const Tile*
tile);
no longer used

[sohanjg, 2014-07-21 12:39:14]

Sorry for the confusion.
Please take a look, thanks,

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

https://codereview.chromium.org/366113002/diff/260001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:485: UpdatePrioritizedTileSetIfNeeded();
On 2014/07/21 12:27:49, reveman wrote:
> This doesn't work as prioritized tile set will only be updated as a result of
> ManageTiles being called. Even if it worked, we do not want to update the
> prioritized tile set here.
> 
> It's probably a good idea to reset the priority in ::Release(Tile* tile) but I
> don't think more than that is needed. There's a small chance that
> DidFinishRunningTasks() is called before ManageTiles() after a tile has been
> released but it's not incorrect to schedule the tile again in that case. It's
> actually more correct to allow this tile to be scheduled again as the new
> priority should not take effect until ManageTiles is called.

Done.
Sorry for updating prioritized tile set wrongly. 
I didnt cq it, just tried some bots.
I have reverted to old code.

https://codereview.chromium.org/366113002/diff/260001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/260001/cc/resources/tile_manag...
cc/resources/tile_manager.h:295: static bool TileHasRasterTask(const Tile*
tile);
On 2014/07/21 12:27:49, reveman wrote:
> no longer used

Done.

[reveman, 2014-07-21 12:54:26]

https://codereview.chromium.org/366113002/diff/280001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/280001/cc/resources/tile.h#new...
cc/resources/tile.h:177: bool HasRasterTask();
I still like this 'const'

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

https://codereview.chromium.org/366113002/diff/280001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:408: void TileManager::Release(Tile* tile) {
I think what vmpstr@ asked you to do is reset the tile priority here to be sure
that the tile is not left around with a stale high priority that will cause it
to be rescheduled even after a call to ManageTiles. I would prefer if we instead
DCHECK(TilePriority() == tile->combined_priority()) and if that doesn't work we
evaluate if this or some other place is the best place to reset the prioirty.

[sohanjg, 2014-07-21 14:43:13]

Please take a look, thanks.

https://codereview.chromium.org/366113002/diff/280001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/280001/cc/resources/tile.h#new...
cc/resources/tile.h:177: bool HasRasterTask();
On 2014/07/21 12:54:26, reveman wrote:
> I still like this 'const'

Done.

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

https://codereview.chromium.org/366113002/diff/280001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:408: void TileManager::Release(Tile* tile) {
On 2014/07/21 12:54:26, reveman wrote:
> I think what vmpstr@ asked you to do is reset the tile priority here to be
sure
> that the tile is not left around with a stale high priority that will cause it
> to be rescheduled even after a call to ManageTiles. I would prefer if we
instead
> DCHECK(TilePriority() == tile->combined_priority()) and if that doesn't work
we
> evaluate if this or some other place is the best place to reset the prioirty.

Done.

Sorry, i misunderstood his comments, as the remaining comments were in
DidFinishRunningTasks, i thought we may need to handle priority there itself.
But, with the current code, even though we are not failing in dcheck here, there
is crash in ::CheckForCompletedRasterizerTasks when we access,
task->AsRasterTask(), looks like we are accessing some invalid mem here.

[reveman, 2014-07-21 14:57:02]

lgtm with nit

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

https://codereview.chromium.org/366113002/diff/300001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:1084: Tile* tile = it->second;
nit: you don't really need the iterator anymore. please replace lines 1081-1084
with:

DCHECK(tiles_.find(tile_id) != tiles_.end());
Tile* tile = tiles_[tile_id];

[vmpstr, 2014-07-21 15:24:15]

Overall, LG, just some questions about dchecks.

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:409: DCHECK(TilePriority() ==
tile->combined_priority());
Why does this work? I don't think we have code anywhere that would reset the
priority. That is, if we invalidate a NOW tile, it will still have NOW priority
here. If I'm missing something, please add a comment.

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: DCHECK(prioritized_tiles_dirty_);
This is kind of weird. Why are we requiring this to be true? (please add a small
comment explaining)

[sohanjg, 2014-07-22 14:17:47]

With latest rebase there is this always crash on android N5, at ::
CheckForCompletedRasterizerTasks , while accessing raster task,
task->AsRasterTask()

Maybe we are messing up completed tasks somehow, i am checking it, please let me
know, if you have some comments.

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:409: DCHECK(TilePriority() ==
tile->combined_priority());
On 2014/07/21 15:24:14, vmpstr wrote:
> Why does this work? I don't think we have code anywhere that would reset the
> priority. That is, if we invalidate a NOW tile, it will still have NOW
priority
> here. If I'm missing something, please add a comment.

Ideally, we should do SetPriority(TREE, TilePriority()) for active and pending,
but we assume it is already the case, and dcheck if its not ? should we change
this ?

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: DCHECK(prioritized_tiles_dirty_);
On 2014/07/21 15:24:14, vmpstr wrote:
> This is kind of weird. Why are we requiring this to be true? (please add a
small
> comment explaining)

hmm..we had earlier seen some crashes when we cleaned up released tiles from
DidFinishRunningTasks, because maybe prioritized tile set is affected when we
modify the tiles vector.
so we had put this extra protective code here.

[vmpstr, 2014-07-22 15:17:30]

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:409: DCHECK(TilePriority() ==
tile->combined_priority());
On 2014/07/22 14:17:46, sohanjg wrote:
> On 2014/07/21 15:24:14, vmpstr wrote:
> > Why does this work? I don't think we have code anywhere that would reset the
> > priority. That is, if we invalidate a NOW tile, it will still have NOW
> priority
> > here. If I'm missing something, please add a comment.
> 
> Ideally, we should do SetPriority(TREE, TilePriority()) for active and
pending,
> but we assume it is already the case, and dcheck if its not ? should we change
> this ?

That's my question... I don't think it's the case right now. Tiles are released
by the tiling during invalidation, among other paths, and I'm pretty sure we
don't touch the priority when we do that. I think this DCHECK has to be
SetPriority(ACTIVE_TREE, TilePriority()); SetPriority(PENDING_TREE,
TilePriority()) instead with a comment saying that this is so that the tile is
not scheduled again.

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: DCHECK(prioritized_tiles_dirty_);
On 2014/07/22 14:17:46, sohanjg wrote:
> On 2014/07/21 15:24:14, vmpstr wrote:
> > This is kind of weird. Why are we requiring this to be true? (please add a
> small
> > comment explaining)
> 
> hmm..we had earlier seen some crashes when we cleaned up released tiles from
> DidFinishRunningTasks, because maybe prioritized tile set is affected when we
> modify the tiles vector.
> so we had put this extra protective code here.

Ah ok. This still makes it a bit fragile. 

The problem is that prioritized set is a view on tiles_ basically. It has no
ownership, it just returns pointers in a certain order. If we remove something
from tiles_ we have to ensure that the next thing we do is rebuild the
prioritized tile set, which I guess explains that DCHECK. The fragile part is
that prioritized_tiles_dirty_ is set elsewhere, so there is no strict guarantee
that we will rebuild the set (although it seems to be the case now). I guess
that's OK. Maybe add a comment in .h above this function saying that we have to
rebuild the set before accessing it?

[reveman, 2014-07-22 16:21:21]

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:409: DCHECK(TilePriority() ==
tile->combined_priority());
On 2014/07/22 15:17:29, vmpstr wrote:
> On 2014/07/22 14:17:46, sohanjg wrote:
> > On 2014/07/21 15:24:14, vmpstr wrote:
> > > Why does this work? I don't think we have code anywhere that would reset
the
> > > priority. That is, if we invalidate a NOW tile, it will still have NOW
> > priority
> > > here. If I'm missing something, please add a comment.
> > 
> > Ideally, we should do SetPriority(TREE, TilePriority()) for active and
> pending,
> > but we assume it is already the case, and dcheck if its not ? should we
change
> > this ?
> 
> That's my question... I don't think it's the case right now. Tiles are
released
> by the tiling during invalidation, among other paths, and I'm pretty sure we
> don't touch the priority when we do that. I think this DCHECK has to be
> SetPriority(ACTIVE_TREE, TilePriority()); SetPriority(PENDING_TREE,
> TilePriority()) instead with a comment saying that this is so that the tile is
> not scheduled again.

Please give some example usage that justifies having the ability to drop tile
references but not reset the priority. I'm fine resetting it here if needed, I
just prefer to have a concrete example of why it's needed rather than assume it
is.

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: DCHECK(prioritized_tiles_dirty_);
On 2014/07/22 15:17:29, vmpstr wrote:
> On 2014/07/22 14:17:46, sohanjg wrote:
> > On 2014/07/21 15:24:14, vmpstr wrote:
> > > This is kind of weird. Why are we requiring this to be true? (please add a
> > small
> > > comment explaining)
> > 
> > hmm..we had earlier seen some crashes when we cleaned up released tiles from
> > DidFinishRunningTasks, because maybe prioritized tile set is affected when
we
> > modify the tiles vector.
> > so we had put this extra protective code here.
> 
> Ah ok. This still makes it a bit fragile. 
> 
> The problem is that prioritized set is a view on tiles_ basically. It has no
> ownership, it just returns pointers in a certain order. If we remove something
> from tiles_ we have to ensure that the next thing we do is rebuild the
> prioritized tile set, which I guess explains that DCHECK. The fragile part is
> that prioritized_tiles_dirty_ is set elsewhere, so there is no strict
guarantee
> that we will rebuild the set (although it seems to be the case now). I guess
> that's OK. Maybe add a comment in .h above this function saying that we have
to
> rebuild the set before accessing it?

After, right? The idea with this DCHECK is that we need to make sure that the
prioritized tile set is rebuild after calling this as we may otherwise use a set
with deleted tiles. A comment explaining this would be nice.

If this DCHECK fails, it's probably because of the call to this function in the
dtor as the call in ::UpdatePrioritizedTileSetIfNeeded() can of course enver
cause it to fail.

[vmpstr, 2014-07-22 16:31:02]

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:409: DCHECK(TilePriority() ==
tile->combined_priority());
On 2014/07/22 16:21:21, reveman wrote:
> On 2014/07/22 15:17:29, vmpstr wrote:
> > On 2014/07/22 14:17:46, sohanjg wrote:
> > > On 2014/07/21 15:24:14, vmpstr wrote:
> > > > Why does this work? I don't think we have code anywhere that would reset
> the
> > > > priority. That is, if we invalidate a NOW tile, it will still have NOW
> > > priority
> > > > here. If I'm missing something, please add a comment.
> > > 
> > > Ideally, we should do SetPriority(TREE, TilePriority()) for active and
> > pending,
> > > but we assume it is already the case, and dcheck if its not ? should we
> change
> > > this ?
> > 
> > That's my question... I don't think it's the case right now. Tiles are
> released
> > by the tiling during invalidation, among other paths, and I'm pretty sure we
> > don't touch the priority when we do that. I think this DCHECK has to be
> > SetPriority(ACTIVE_TREE, TilePriority()); SetPriority(PENDING_TREE,
> > TilePriority()) instead with a comment saying that this is so that the tile
is
> > not scheduled again.
> 
> Please give some example usage that justifies having the ability to drop tile
> references but not reset the priority. I'm fine resetting it here if needed, I
> just prefer to have a concrete example of why it's needed rather than assume
it
> is.

Suppose we have some tiles on the active tree with all priorities up to date,
then the live tiles rect moves down and we release a top row of tiles (just
active tree in this case). This just removes references from the tiling without
setting priorities to be empty. It's not really a justification of why we don't
do it, it's just a fact that we don't (at least IIRC). In general, when tilings
remove tiles, priorities aren't affected. From there, it's implicit that
whatever is released is not needed. 

I think invalidation case works, because we have
DidBecomeRecycled/DidBecomeActive things that play around with priorities.
However, on a long enough page, I think we will hit the dcheck once the live
tiles rect starts culling tiles.

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: DCHECK(prioritized_tiles_dirty_);
On 2014/07/22 16:21:21, reveman wrote:
> On 2014/07/22 15:17:29, vmpstr wrote:
> > On 2014/07/22 14:17:46, sohanjg wrote:
> > > On 2014/07/21 15:24:14, vmpstr wrote:
> > > > This is kind of weird. Why are we requiring this to be true? (please add
a
> > > small
> > > > comment explaining)
> > > 
> > > hmm..we had earlier seen some crashes when we cleaned up released tiles
from
> > > DidFinishRunningTasks, because maybe prioritized tile set is affected when
> we
> > > modify the tiles vector.
> > > so we had put this extra protective code here.
> > 
> > Ah ok. This still makes it a bit fragile. 
> > 
> > The problem is that prioritized set is a view on tiles_ basically. It has no
> > ownership, it just returns pointers in a certain order. If we remove
something
> > from tiles_ we have to ensure that the next thing we do is rebuild the
> > prioritized tile set, which I guess explains that DCHECK. The fragile part
is
> > that prioritized_tiles_dirty_ is set elsewhere, so there is no strict
> guarantee
> > that we will rebuild the set (although it seems to be the case now). I guess
> > that's OK. Maybe add a comment in .h above this function saying that we have
> to
> > rebuild the set before accessing it?
> 
> After, right? The idea with this DCHECK is that we need to make sure that the
> prioritized tile set is rebuild after calling this as we may otherwise use a
set
> with deleted tiles. A comment explaining this would be nice.
> 
> If this DCHECK fails, it's probably because of the call to this function in
the
> dtor as the call in ::UpdatePrioritizedTileSetIfNeeded() can of course enver
> cause it to fail.

Yeah, after. My only worry is that we set prioritized_tiles_dirty_ to true in
DidChangeTilePriority for example, which allows this function to be called with
no promise of rebuilding the set. (DidFinishRunningTasks for instance doesn't
rebuild the set, even if this flag is set).

[reveman, 2014-07-22 16:51:08]

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:409: DCHECK(TilePriority() ==
tile->combined_priority());
On 2014/07/22 16:31:02, vmpstr wrote:
> On 2014/07/22 16:21:21, reveman wrote:
> > On 2014/07/22 15:17:29, vmpstr wrote:
> > > On 2014/07/22 14:17:46, sohanjg wrote:
> > > > On 2014/07/21 15:24:14, vmpstr wrote:
> > > > > Why does this work? I don't think we have code anywhere that would
reset
> > the
> > > > > priority. That is, if we invalidate a NOW tile, it will still have NOW
> > > > priority
> > > > > here. If I'm missing something, please add a comment.
> > > > 
> > > > Ideally, we should do SetPriority(TREE, TilePriority()) for active and
> > > pending,
> > > > but we assume it is already the case, and dcheck if its not ? should we
> > change
> > > > this ?
> > > 
> > > That's my question... I don't think it's the case right now. Tiles are
> > released
> > > by the tiling during invalidation, among other paths, and I'm pretty sure
we
> > > don't touch the priority when we do that. I think this DCHECK has to be
> > > SetPriority(ACTIVE_TREE, TilePriority()); SetPriority(PENDING_TREE,
> > > TilePriority()) instead with a comment saying that this is so that the
tile
> is
> > > not scheduled again.
> > 
> > Please give some example usage that justifies having the ability to drop
tile
> > references but not reset the priority. I'm fine resetting it here if needed,
I
> > just prefer to have a concrete example of why it's needed rather than assume
> it
> > is.
> 
> Suppose we have some tiles on the active tree with all priorities up to date,
> then the live tiles rect moves down and we release a top row of tiles (just
> active tree in this case). This just removes references from the tiling
without
> setting priorities to be empty. It's not really a justification of why we
don't
> do it, it's just a fact that we don't (at least IIRC). In general, when
tilings
> remove tiles, priorities aren't affected. From there, it's implicit that
> whatever is released is not needed. 
> 
> I think invalidation case works, because we have
> DidBecomeRecycled/DidBecomeActive things that play around with priorities.
> However, on a long enough page, I think we will hit the dcheck once the live
> tiles rect starts culling tiles. 

So I guess my next question is; does it make more sense to reset the priority
when removing references from a tiling than doing it here? When the live tile
rect moves and we decide to drop the top row of tiles it seems like it's a
change in priority of those tiles that causes us to do this and it might be
appropriate to have the actual tile priority reflect that. Also sounds like
doing it here would often cause us to reset it when not needed.

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: DCHECK(prioritized_tiles_dirty_);
On 2014/07/22 16:31:02, vmpstr wrote:
> On 2014/07/22 16:21:21, reveman wrote:
> > On 2014/07/22 15:17:29, vmpstr wrote:
> > > On 2014/07/22 14:17:46, sohanjg wrote:
> > > > On 2014/07/21 15:24:14, vmpstr wrote:
> > > > > This is kind of weird. Why are we requiring this to be true? (please
add
> a
> > > > small
> > > > > comment explaining)
> > > > 
> > > > hmm..we had earlier seen some crashes when we cleaned up released tiles
> from
> > > > DidFinishRunningTasks, because maybe prioritized tile set is affected
when
> > we
> > > > modify the tiles vector.
> > > > so we had put this extra protective code here.
> > > 
> > > Ah ok. This still makes it a bit fragile. 
> > > 
> > > The problem is that prioritized set is a view on tiles_ basically. It has
no
> > > ownership, it just returns pointers in a certain order. If we remove
> something
> > > from tiles_ we have to ensure that the next thing we do is rebuild the
> > > prioritized tile set, which I guess explains that DCHECK. The fragile part
> is
> > > that prioritized_tiles_dirty_ is set elsewhere, so there is no strict
> > guarantee
> > > that we will rebuild the set (although it seems to be the case now). I
guess
> > > that's OK. Maybe add a comment in .h above this function saying that we
have
> > to
> > > rebuild the set before accessing it?
> > 
> > After, right? The idea with this DCHECK is that we need to make sure that
the
> > prioritized tile set is rebuild after calling this as we may otherwise use a
> set
> > with deleted tiles. A comment explaining this would be nice.
> > 
> > If this DCHECK fails, it's probably because of the call to this function in
> the
> > dtor as the call in ::UpdatePrioritizedTileSetIfNeeded() can of course enver
> > cause it to fail.
> 
> Yeah, after. My only worry is that we set prioritized_tiles_dirty_ to true in
> DidChangeTilePriority for example, which allows this function to be called
with
> no promise of rebuilding the set. (DidFinishRunningTasks for instance doesn't
> rebuild the set, even if this flag is set). 

Yea, the DCHECK is not perfect. We could just move all this code to
UpdatePrioritizedTileSetIfNeeded() instead. Just a bit ugly to have to call that
from the dtor..

[vmpstr, 2014-07-22 17:23:34]

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:409: DCHECK(TilePriority() ==
tile->combined_priority());
On 2014/07/22 16:51:08, reveman wrote:
> On 2014/07/22 16:31:02, vmpstr wrote:
> > On 2014/07/22 16:21:21, reveman wrote:
> > > On 2014/07/22 15:17:29, vmpstr wrote:
> > > > On 2014/07/22 14:17:46, sohanjg wrote:
> > > > > On 2014/07/21 15:24:14, vmpstr wrote:
> > > > > > Why does this work? I don't think we have code anywhere that would
> reset
> > > the
> > > > > > priority. That is, if we invalidate a NOW tile, it will still have
NOW
> > > > > priority
> > > > > > here. If I'm missing something, please add a comment.
> > > > > 
> > > > > Ideally, we should do SetPriority(TREE, TilePriority()) for active and
> > > > pending,
> > > > > but we assume it is already the case, and dcheck if its not ? should
we
> > > change
> > > > > this ?
> > > > 
> > > > That's my question... I don't think it's the case right now. Tiles are
> > > released
> > > > by the tiling during invalidation, among other paths, and I'm pretty
sure
> we
> > > > don't touch the priority when we do that. I think this DCHECK has to be
> > > > SetPriority(ACTIVE_TREE, TilePriority()); SetPriority(PENDING_TREE,
> > > > TilePriority()) instead with a comment saying that this is so that the
> tile
> > is
> > > > not scheduled again.
> > > 
> > > Please give some example usage that justifies having the ability to drop
> tile
> > > references but not reset the priority. I'm fine resetting it here if
needed,
> I
> > > just prefer to have a concrete example of why it's needed rather than
assume
> > it
> > > is.
> > 
> > Suppose we have some tiles on the active tree with all priorities up to
date,
> > then the live tiles rect moves down and we release a top row of tiles (just
> > active tree in this case). This just removes references from the tiling
> without
> > setting priorities to be empty. It's not really a justification of why we
> don't
> > do it, it's just a fact that we don't (at least IIRC). In general, when
> tilings
> > remove tiles, priorities aren't affected. From there, it's implicit that
> > whatever is released is not needed. 
> > 
> > I think invalidation case works, because we have
> > DidBecomeRecycled/DidBecomeActive things that play around with priorities.
> > However, on a long enough page, I think we will hit the dcheck once the live
> > tiles rect starts culling tiles. 
> 
> So I guess my next question is; does it make more sense to reset the priority
> when removing references from a tiling than doing it here? When the live tile
> rect moves and we decide to drop the top row of tiles it seems like it's a
> change in priority of those tiles that causes us to do this and it might be
> appropriate to have the actual tile priority reflect that. Also sounds like
> doing it here would often cause us to reset it when not needed.

I'm not sure I like the approach of clearing the priority when we drop tiles in
the tiling. From the tiling's perspective, the tile is gone. It's kind of akin
to clearing some variables before calling delete on an object: it's a bit
awkward and not obvious why it's required.

The fact that released tiles end up in tile manager is something that tilings
aren't aware of. I think it's a bit of a can of worms, since it's not crystal
clear to me how we clear priorities on those tiles even if they don't end up in
the tile manager (ie, they are still alive on the twin tiling). I _think_ it's
DidBecomeRecycled or DidBecomeActive calls on the twin that end up clearing
these priorities.

I mean, in the end I would just still like to reiterate that I think the fact
that tile manager requires released tiles to have default priority so that it
doesn't schedule the tile again should be the responsibility of the tile
manager. I would hate to see a comment like "Clear priorities so tile manager
doesn't schedule this tile I'm about to delete" in the tiling.

[reveman, 2014-07-22 18:02:47]

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:409: DCHECK(TilePriority() ==
tile->combined_priority());
On 2014/07/22 17:23:34, vmpstr wrote:
> On 2014/07/22 16:51:08, reveman wrote:
> > On 2014/07/22 16:31:02, vmpstr wrote:
> > > On 2014/07/22 16:21:21, reveman wrote:
> > > > On 2014/07/22 15:17:29, vmpstr wrote:
> > > > > On 2014/07/22 14:17:46, sohanjg wrote:
> > > > > > On 2014/07/21 15:24:14, vmpstr wrote:
> > > > > > > Why does this work? I don't think we have code anywhere that would
> > reset
> > > > the
> > > > > > > priority. That is, if we invalidate a NOW tile, it will still have
> NOW
> > > > > > priority
> > > > > > > here. If I'm missing something, please add a comment.
> > > > > > 
> > > > > > Ideally, we should do SetPriority(TREE, TilePriority()) for active
and
> > > > > pending,
> > > > > > but we assume it is already the case, and dcheck if its not ? should
> we
> > > > change
> > > > > > this ?
> > > > > 
> > > > > That's my question... I don't think it's the case right now. Tiles are
> > > > released
> > > > > by the tiling during invalidation, among other paths, and I'm pretty
> sure
> > we
> > > > > don't touch the priority when we do that. I think this DCHECK has to
be
> > > > > SetPriority(ACTIVE_TREE, TilePriority()); SetPriority(PENDING_TREE,
> > > > > TilePriority()) instead with a comment saying that this is so that the
> > tile
> > > is
> > > > > not scheduled again.
> > > > 
> > > > Please give some example usage that justifies having the ability to drop
> > tile
> > > > references but not reset the priority. I'm fine resetting it here if
> needed,
> > I
> > > > just prefer to have a concrete example of why it's needed rather than
> assume
> > > it
> > > > is.
> > > 
> > > Suppose we have some tiles on the active tree with all priorities up to
> date,
> > > then the live tiles rect moves down and we release a top row of tiles
(just
> > > active tree in this case). This just removes references from the tiling
> > without
> > > setting priorities to be empty. It's not really a justification of why we
> > don't
> > > do it, it's just a fact that we don't (at least IIRC). In general, when
> > tilings
> > > remove tiles, priorities aren't affected. From there, it's implicit that
> > > whatever is released is not needed. 
> > > 
> > > I think invalidation case works, because we have
> > > DidBecomeRecycled/DidBecomeActive things that play around with priorities.
> > > However, on a long enough page, I think we will hit the dcheck once the
live
> > > tiles rect starts culling tiles. 
> > 
> > So I guess my next question is; does it make more sense to reset the
priority
> > when removing references from a tiling than doing it here? When the live
tile
> > rect moves and we decide to drop the top row of tiles it seems like it's a
> > change in priority of those tiles that causes us to do this and it might be
> > appropriate to have the actual tile priority reflect that. Also sounds like
> > doing it here would often cause us to reset it when not needed.
> 
> I'm not sure I like the approach of clearing the priority when we drop tiles
in
> the tiling. From the tiling's perspective, the tile is gone. It's kind of akin
> to clearing some variables before calling delete on an object: it's a bit
> awkward and not obvious why it's required.
> 
> The fact that released tiles end up in tile manager is something that tilings
> aren't aware of. I think it's a bit of a can of worms, since it's not crystal
> clear to me how we clear priorities on those tiles even if they don't end up
in
> the tile manager (ie, they are still alive on the twin tiling). I _think_ it's
> DidBecomeRecycled or DidBecomeActive calls on the twin that end up clearing
> these priorities.
> 
> I mean, in the end I would just still like to reiterate that I think the fact
> that tile manager requires released tiles to have default priority so that it
> doesn't schedule the tile again should be the responsibility of the tile
> manager. I would hate to see a comment like "Clear priorities so tile manager
> doesn't schedule this tile I'm about to delete" in the tiling. 

Yes, that would be the wrong reason to reset the priority. Does the tiling code
somehow know that all references to the tile are dropped? If it's just dropping
some reference to the tile but has no idea if more are alive then resetting the
priority makes sense IMO. I think it would be wrong to assume that the tile
priority would implicitly be reset just because a reference to the tile was
dropped.

I think a comment like this would be perfectly valid:
"Reset priority as tile is ref-counted and might still be used even though we no
longer hold a reference to it here anymore."

[sohanjg, 2014-07-23 13:05:41]

Looks like we need to destroy the completed raster tasks after scheduling, i
have re-used the orphan_raster_tasks_ vector.
This resolves the crash and local unit tests issues.
Please take a look, thanks.

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:409: DCHECK(TilePriority() ==
tile->combined_priority());
On 2014/07/22 18:02:47, reveman wrote:
> On 2014/07/22 17:23:34, vmpstr wrote:
> > On 2014/07/22 16:51:08, reveman wrote:
> > > On 2014/07/22 16:31:02, vmpstr wrote:
> > > > On 2014/07/22 16:21:21, reveman wrote:
> > > > > On 2014/07/22 15:17:29, vmpstr wrote:
> > > > > > On 2014/07/22 14:17:46, sohanjg wrote:
> > > > > > > On 2014/07/21 15:24:14, vmpstr wrote:
> > > > > > > > Why does this work? I don't think we have code anywhere that
would
> > > reset
> > > > > the
> > > > > > > > priority. That is, if we invalidate a NOW tile, it will still
have
> > NOW
> > > > > > > priority
> > > > > > > > here. If I'm missing something, please add a comment.
> > > > > > > 
> > > > > > > Ideally, we should do SetPriority(TREE, TilePriority()) for active
> and
> > > > > > pending,
> > > > > > > but we assume it is already the case, and dcheck if its not ?
should
> > we
> > > > > change
> > > > > > > this ?
> > > > > > 
> > > > > > That's my question... I don't think it's the case right now. Tiles
are
> > > > > released
> > > > > > by the tiling during invalidation, among other paths, and I'm pretty
> > sure
> > > we
> > > > > > don't touch the priority when we do that. I think this DCHECK has to
> be
> > > > > > SetPriority(ACTIVE_TREE, TilePriority()); SetPriority(PENDING_TREE,
> > > > > > TilePriority()) instead with a comment saying that this is so that
the
> > > tile
> > > > is
> > > > > > not scheduled again.
> > > > > 
> > > > > Please give some example usage that justifies having the ability to
drop
> > > tile
> > > > > references but not reset the priority. I'm fine resetting it here if
> > needed,
> > > I
> > > > > just prefer to have a concrete example of why it's needed rather than
> > assume
> > > > it
> > > > > is.
> > > > 
> > > > Suppose we have some tiles on the active tree with all priorities up to
> > date,
> > > > then the live tiles rect moves down and we release a top row of tiles
> (just
> > > > active tree in this case). This just removes references from the tiling
> > > without
> > > > setting priorities to be empty. It's not really a justification of why
we
> > > don't
> > > > do it, it's just a fact that we don't (at least IIRC). In general, when
> > > tilings
> > > > remove tiles, priorities aren't affected. From there, it's implicit that
> > > > whatever is released is not needed. 
> > > > 
> > > > I think invalidation case works, because we have
> > > > DidBecomeRecycled/DidBecomeActive things that play around with
priorities.
> > > > However, on a long enough page, I think we will hit the dcheck once the
> live
> > > > tiles rect starts culling tiles. 
> > > 
> > > So I guess my next question is; does it make more sense to reset the
> priority
> > > when removing references from a tiling than doing it here? When the live
> tile
> > > rect moves and we decide to drop the top row of tiles it seems like it's a
> > > change in priority of those tiles that causes us to do this and it might
be
> > > appropriate to have the actual tile priority reflect that. Also sounds
like
> > > doing it here would often cause us to reset it when not needed.
> > 
> > I'm not sure I like the approach of clearing the priority when we drop tiles
> in
> > the tiling. From the tiling's perspective, the tile is gone. It's kind of
akin
> > to clearing some variables before calling delete on an object: it's a bit
> > awkward and not obvious why it's required.
> > 
> > The fact that released tiles end up in tile manager is something that
tilings
> > aren't aware of. I think it's a bit of a can of worms, since it's not
crystal
> > clear to me how we clear priorities on those tiles even if they don't end up
> in
> > the tile manager (ie, they are still alive on the twin tiling). I _think_
it's
> > DidBecomeRecycled or DidBecomeActive calls on the twin that end up clearing
> > these priorities.
> > 
> > I mean, in the end I would just still like to reiterate that I think the
fact
> > that tile manager requires released tiles to have default priority so that
it
> > doesn't schedule the tile again should be the responsibility of the tile
> > manager. I would hate to see a comment like "Clear priorities so tile
manager
> > doesn't schedule this tile I'm about to delete" in the tiling. 
> 
> Yes, that would be the wrong reason to reset the priority. Does the tiling
code
> somehow know that all references to the tile are dropped? If it's just
dropping
> some reference to the tile but has no idea if more are alive then resetting
the
> priority makes sense IMO. I think it would be wrong to assume that the tile
> priority would implicitly be reset just because a reference to the tile was
> dropped.
> 
> I think a comment like this would be perfectly valid:
> "Reset priority as tile is ref-counted and might still be used even though we
no
> longer hold a reference to it here anymore."

Done.
Have reset the priority and removed the dcheck.

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: DCHECK(prioritized_tiles_dirty_);
On 2014/07/22 16:51:08, reveman wrote:
> On 2014/07/22 16:31:02, vmpstr wrote:
> > On 2014/07/22 16:21:21, reveman wrote:
> > > On 2014/07/22 15:17:29, vmpstr wrote:
> > > > On 2014/07/22 14:17:46, sohanjg wrote:
> > > > > On 2014/07/21 15:24:14, vmpstr wrote:
> > > > > > This is kind of weird. Why are we requiring this to be true? (please
> add
> > a
> > > > > small
> > > > > > comment explaining)
> > > > > 
> > > > > hmm..we had earlier seen some crashes when we cleaned up released
tiles
> > from
> > > > > DidFinishRunningTasks, because maybe prioritized tile set is affected
> when
> > > we
> > > > > modify the tiles vector.
> > > > > so we had put this extra protective code here.
> > > > 
> > > > Ah ok. This still makes it a bit fragile. 
> > > > 
> > > > The problem is that prioritized set is a view on tiles_ basically. It
has
> no
> > > > ownership, it just returns pointers in a certain order. If we remove
> > something
> > > > from tiles_ we have to ensure that the next thing we do is rebuild the
> > > > prioritized tile set, which I guess explains that DCHECK. The fragile
part
> > is
> > > > that prioritized_tiles_dirty_ is set elsewhere, so there is no strict
> > > guarantee
> > > > that we will rebuild the set (although it seems to be the case now). I
> guess
> > > > that's OK. Maybe add a comment in .h above this function saying that we
> have
> > > to
> > > > rebuild the set before accessing it?
> > > 
> > > After, right? The idea with this DCHECK is that we need to make sure that
> the
> > > prioritized tile set is rebuild after calling this as we may otherwise use
a
> > set
> > > with deleted tiles. A comment explaining this would be nice.
> > > 
> > > If this DCHECK fails, it's probably because of the call to this function
in
> > the
> > > dtor as the call in ::UpdatePrioritizedTileSetIfNeeded() can of course
enver
> > > cause it to fail.
> > 
> > Yeah, after. My only worry is that we set prioritized_tiles_dirty_ to true
in
> > DidChangeTilePriority for example, which allows this function to be called
> with
> > no promise of rebuilding the set. (DidFinishRunningTasks for instance
doesn't
> > rebuild the set, even if this flag is set). 
> 
> Yea, the DCHECK is not perfect. We could just move all this code to
> UpdatePrioritizedTileSetIfNeeded() instead. Just a bit ugly to have to call
that
> from the dtor..

Done.
Have kept the dcheck for now, and added a description and todo.

[sohanjg, 2014-07-23 14:16:32]

Had to avoid the dcheck on prioritized tile set for build issue, please take a
look.
Thanks.

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

https://codereview.chromium.org/366113002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: DCHECK(prioritized_tiles_dirty_);
On 2014/07/23 13:05:41, sohanjg wrote:
> On 2014/07/22 16:51:08, reveman wrote:
> > On 2014/07/22 16:31:02, vmpstr wrote:
> > > On 2014/07/22 16:21:21, reveman wrote:
> > > > On 2014/07/22 15:17:29, vmpstr wrote:
> > > > > On 2014/07/22 14:17:46, sohanjg wrote:
> > > > > > On 2014/07/21 15:24:14, vmpstr wrote:
> > > > > > > This is kind of weird. Why are we requiring this to be true?
(please
> > add
> > > a
> > > > > > small
> > > > > > > comment explaining)
> > > > > > 
> > > > > > hmm..we had earlier seen some crashes when we cleaned up released
> tiles
> > > from
> > > > > > DidFinishRunningTasks, because maybe prioritized tile set is
affected
> > when
> > > > we
> > > > > > modify the tiles vector.
> > > > > > so we had put this extra protective code here.
> > > > > 
> > > > > Ah ok. This still makes it a bit fragile. 
> > > > > 
> > > > > The problem is that prioritized set is a view on tiles_ basically. It
> has
> > no
> > > > > ownership, it just returns pointers in a certain order. If we remove
> > > something
> > > > > from tiles_ we have to ensure that the next thing we do is rebuild the
> > > > > prioritized tile set, which I guess explains that DCHECK. The fragile
> part
> > > is
> > > > > that prioritized_tiles_dirty_ is set elsewhere, so there is no strict
> > > > guarantee
> > > > > that we will rebuild the set (although it seems to be the case now). I
> > guess
> > > > > that's OK. Maybe add a comment in .h above this function saying that
we
> > have
> > > > to
> > > > > rebuild the set before accessing it?
> > > > 
> > > > After, right? The idea with this DCHECK is that we need to make sure
that
> > the
> > > > prioritized tile set is rebuild after calling this as we may otherwise
use
> a
> > > set
> > > > with deleted tiles. A comment explaining this would be nice.
> > > > 
> > > > If this DCHECK fails, it's probably because of the call to this function
> in
> > > the
> > > > dtor as the call in ::UpdatePrioritizedTileSetIfNeeded() can of course
> enver
> > > > cause it to fail.
> > > 
> > > Yeah, after. My only worry is that we set prioritized_tiles_dirty_ to true
> in
> > > DidChangeTilePriority for example, which allows this function to be called
> > with
> > > no promise of rebuilding the set. (DidFinishRunningTasks for instance
> doesn't
> > > rebuild the set, even if this flag is set). 
> > 
> > Yea, the DCHECK is not perfect. We could just move all this code to
> > UpdatePrioritizedTileSetIfNeeded() instead. Just a bit ugly to have to call
> that
> > from the dtor..
> 
> Done.
> Have kept the dcheck for now, and added a description and todo.

Avoided the dcheck as it breaks tests, added todo to move it to
UpdatePrioritizedTileSetIfNeeded, where rebuild set is ensured.

[vmpstr, 2014-07-23 16:10:37]

Aside from the comment below, this lgtm. Please wait for reveman to do another
review though.

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

https://codereview.chromium.org/366113002/diff/420001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:411: // even though we no longer hold a reference
to it here anymore.
This comment was meant to be for the picture layer tiling, I think. At this
point it's actually not correct. TileManager::Release means that the tile is
_not_ used anywhere anymore. That being said, I don't mind resetting it here (up
to reveman)

[reveman, 2014-07-23 16:26:54]

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

https://codereview.chromium.org/366113002/diff/380001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:439: DCHECK(prioritized_tiles_dirty_);
Why did you remove this DCHECK? What was the reason for it failing and why did
you decide that removing it was the best solution?

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

https://codereview.chromium.org/366113002/diff/420001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:411: // even though we no longer hold a reference
to it here anymore.
On 2014/07/23 16:10:37, vmpstr wrote:
> This comment was meant to be for the picture layer tiling, I think. At this
> point it's actually not correct. TileManager::Release means that the tile is
> _not_ used anywhere anymore. That being said, I don't mind resetting it here
(up
> to reveman)

Yea, this doesn't make sense here.

https://codereview.chromium.org/366113002/diff/420001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:413: tile->SetPriority(PENDING_TREE,
TilePriority());
Please try to reset these when dropping the ref from the picture layer tiling
instead. And keep the DCHECK you had here before. The above comment makes sense
in that context. I'd like to see what that looks like before we resort to
resetting the priority here.

[sohanjg, 2014-07-23 16:59:51]

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

https://codereview.chromium.org/366113002/diff/380001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:439: DCHECK(prioritized_tiles_dirty_);
On 2014/07/23 16:26:54, reveman wrote:
> Why did you remove this DCHECK? What was the reason for it failing and why did
> you decide that removing it was the best solution?

Well. the tests are not failing in my local debug cc_unittests so, based on the
bot logs, i made the changes.
I will try and verify it locally and see if there can be some other solution,
maybe the changes in faketilemanager can fix it (i made these before), i will
check more.

[reveman, 2014-07-23 17:08:49]

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

https://codereview.chromium.org/366113002/diff/380001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:439: DCHECK(prioritized_tiles_dirty_);
On 2014/07/23 16:59:51, sohanjg wrote:
> On 2014/07/23 16:26:54, reveman wrote:
> > Why did you remove this DCHECK? What was the reason for it failing and why
did
> > you decide that removing it was the best solution?
> 
> Well. the tests are not failing in my local debug cc_unittests so, based on
the
> bot logs, i made the changes.
> I will try and verify it locally and see if there can be some other solution,
> maybe the changes in faketilemanager can fix it (i made these before), i will
> check more.

The problem is the dtor where you might call this without
prioritized_tiles_dirty_ being set. I think it would be fine to just remove that
call from there and not expect tiles_.size() == 0.

[sohanjg, 2014-07-24 11:08:41]

Moved resetting the priority to picture layer tiling set, while releasing tiles,
in SetLiveTilesRect, DoInvalidate, and its Dtor.
Please take a look, thanks.

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

https://codereview.chromium.org/366113002/diff/420001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:411: // even though we no longer hold a reference
to it here anymore.
On 2014/07/23 16:26:54, reveman wrote:
> On 2014/07/23 16:10:37, vmpstr wrote:
> > This comment was meant to be for the picture layer tiling, I think. At this
> > point it's actually not correct. TileManager::Release means that the tile is
> > _not_ used anywhere anymore. That being said, I don't mind resetting it here
> (up
> > to reveman)
> 
> Yea, this doesn't make sense here.

Done.

https://codereview.chromium.org/366113002/diff/420001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:413: tile->SetPriority(PENDING_TREE,
TilePriority());
On 2014/07/23 16:26:54, reveman wrote:
> Please try to reset these when dropping the ref from the picture layer tiling
> instead. And keep the DCHECK you had here before. The above comment makes
sense
> in that context. I'd like to see what that looks like before we resort to
> resetting the priority here.

Done.

[reveman, 2014-07-24 13:36:24]

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:100: tile->SetPriority(PENDING_TREE,
TilePriority());
I don't think you can reset both pending and active priority here. What about
the twin tiling that might be alive and using one of these?

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:227: tile->SetPriority(PENDING_TREE,
TilePriority());
same here. resetting both active and pending priority seems wrong.

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:616: tile->SetPriority(PENDING_TREE,
TilePriority());
and here. resetting both active and pending priority seems wrong.

https://codereview.chromium.org/366113002/diff/440001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/366113002/diff/440001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:392: CleanUpReleasedTiles();
We don't have to do all the work that cleanup released tiles does but we still
need to make sure that released tiles are actually deleted as that won't happen
automatically. one way to handle that would be to just store them in a
ScopedVector instead.

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

https://codereview.chromium.org/366113002/diff/440001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: // the prioritized set is rebuild.
Not sure why you have this TODO here. Does it refer to the DCHECK? I'm not sure
we should move this to UpdatePrioritizedTileSetIfNeeded, it might make that
function harder to read. Please comment on the DCHECK instead. It might be
easier to understand why we have this check if it's
DCHECK(prioritized_tiles_.empty()) and you adjust
UpdatePrioritizedTileSetIfNeeded() so that's the case. Your call.

[sohanjg, 2014-07-24 15:01:55]

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:100: tile->SetPriority(PENDING_TREE,
TilePriority());
On 2014/07/24 13:36:24, reveman wrote:
> I don't think you can reset both pending and active priority here. What about
> the twin tiling that might be alive and using one of these?

If we reset the priority of only the pending tree here,
how do we reset for the active tree before we drop the tile ?
Vlad , enne ?

[vmpstr, 2014-07-24 16:19:22]

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:100: tile->SetPriority(PENDING_TREE,
TilePriority());
On 2014/07/24 15:01:55, sohanjg wrote:
> On 2014/07/24 13:36:24, reveman wrote:
> > I don't think you can reset both pending and active priority here. What
about
> > the twin tiling that might be alive and using one of these?
> 
> If we reset the priority of only the pending tree here,
> how do we reset for the active tree before we drop the tile ?
> Vlad , enne ?

Since at this point the tile can be shared on both trees, you should only reset
the priority of the tree that it's on. You might have to add WhichTree
PictureLayerTilingClient::GetTree(); and then do 

tile->SetPriority(client_->GetTree(), TilePriority());

If the tile is released into tile manager that means one of two things: 1. It
wasn't shared, in which case the other priority wasn't ever set or 2. both trees
released it, which means you would get two of these tilings releasing the same
tile with a different client_->GetTree value.

(I mean this is the reason that I was OK with clearing it in tile manager: it's
just a final stop for the tile... here works as well, but it's a bit more work).

[sohanjg, 2014-07-25 09:38:26]

Can you please take a look, hopefully the last iteration :)
Thanks.

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:100: tile->SetPriority(PENDING_TREE,
TilePriority());
On 2014/07/24 16:19:21, vmpstr wrote:
> On 2014/07/24 15:01:55, sohanjg wrote:
> > On 2014/07/24 13:36:24, reveman wrote:
> > > I don't think you can reset both pending and active priority here. What
> about
> > > the twin tiling that might be alive and using one of these?
> > 
> > If we reset the priority of only the pending tree here,
> > how do we reset for the active tree before we drop the tile ?
> > Vlad , enne ?
> 
> Since at this point the tile can be shared on both trees, you should only
reset
> the priority of the tree that it's on. You might have to add WhichTree
> PictureLayerTilingClient::GetTree(); and then do 
> 
> tile->SetPriority(client_->GetTree(), TilePriority());
> 
> If the tile is released into tile manager that means one of two things: 1. It
> wasn't shared, in which case the other priority wasn't ever set or 2. both
trees
> released it, which means you would get two of these tilings releasing the same
> tile with a different client_->GetTree value.
> 
> (I mean this is the reason that I was OK with clearing it in tile manager:
it's
> just a final stop for the tile... here works as well, but it's a bit more
work).

Done.

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:227: tile->SetPriority(PENDING_TREE,
TilePriority());
On 2014/07/24 13:36:24, reveman wrote:
> same here. resetting both active and pending priority seems wrong.

Done.

https://codereview.chromium.org/366113002/diff/440001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:616: tile->SetPriority(PENDING_TREE,
TilePriority());
On 2014/07/24 13:36:24, reveman wrote:
> and here. resetting both active and pending priority seems wrong.

Done.

https://codereview.chromium.org/366113002/diff/440001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/366113002/diff/440001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:392: CleanUpReleasedTiles();
On 2014/07/24 13:36:24, reveman wrote:
> We don't have to do all the work that cleanup released tiles does but we still
> need to make sure that released tiles are actually deleted as that won't
happen
> automatically. one way to handle that would be to just store them in a
> ScopedVector instead.

Done.
yea..missed that, have cleared the released tiles for now, and have marked a
todo to use scopedvector, hope its ok.

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

https://codereview.chromium.org/366113002/diff/440001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:432: // the prioritized set is rebuild.
On 2014/07/24 13:36:24, reveman wrote:
> Not sure why you have this TODO here. Does it refer to the DCHECK? I'm not
sure
> we should move this to UpdatePrioritizedTileSetIfNeeded, it might make that
> function harder to read. Please comment on the DCHECK instead. It might be
> easier to understand why we have this check if it's
> DCHECK(prioritized_tiles_.empty()) and you adjust
> UpdatePrioritizedTileSetIfNeeded() so that's the case. Your call.

PrioritizedTileSet doesnt have an .empty operator yet, so kept the old dcheck,
maybe a follow up?

[vmpstr, 2014-07-25 16:29:25]

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

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:402: released_tiles_.clear();
You should call CleanUp... here instead and dcheck that release_tiles_.empty().
Reason being is that released tiles are raw pointers and would leak otherwise.

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
cc/resources/tile_manager.h:331: // TODO(sohanjg):  Use ScopedVector.
Scoped vector would keep the tiles alive (thus preventing a Release call)

[vmpstr, 2014-07-25 16:43:43]

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
cc/resources/tile_manager.h:331: // TODO(sohanjg):  Use ScopedVector.
On 2014/07/25 16:29:25, vmpstr wrote:
> Scoped vector would keep the tiles alive (thus preventing a Release call)

Ignore this comment, I thought this was for prioritized tile set for some
reason.

[reveman, 2014-07-25 17:00:55]

https://codereview.chromium.org/366113002/diff/460001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/460001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:222:
find->second.get()->SetPriority(client_->GetTree(), TilePriority());
This comment is relevant not just here but in all cases where you drop a tile
reference. Maybe you can add a "void ReleaseTile(Tile* tile, WhichTree tree)"
utility function to the anonymous namespace that will reset the priority and
contain this comment.

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

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:402: released_tiles_.clear();
On 2014/07/25 16:29:25, vmpstr wrote:
> You should call CleanUp... here instead and dcheck that
release_tiles_.empty().
> Reason being is that released tiles are raw pointers and would leak otherwise.

The problem is that we can't have the DCHECK(prioritized_tiles_dirty_) check in
::CleanUpReleasedTiles() without awkwardly setting prioritized_tiles_dirty_ to
true here before the call.

released_tiles_.clear() is obviously wrong as it will leak the pointers but if
released_tiles_ was a ScopedVector instead I think all would be good as we don't
actually have to do all the work CleanUpReleasedTiles() does here. I think
making released_tiles_ a ScopedVector is good idea in general.

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
cc/resources/tile_manager.h:331: // TODO(sohanjg):  Use ScopedVector.
This can't be a TODO. You're introducing a memory leak with this patch unless
you change this.

[sohanjg, 2014-07-26 09:36:01]

Please take a look, thanks.

https://codereview.chromium.org/366113002/diff/460001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/460001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:222:
find->second.get()->SetPriority(client_->GetTree(), TilePriority());
On 2014/07/25 17:00:54, reveman wrote:
> This comment is relevant not just here but in all cases where you drop a tile
> reference. Maybe you can add a "void ReleaseTile(Tile* tile, WhichTree tree)"
> utility function to the anonymous namespace that will reset the priority and
> contain this comment.

I have added the comment in all the places for now, will move to a new func as a
follow up for sure.

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

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:402: released_tiles_.clear();
On 2014/07/25 17:00:55, reveman wrote:
> On 2014/07/25 16:29:25, vmpstr wrote:
> > You should call CleanUp... here instead and dcheck that
> release_tiles_.empty().
> > Reason being is that released tiles are raw pointers and would leak
otherwise.
> 
> The problem is that we can't have the DCHECK(prioritized_tiles_dirty_) check
in
> ::CleanUpReleasedTiles() without awkwardly setting prioritized_tiles_dirty_ to
> true here before the call.
> 
> released_tiles_.clear() is obviously wrong as it will leak the pointers but if
> released_tiles_ was a ScopedVector instead I think all would be good as we
don't
> actually have to do all the work CleanUpReleasedTiles() does here. I think
> making released_tiles_ a ScopedVector is good idea in general.

Done.

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/366113002/diff/460001/cc/resources/tile_manag...
cc/resources/tile_manager.h:331: // TODO(sohanjg):  Use ScopedVector.
On 2014/07/25 17:00:55, reveman wrote:
> This can't be a TODO. You're introducing a memory leak with this patch unless
> you change this.

Done.
yea.right!

[reveman, 2014-07-28 02:24:36]

https://codereview.chromium.org/366113002/diff/460001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/460001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:222:
find->second.get()->SetPriority(client_->GetTree(), TilePriority());
On 2014/07/26 09:36:01, sohanjg wrote:
> On 2014/07/25 17:00:54, reveman wrote:
> > This comment is relevant not just here but in all cases where you drop a
tile
> > reference. Maybe you can add a "void ReleaseTile(Tile* tile, WhichTree
tree)"
> > utility function to the anonymous namespace that will reset the priority and
> > contain this comment.
> 
> I have added the comment in all the places for now, will move to a new func as
a
> follow up for sure.

I don't think that makes sense as a follow up. Please move it to a function in
this patch rather than duplicating code.

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
can you make ScopedVector<Tile> a friend class instead?

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

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:433: DCHECK(prioritized_tiles_dirty_);
Let's change this to DCHECK(prioritized_tiles_.Empty()) to be safe. I think
that's also easier to understand. And the comment can say "Make sure
|prioritized_tiles_| doesn't contain any of the tiles we're about to delete."

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:457: it = released_tiles_.weak_erase(it);
remove "delete tile" and use standard erase instead

https://codereview.chromium.org/366113002/diff/470001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer_tiling_client.cc (right):

https://codereview.chromium.org/366113002/diff/470001/cc/test/fake_picture_la...
cc/test/fake_picture_layer_tiling_client.cc:83: // TODO(sohanjg): Find correct
tree for test.
I don't think this should be a todo. What needs to be done to implement this
properly?

https://codereview.chromium.org/366113002/diff/470001/cc/test/fake_tile_manag...
File cc/test/fake_tile_manager.cc (right):

https://codereview.chromium.org/366113002/diff/470001/cc/test/fake_tile_manag...
cc/test/fake_tile_manager.cc:105: tile->SetPriority(PENDING_TREE,
TilePriority());
please remove these and figure out how to reset priority properly for tests.

[sohanjg, 2014-07-28 10:10:19]

Please have a look, and let me know your opinion.
And thank you for your patience on this :)

https://codereview.chromium.org/366113002/diff/460001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/460001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:222:
find->second.get()->SetPriority(client_->GetTree(), TilePriority());
On 2014/07/28 02:24:35, reveman wrote:
> On 2014/07/26 09:36:01, sohanjg wrote:
> > On 2014/07/25 17:00:54, reveman wrote:
> > > This comment is relevant not just here but in all cases where you drop a
> tile
> > > reference. Maybe you can add a "void ReleaseTile(Tile* tile, WhichTree
> tree)"
> > > utility function to the anonymous namespace that will reset the priority
and
> > > contain this comment.
> > 
> > I have added the comment in all the places for now, will move to a new func
as
> a
> > follow up for sure.
> 
> I don't think that makes sense as a follow up. Please move it to a function in
> this patch rather than duplicating code.

Done.

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/28 02:24:35, reveman wrote:
> can you make ScopedVector<Tile> a friend class instead?

In ScopedVector, we would do the actual ptr deletion from
STLDeleteContainerPointers(), via STLDeleteElements(), so making only 
ScopedVector<Tile> friend will not be sufficient.

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

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:433: DCHECK(prioritized_tiles_dirty_);
On 2014/07/28 02:24:35, reveman wrote:
> Let's change this to DCHECK(prioritized_tiles_.Empty()) to be safe. I think
> that's also easier to understand. And the comment can say "Make sure
> |prioritized_tiles_| doesn't contain any of the tiles we're about to delete."

Are you sure we want this, because if we check for empty - if
(!tiles_[bin].empty()) return false..
there are test failures.
Wont UpdatePrioritizedTileSetIfNeeded, clear it right after we do
CleanUpReleasedTiles ? why do we need the check ? now that we have avoided the
path from dtor.

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:457: it = released_tiles_.weak_erase(it);
On 2014/07/28 02:24:35, reveman wrote:
> remove "delete tile" and use standard erase instead

Done.

https://codereview.chromium.org/366113002/diff/470001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer_tiling_client.cc (right):

https://codereview.chromium.org/366113002/diff/470001/cc/test/fake_picture_la...
cc/test/fake_picture_layer_tiling_client.cc:83: // TODO(sohanjg): Find correct
tree for test.
On 2014/07/28 02:24:35, reveman wrote:
> I don't think this should be a todo. What needs to be done to implement this
> properly?

For getting correct tree we need to set which tree during tests explicitly,
which will involve changing and verifying all tests using
FakePictureLayerTilingClient i.e PictureLayerTiling and PictureLayerTilingSet
tests.
So i thought of doing it as a seperate CL, right after this gets landed.

https://codereview.chromium.org/366113002/diff/470001/cc/test/fake_tile_manag...
File cc/test/fake_tile_manager.cc (right):

https://codereview.chromium.org/366113002/diff/470001/cc/test/fake_tile_manag...
cc/test/fake_tile_manager.cc:105: tile->SetPriority(PENDING_TREE,
TilePriority());
On 2014/07/28 02:24:35, reveman wrote:
> please remove these and figure out how to reset priority properly for tests.

As mentioned above, once we implement ::GetTree in FakePictureLayerTilingClient,
we wouldnt need this, but that involves a lot of change, so holding it up for
the next CL.

Alternatively, we may reset priority in TileManager::Release in this CL, and in
the next one move it to PictureLayerTiling along with the test changes.

[danakj, 2014-07-28 10:20:03]

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/28 10:10:19, sohanjg wrote:
> On 2014/07/28 02:24:35, reveman wrote:
> > can you make ScopedVector<Tile> a friend class instead?
> 
> In ScopedVector, we would do the actual ptr deletion from
> STLDeleteContainerPointers(), via STLDeleteElements(), so making only 
> ScopedVector<Tile> friend will not be sufficient.

If u do want to use scopedvector can u use ScopedPtrVector instead?

[sohanjg, 2014-07-28 11:34:50]

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote:
> On 2014/07/28 10:10:19, sohanjg wrote:
> > On 2014/07/28 02:24:35, reveman wrote:
> > > can you make ScopedVector<Tile> a friend class instead?
> > 
> > In ScopedVector, we would do the actual ptr deletion from
> > STLDeleteContainerPointers(), via STLDeleteElements(), so making only 
> > ScopedVector<Tile> friend will not be sufficient.
> 
> If u do want to use scopedvector can u use ScopedPtrVector instead?

Hmm..but tiles here are ref counted (RefCountedManaged), but ScopedPtrVector
works with scoped ptrs (it asserts for ref counted types).

[danakj, 2014-07-28 12:29:28]

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/28 11:34:50, sohanjg wrote:
> On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote:
> > On 2014/07/28 10:10:19, sohanjg wrote:
> > > On 2014/07/28 02:24:35, reveman wrote:
> > > > can you make ScopedVector<Tile> a friend class instead?
> > > 
> > > In ScopedVector, we would do the actual ptr deletion from
> > > STLDeleteContainerPointers(), via STLDeleteElements(), so making only 
> > > ScopedVector<Tile> friend will not be sufficient.
> > 
> > If u do want to use scopedvector can u use ScopedPtrVector instead?
> 
> Hmm..but tiles here are ref counted (RefCountedManaged), but ScopedPtrVector
> works with scoped ptrs (it asserts for ref counted types).

Er, okay, putting refcounted things in ScopedVector sounds really weird too
then, since it takes "ownership" of them and would delete them on destruction,
not unref them, unless I'm missing something? 

Generally refptrs and data structures for scoped_ptrs do not mix. We'd use STL
data structures for refptrs, such as using std::vector. So this sounds strange
without knowing more, but I'll leave to reveman to sort this out, maybe it makes
more sense here.

(Note: I would like to one day replace ScopedVector with ScopedPtrVector
essentially, getting rid of raw pointer ownership transfers with scoped_ptrs.)

[vmpstr, 2014-07-28 15:23:14]

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote:
> On 2014/07/28 11:34:50, sohanjg wrote:
> > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote:
> > > On 2014/07/28 10:10:19, sohanjg wrote:
> > > > On 2014/07/28 02:24:35, reveman wrote:
> > > > > can you make ScopedVector<Tile> a friend class instead?
> > > > 
> > > > In ScopedVector, we would do the actual ptr deletion from
> > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making only 
> > > > ScopedVector<Tile> friend will not be sufficient.
> > > 
> > > If u do want to use scopedvector can u use ScopedPtrVector instead?
> > 
> > Hmm..but tiles here are ref counted (RefCountedManaged), but ScopedPtrVector
> > works with scoped ptrs (it asserts for ref counted types).
> 
> Er, okay, putting refcounted things in ScopedVector sounds really weird too
> then, since it takes "ownership" of them and would delete them on destruction,
> not unref them, unless I'm missing something? 
> 
> Generally refptrs and data structures for scoped_ptrs do not mix. We'd use STL
> data structures for refptrs, such as using std::vector. So this sounds strange
> without knowing more, but I'll leave to reveman to sort this out, maybe it
makes
> more sense here.
> 
> (Note: I would like to one day replace ScopedVector with ScopedPtrVector
> essentially, getting rid of raw pointer ownership transfers with scoped_ptrs.)

Tile is weird this way. It's ref counted managed, which means that it acts like
a ref counted object (when it lives on tilings). However, when tilings release
it (ie ref count goes to 0), it isn't deleted, it goes to TileManager::Release
as a raw pointer. It's up to TileManager to do whatever it needs to do. Right
now, it extends its life if it has a raster task until the task finishes.
Currently, we just have a vector of raw pointers that we have to make sure to
delete, but having a scoped (ptr) vector would allow us to take care of that
automatically.

[danakj, 2014-07-28 15:27:13]

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/28 15:23:14, vmpstr wrote:
> On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote:
> > On 2014/07/28 11:34:50, sohanjg wrote:
> > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote:
> > > > On 2014/07/28 10:10:19, sohanjg wrote:
> > > > > On 2014/07/28 02:24:35, reveman wrote:
> > > > > > can you make ScopedVector<Tile> a friend class instead?
> > > > > 
> > > > > In ScopedVector, we would do the actual ptr deletion from
> > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making only 
> > > > > ScopedVector<Tile> friend will not be sufficient.
> > > > 
> > > > If u do want to use scopedvector can u use ScopedPtrVector instead?
> > > 
> > > Hmm..but tiles here are ref counted (RefCountedManaged), but
ScopedPtrVector
> > > works with scoped ptrs (it asserts for ref counted types).
> > 
> > Er, okay, putting refcounted things in ScopedVector sounds really weird too
> > then, since it takes "ownership" of them and would delete them on
destruction,
> > not unref them, unless I'm missing something? 
> > 
> > Generally refptrs and data structures for scoped_ptrs do not mix. We'd use
STL
> > data structures for refptrs, such as using std::vector. So this sounds
strange
> > without knowing more, but I'll leave to reveman to sort this out, maybe it
> makes
> > more sense here.
> > 
> > (Note: I would like to one day replace ScopedVector with ScopedPtrVector
> > essentially, getting rid of raw pointer ownership transfers with
scoped_ptrs.)
> 
> Tile is weird this way. It's ref counted managed, which means that it acts
like
> a ref counted object (when it lives on tilings). However, when tilings release
> it (ie ref count goes to 0), it isn't deleted, it goes to TileManager::Release
> as a raw pointer. It's up to TileManager to do whatever it needs to do. Right
> now, it extends its life if it has a raster task until the task finishes.
> Currently, we just have a vector of raw pointers that we have to make sure to
> delete, but having a scoped (ptr) vector would allow us to take care of that
> automatically.

Oh okay, that's super funky, thanks for the explanation. How come it doesn't put
the raw pointer into a vector<scoped_refptr<Tile>> again there, which would take
a ref on it and then delete it automatically? Or if that's not possible, we
could wrap that raw pointer in a scoped_ptr'd object that deletes the contained
raw pointer, and stick that into a ScopedPtrVector. Or we could just use a
ScopedVector but my allergies to raw pointers kicked in.

[reveman, 2014-07-28 15:33:07]

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/28 15:27:13, danakj_OOO_until_Aug_3 wrote:
> On 2014/07/28 15:23:14, vmpstr wrote:
> > On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote:
> > > On 2014/07/28 11:34:50, sohanjg wrote:
> > > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote:
> > > > > On 2014/07/28 10:10:19, sohanjg wrote:
> > > > > > On 2014/07/28 02:24:35, reveman wrote:
> > > > > > > can you make ScopedVector<Tile> a friend class instead?
> > > > > > 
> > > > > > In ScopedVector, we would do the actual ptr deletion from
> > > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making
only 
> > > > > > ScopedVector<Tile> friend will not be sufficient.
> > > > > 
> > > > > If u do want to use scopedvector can u use ScopedPtrVector instead?
> > > > 
> > > > Hmm..but tiles here are ref counted (RefCountedManaged), but
> ScopedPtrVector
> > > > works with scoped ptrs (it asserts for ref counted types).
> > > 
> > > Er, okay, putting refcounted things in ScopedVector sounds really weird
too
> > > then, since it takes "ownership" of them and would delete them on
> destruction,
> > > not unref them, unless I'm missing something? 
> > > 
> > > Generally refptrs and data structures for scoped_ptrs do not mix. We'd use
> STL
> > > data structures for refptrs, such as using std::vector. So this sounds
> strange
> > > without knowing more, but I'll leave to reveman to sort this out, maybe it
> > makes
> > > more sense here.
> > > 
> > > (Note: I would like to one day replace ScopedVector with ScopedPtrVector
> > > essentially, getting rid of raw pointer ownership transfers with
> scoped_ptrs.)
> > 
> > Tile is weird this way. It's ref counted managed, which means that it acts
> like
> > a ref counted object (when it lives on tilings). However, when tilings
release
> > it (ie ref count goes to 0), it isn't deleted, it goes to
TileManager::Release
> > as a raw pointer. It's up to TileManager to do whatever it needs to do.
Right
> > now, it extends its life if it has a raster task until the task finishes.
> > Currently, we just have a vector of raw pointers that we have to make sure
to
> > delete, but having a scoped (ptr) vector would allow us to take care of that
> > automatically.
> 
> Oh okay, that's super funky, thanks for the explanation. How come it doesn't
put
> the raw pointer into a vector<scoped_refptr<Tile>> again there, which would
take
> a ref on it and then delete it automatically? Or if that's not possible, we
> could wrap that raw pointer in a scoped_ptr'd object that deletes the
contained
> raw pointer, and stick that into a ScopedPtrVector. Or we could just use a
> ScopedVector but my allergies to raw pointers kicked in.

It would be nice if RefCountedManager::Release could instead pass a scoped_ptr
to make it clear that non ref-counted ownership is passed to the manager and we
could use ScopedPtrVector in the manager. Unfortunately, this doesn't work
because of this DCHECK:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

We're not allowed to create a scoped_ptr<T> when T is derived from
base::subtle::RefCountedBase.

ScopedVector is at least better than a std::vector IMO.

[vmpstr, 2014-07-28 15:33:57]

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/28 15:27:13, danakj_OOO_until_Aug_3 wrote:
> On 2014/07/28 15:23:14, vmpstr wrote:
> > On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote:
> > > On 2014/07/28 11:34:50, sohanjg wrote:
> > > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote:
> > > > > On 2014/07/28 10:10:19, sohanjg wrote:
> > > > > > On 2014/07/28 02:24:35, reveman wrote:
> > > > > > > can you make ScopedVector<Tile> a friend class instead?
> > > > > > 
> > > > > > In ScopedVector, we would do the actual ptr deletion from
> > > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making
only 
> > > > > > ScopedVector<Tile> friend will not be sufficient.
> > > > > 
> > > > > If u do want to use scopedvector can u use ScopedPtrVector instead?
> > > > 
> > > > Hmm..but tiles here are ref counted (RefCountedManaged), but
> ScopedPtrVector
> > > > works with scoped ptrs (it asserts for ref counted types).
> > > 
> > > Er, okay, putting refcounted things in ScopedVector sounds really weird
too
> > > then, since it takes "ownership" of them and would delete them on
> destruction,
> > > not unref them, unless I'm missing something? 
> > > 
> > > Generally refptrs and data structures for scoped_ptrs do not mix. We'd use
> STL
> > > data structures for refptrs, such as using std::vector. So this sounds
> strange
> > > without knowing more, but I'll leave to reveman to sort this out, maybe it
> > makes
> > > more sense here.
> > > 
> > > (Note: I would like to one day replace ScopedVector with ScopedPtrVector
> > > essentially, getting rid of raw pointer ownership transfers with
> scoped_ptrs.)
> > 
> > Tile is weird this way. It's ref counted managed, which means that it acts
> like
> > a ref counted object (when it lives on tilings). However, when tilings
release
> > it (ie ref count goes to 0), it isn't deleted, it goes to
TileManager::Release
> > as a raw pointer. It's up to TileManager to do whatever it needs to do.
Right
> > now, it extends its life if it has a raster task until the task finishes.
> > Currently, we just have a vector of raw pointers that we have to make sure
to
> > delete, but having a scoped (ptr) vector would allow us to take care of that
> > automatically.
> 
> Oh okay, that's super funky, thanks for the explanation. How come it doesn't
put
> the raw pointer into a vector<scoped_refptr<Tile>> again there, which would
take
> a ref on it and then delete it automatically? Or if that's not possible, we
> could wrap that raw pointer in a scoped_ptr'd object that deletes the
contained
> raw pointer, and stick that into a ScopedPtrVector. Or we could just use a
> ScopedVector but my allergies to raw pointers kicked in.

We can't really ref it again for a couple of reasons: base would dcheck if you
ref something that went to 0 already, and it would just call Release again when
it's released again. Having a simple wrapper would work, I think. Any other
approach would also work, as long as we're always deleting these tiles. If
ScopedVector is on its way out of the door, then maybe a wrapper is a way to go?

[reveman, 2014-07-28 15:50:53]

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

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:433: DCHECK(prioritized_tiles_dirty_);
On 2014/07/28 10:10:19, sohanjg wrote:
> On 2014/07/28 02:24:35, reveman wrote:
> > Let's change this to DCHECK(prioritized_tiles_.Empty()) to be safe. I think
> > that's also easier to understand. And the comment can say "Make sure
> > |prioritized_tiles_| doesn't contain any of the tiles we're about to
delete."
> 
> Are you sure we want this, because if we check for empty - if
> (!tiles_[bin].empty()) return false..
> there are test failures.
> Wont UpdatePrioritizedTileSetIfNeeded, clear it right after we do
> CleanUpReleasedTiles ? why do we need the check ? now that we have avoided the
> path from dtor.

I want the check to prevent this from being broken in the future. If someone
decides to call this function from somewhere else, things could go very bad.
Please figure out why the tests are failing and change this.

https://codereview.chromium.org/366113002/diff/490001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/490001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:62: // even though we no longer hold a
reference to it here anymore.
Please move this comment just above "tile->SetPriority(..." below

https://codereview.chromium.org/366113002/diff/490001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:63: void ResetPriority(Tile* tile,
WhichTree tree) {
I would prefer if you named this "ReleaseTile" and resetting priority is a
implementation detail of it.

https://codereview.chromium.org/366113002/diff/490001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:64: DCHECK(tile);
Remove this. It's awkward to not be able to assume that |tile| != NULL in a
function that takes a Tile as its main parameter. If you need to DCHECK |tile|,
do it before calling this function instead. Not sure it's needed though.

https://codereview.chromium.org/366113002/diff/490001/cc/resources/prioritize...
File cc/resources/prioritized_tile_set.cc (right):

https://codereview.chromium.org/366113002/diff/490001/cc/resources/prioritize...
cc/resources/prioritized_tile_set.cc:88: }
nit: no need for "{" "}" here

[sohanjg, 2014-07-28 17:36:02]

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

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:433: DCHECK(prioritized_tiles_dirty_);
On 2014/07/28 15:50:53, reveman wrote:
> On 2014/07/28 10:10:19, sohanjg wrote:
> > On 2014/07/28 02:24:35, reveman wrote:
> > > Let's change this to DCHECK(prioritized_tiles_.Empty()) to be safe. I
think
> > > that's also easier to understand. And the comment can say "Make sure
> > > |prioritized_tiles_| doesn't contain any of the tiles we're about to
> delete."
> > 
> > Are you sure we want this, because if we check for empty - if
> > (!tiles_[bin].empty()) return false..
> > there are test failures.
> > Wont UpdatePrioritizedTileSetIfNeeded, clear it right after we do
> > CleanUpReleasedTiles ? why do we need the check ? now that we have avoided
the
> > path from dtor.
> 
> I want the check to prevent this from being broken in the future. If someone
> decides to call this function from somewhere else, things could go very bad.
> Please figure out why the tests are failing and change this.

OK.
The test fails when this is invoked from UpdatePrioritizedTileSetIfNeeded, there
are cases when we have prioritized_tiles_ as non empty when we make this call.
The thing is we do clear prioritized_tiles_ right after this, so do you want us
to move the clearing call before calling CleanUpReleasedTiles ? 
We still would protect this being called from somewhere else as you pointed.

[jamesr, 2014-07-28 17:46:47]

On 2014/07/28 15:23:14, vmpstr wrote:
> https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
> File cc/resources/tile.h (right):
> 
>
https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
> cc/resources/tile.h:24: ~Tile();
> On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote:
> > On 2014/07/28 11:34:50, sohanjg wrote:
> > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote:
> > > > On 2014/07/28 10:10:19, sohanjg wrote:
> > > > > On 2014/07/28 02:24:35, reveman wrote:
> > > > > > can you make ScopedVector<Tile> a friend class instead?
> > > > > 
> > > > > In ScopedVector, we would do the actual ptr deletion from
> > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making only 
> > > > > ScopedVector<Tile> friend will not be sufficient.
> > > > 
> > > > If u do want to use scopedvector can u use ScopedPtrVector instead?
> > > 
> > > Hmm..but tiles here are ref counted (RefCountedManaged), but
ScopedPtrVector
> > > works with scoped ptrs (it asserts for ref counted types).
> > 
> > Er, okay, putting refcounted things in ScopedVector sounds really weird too
> > then, since it takes "ownership" of them and would delete them on
destruction,
> > not unref them, unless I'm missing something? 
> > 
> > Generally refptrs and data structures for scoped_ptrs do not mix. We'd use
STL
> > data structures for refptrs, such as using std::vector. So this sounds
strange
> > without knowing more, but I'll leave to reveman to sort this out, maybe it
> makes
> > more sense here.
> > 
> > (Note: I would like to one day replace ScopedVector with ScopedPtrVector
> > essentially, getting rid of raw pointer ownership transfers with
scoped_ptrs.)
> 
> Tile is weird this way. It's ref counted managed, which means that it acts
like
> a ref counted object (when it lives on tilings). However, when tilings release
> it (ie ref count goes to 0), it isn't deleted, it goes to TileManager::Release
> as a raw pointer. It's up to TileManager to do whatever it needs to do. Right
> now, it extends its life if it has a raster task until the task finishes.
> Currently, we just have a vector of raw pointers that we have to make sure to
> delete, but having a scoped (ptr) vector would allow us to take care of that
> automatically.

That's bad.  If you want to use scoped_refptr<T> but don't want T to be deleted
when the ref count hits zero then don't inherit from RefCountedBase.  Just
implement Add/Release yourself and track the reference count yourself. 
scoped_refptr<T> only requires that T support Add() and Release().

[reveman, 2014-07-28 18:25:34]

On 2014/07/28 17:46:47, jamesr wrote:
> On 2014/07/28 15:23:14, vmpstr wrote:
> > https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
> > File cc/resources/tile.h (right):
> > 
> >
>
https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
> > cc/resources/tile.h:24: ~Tile();
> > On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote:
> > > On 2014/07/28 11:34:50, sohanjg wrote:
> > > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote:
> > > > > On 2014/07/28 10:10:19, sohanjg wrote:
> > > > > > On 2014/07/28 02:24:35, reveman wrote:
> > > > > > > can you make ScopedVector<Tile> a friend class instead?
> > > > > > 
> > > > > > In ScopedVector, we would do the actual ptr deletion from
> > > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making
only 
> > > > > > ScopedVector<Tile> friend will not be sufficient.
> > > > > 
> > > > > If u do want to use scopedvector can u use ScopedPtrVector instead?
> > > > 
> > > > Hmm..but tiles here are ref counted (RefCountedManaged), but
> ScopedPtrVector
> > > > works with scoped ptrs (it asserts for ref counted types).
> > > 
> > > Er, okay, putting refcounted things in ScopedVector sounds really weird
too
> > > then, since it takes "ownership" of them and would delete them on
> destruction,
> > > not unref them, unless I'm missing something? 
> > > 
> > > Generally refptrs and data structures for scoped_ptrs do not mix. We'd use
> STL
> > > data structures for refptrs, such as using std::vector. So this sounds
> strange
> > > without knowing more, but I'll leave to reveman to sort this out, maybe it
> > makes
> > > more sense here.
> > > 
> > > (Note: I would like to one day replace ScopedVector with ScopedPtrVector
> > > essentially, getting rid of raw pointer ownership transfers with
> scoped_ptrs.)
> > 
> > Tile is weird this way. It's ref counted managed, which means that it acts
> like
> > a ref counted object (when it lives on tilings). However, when tilings
release
> > it (ie ref count goes to 0), it isn't deleted, it goes to
TileManager::Release
> > as a raw pointer. It's up to TileManager to do whatever it needs to do.
Right
> > now, it extends its life if it has a raster task until the task finishes.
> > Currently, we just have a vector of raw pointers that we have to make sure
to
> > delete, but having a scoped (ptr) vector would allow us to take care of that
> > automatically.
> 
> That's bad.  If you want to use scoped_refptr<T> but don't want T to be
deleted
> when the ref count hits zero then don't inherit from RefCountedBase.  Just
> implement Add/Release yourself and track the reference count yourself. 
> scoped_refptr<T> only requires that T support Add() and Release().

I like this approach. It would allow RefCountedManager::Release to pass a
scoped_ptr<T> and us to use ScopedPtrVector in the manager.

[reveman, 2014-07-28 18:27:51]

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

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:433: DCHECK(prioritized_tiles_dirty_);
On 2014/07/28 17:36:02, sohanjg wrote:
> On 2014/07/28 15:50:53, reveman wrote:
> > On 2014/07/28 10:10:19, sohanjg wrote:
> > > On 2014/07/28 02:24:35, reveman wrote:
> > > > Let's change this to DCHECK(prioritized_tiles_.Empty()) to be safe. I
> think
> > > > that's also easier to understand. And the comment can say "Make sure
> > > > |prioritized_tiles_| doesn't contain any of the tiles we're about to
> > delete."
> > > 
> > > Are you sure we want this, because if we check for empty - if
> > > (!tiles_[bin].empty()) return false..
> > > there are test failures.
> > > Wont UpdatePrioritizedTileSetIfNeeded, clear it right after we do
> > > CleanUpReleasedTiles ? why do we need the check ? now that we have avoided
> the
> > > path from dtor.
> > 
> > I want the check to prevent this from being broken in the future. If someone
> > decides to call this function from somewhere else, things could go very bad.
> > Please figure out why the tests are failing and change this.
> 
> OK.
> The test fails when this is invoked from UpdatePrioritizedTileSetIfNeeded,
there
> are cases when we have prioritized_tiles_ as non empty when we make this call.
> The thing is we do clear prioritized_tiles_ right after this, so do you want
us
> to move the clearing call before calling CleanUpReleasedTiles ?

yes, it's of course not going to work otherwise.
 
> We still would protect this being called from somewhere else as you pointed.

[sohanjg, 2014-07-30 11:12:57]

Please take a look, thanks a lot for bearing through this :)

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/28 15:33:56, vmpstr wrote:
> On 2014/07/28 15:27:13, danakj_OOO_until_Aug_3 wrote:
> > On 2014/07/28 15:23:14, vmpstr wrote:
> > > On 2014/07/28 12:29:28, danakj_OOO_until_Aug_3 wrote:
> > > > On 2014/07/28 11:34:50, sohanjg wrote:
> > > > > On 2014/07/28 10:20:03, danakj_OOO_until_Aug_3 wrote:
> > > > > > On 2014/07/28 10:10:19, sohanjg wrote:
> > > > > > > On 2014/07/28 02:24:35, reveman wrote:
> > > > > > > > can you make ScopedVector<Tile> a friend class instead?
> > > > > > > 
> > > > > > > In ScopedVector, we would do the actual ptr deletion from
> > > > > > > STLDeleteContainerPointers(), via STLDeleteElements(), so making
> only 
> > > > > > > ScopedVector<Tile> friend will not be sufficient.
> > > > > > 
> > > > > > If u do want to use scopedvector can u use ScopedPtrVector instead?
> > > > > 
> > > > > Hmm..but tiles here are ref counted (RefCountedManaged), but
> > ScopedPtrVector
> > > > > works with scoped ptrs (it asserts for ref counted types).
> > > > 
> > > > Er, okay, putting refcounted things in ScopedVector sounds really weird
> too
> > > > then, since it takes "ownership" of them and would delete them on
> > destruction,
> > > > not unref them, unless I'm missing something? 
> > > > 
> > > > Generally refptrs and data structures for scoped_ptrs do not mix. We'd
use
> > STL
> > > > data structures for refptrs, such as using std::vector. So this sounds
> > strange
> > > > without knowing more, but I'll leave to reveman to sort this out, maybe
it
> > > makes
> > > > more sense here.
> > > > 
> > > > (Note: I would like to one day replace ScopedVector with ScopedPtrVector
> > > > essentially, getting rid of raw pointer ownership transfers with
> > scoped_ptrs.)
> > > 
> > > Tile is weird this way. It's ref counted managed, which means that it acts
> > like
> > > a ref counted object (when it lives on tilings). However, when tilings
> release
> > > it (ie ref count goes to 0), it isn't deleted, it goes to
> TileManager::Release
> > > as a raw pointer. It's up to TileManager to do whatever it needs to do.
> Right
> > > now, it extends its life if it has a raster task until the task finishes.
> > > Currently, we just have a vector of raw pointers that we have to make sure
> to
> > > delete, but having a scoped (ptr) vector would allow us to take care of
that
> > > automatically.
> > 
> > Oh okay, that's super funky, thanks for the explanation. How come it doesn't
> put
> > the raw pointer into a vector<scoped_refptr<Tile>> again there, which would
> take
> > a ref on it and then delete it automatically? Or if that's not possible, we
> > could wrap that raw pointer in a scoped_ptr'd object that deletes the
> contained
> > raw pointer, and stick that into a ScopedPtrVector. Or we could just use a
> > ScopedVector but my allergies to raw pointers kicked in.
> 
> We can't really ref it again for a couple of reasons: base would dcheck if you
> ref something that went to 0 already, and it would just call Release again
when
> it's released again. Having a simple wrapper would work, I think. Any other
> approach would also work, as long as we're always deleting these tiles. If
> ScopedVector is on its way out of the door, then maybe a wrapper is a way to
go?

Lets add the Add/Release and track the reference count and use ScopedPtrVector
in TileManager in a followup CL ? 
this has been iterating for ever now :(
danakj@, jamesr@ , hope that is ok with you.

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

https://codereview.chromium.org/366113002/diff/470001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:433: DCHECK(prioritized_tiles_dirty_);
On 2014/07/28 18:27:51, reveman wrote:
> On 2014/07/28 17:36:02, sohanjg wrote:
> > On 2014/07/28 15:50:53, reveman wrote:
> > > On 2014/07/28 10:10:19, sohanjg wrote:
> > > > On 2014/07/28 02:24:35, reveman wrote:
> > > > > Let's change this to DCHECK(prioritized_tiles_.Empty()) to be safe. I
> > think
> > > > > that's also easier to understand. And the comment can say "Make sure
> > > > > |prioritized_tiles_| doesn't contain any of the tiles we're about to
> > > delete."
> > > > 
> > > > Are you sure we want this, because if we check for empty - if
> > > > (!tiles_[bin].empty()) return false..
> > > > there are test failures.
> > > > Wont UpdatePrioritizedTileSetIfNeeded, clear it right after we do
> > > > CleanUpReleasedTiles ? why do we need the check ? now that we have
avoided
> > the
> > > > path from dtor.
> > > 
> > > I want the check to prevent this from being broken in the future. If
someone
> > > decides to call this function from somewhere else, things could go very
bad.
> > > Please figure out why the tests are failing and change this.
> > 
> > OK.
> > The test fails when this is invoked from UpdatePrioritizedTileSetIfNeeded,
> there
> > are cases when we have prioritized_tiles_ as non empty when we make this
call.
> > The thing is we do clear prioritized_tiles_ right after this, so do you want
> us
> > to move the clearing call before calling CleanUpReleasedTiles ?
> 
> yes, it's of course not going to work otherwise.
>  
> > We still would protect this being called from somewhere else as you pointed.
> 

Done.

https://codereview.chromium.org/366113002/diff/490001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/366113002/diff/490001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:62: // even though we no longer hold a
reference to it here anymore.
On 2014/07/28 15:50:53, reveman wrote:
> Please move this comment just above "tile->SetPriority(..." below

Done.

https://codereview.chromium.org/366113002/diff/490001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:63: void ResetPriority(Tile* tile,
WhichTree tree) {
On 2014/07/28 15:50:53, reveman wrote:
> I would prefer if you named this "ReleaseTile" and resetting priority is a
> implementation detail of it.

Done.

https://codereview.chromium.org/366113002/diff/490001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:64: DCHECK(tile);
On 2014/07/28 15:50:53, reveman wrote:
> Remove this. It's awkward to not be able to assume that |tile| != NULL in a
> function that takes a Tile as its main parameter. If you need to DCHECK
|tile|,
> do it before calling this function instead. Not sure it's needed though.

Acknowledged.

https://codereview.chromium.org/366113002/diff/490001/cc/resources/prioritize...
File cc/resources/prioritized_tile_set.cc (right):

https://codereview.chromium.org/366113002/diff/490001/cc/resources/prioritize...
cc/resources/prioritized_tile_set.cc:88: }
On 2014/07/28 15:50:53, reveman wrote:
> nit: no need for "{" "}" here

Done.

[reveman, 2014-07-30 14:24:01]

https://codereview.chromium.org/366113002/diff/510001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/510001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
This is really inappropriate for a ref counted type. I'm going to try refactor
RefCountedManaged so this is not necessary.

https://codereview.chromium.org/366113002/diff/510001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer_tiling_client.cc (right):

https://codereview.chromium.org/366113002/diff/510001/cc/test/fake_picture_la...
cc/test/fake_picture_layer_tiling_client.cc:83: // TODO(sohanjg): Find correct
tree for test.
I assume that this needs to be fixed before landing this patch. Please explain
why not otherwise.

https://codereview.chromium.org/366113002/diff/510001/cc/test/fake_tile_manag...
File cc/test/fake_tile_manager.cc (right):

https://codereview.chromium.org/366113002/diff/510001/cc/test/fake_tile_manag...
cc/test/fake_tile_manager.cc:105: tile->SetPriority(PENDING_TREE,
TilePriority());
You need to fix this before we can land this patch. Our unit tests should match
the real behavior as close as possible. With this code here they don't.

[sohanjg, 2014-07-30 17:27:04]

Update the unit tests, please take a look, if they are okay?
Thanks.

https://codereview.chromium.org/366113002/diff/510001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/510001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
On 2014/07/30 14:24:01, reveman wrote:
> This is really inappropriate for a ref counted type. I'm going to try refactor
> RefCountedManaged so this is not necessary.

Acknowledged.

https://codereview.chromium.org/366113002/diff/510001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer_tiling_client.cc (right):

https://codereview.chromium.org/366113002/diff/510001/cc/test/fake_picture_la...
cc/test/fake_picture_layer_tiling_client.cc:83: // TODO(sohanjg): Find correct
tree for test.
On 2014/07/30 14:24:01, reveman wrote:
> I assume that this needs to be fixed before landing this patch. Please explain
> why not otherwise.

Done.

https://codereview.chromium.org/366113002/diff/510001/cc/test/fake_tile_manag...
File cc/test/fake_tile_manager.cc (right):

https://codereview.chromium.org/366113002/diff/510001/cc/test/fake_tile_manag...
cc/test/fake_tile_manager.cc:105: tile->SetPriority(PENDING_TREE,
TilePriority());
On 2014/07/30 14:24:01, reveman wrote:
> You need to fix this before we can land this patch. Our unit tests should
match
> the real behavior as close as possible. With this code here they don't.

Done.

[reveman, 2014-07-30 18:41:39]

https://codereview.chromium.org/366113002/diff/550001/cc/resources/prioritize...
File cc/resources/prioritized_tile_set_unittest.cc (right):

https://codereview.chromium.org/366113002/diff/550001/cc/resources/prioritize...
cc/resources/prioritized_tile_set_unittest.cc:80: void
ReleaseTileOnBothTree(std::vector<scoped_refptr<Tile> > tiles) {
nit: don't pass the vector by value. pass a pointer instead. the name of this
function makes it sound like it's only one tile. think it should be "Tiles"
instead of "Tile". i think you can also omit the OnBothTrees suffix.
"ReleaseTiles" would be fine.

https://codereview.chromium.org/366113002/diff/550001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/550001/cc/resources/tile.h#new...
cc/resources/tile.h:24: ~Tile();
please wait with landing this until this change is no longer needed.

https://codereview.chromium.org/366113002/diff/550001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/366113002/diff/550001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:120: void ReleaseTileOnBothTree(TileVector
tiles) {
ReleaseTiles(TileVector* tiles)

[sohanjg, 2014-07-31 13:42:56]

https://codereview.chromium.org/366113002/diff/550001/cc/resources/prioritize...
File cc/resources/prioritized_tile_set_unittest.cc (right):

https://codereview.chromium.org/366113002/diff/550001/cc/resources/prioritize...
cc/resources/prioritized_tile_set_unittest.cc:80: void
ReleaseTileOnBothTree(std::vector<scoped_refptr<Tile> > tiles) {
On 2014/07/30 18:41:39, reveman wrote:
> nit: don't pass the vector by value. pass a pointer instead. the name of this
> function makes it sound like it's only one tile. think it should be "Tiles"
> instead of "Tile". i think you can also omit the OnBothTrees suffix.
> "ReleaseTiles" would be fine.

Done.

https://codereview.chromium.org/366113002/diff/550001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/366113002/diff/550001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:120: void ReleaseTileOnBothTree(TileVector
tiles) {
On 2014/07/30 18:41:39, reveman wrote:
> ReleaseTiles(TileVector* tiles)

Done.

[reveman, 2014-07-31 17:00:33]

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

https://codereview.chromium.org/366113002/diff/570001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:401: FreeResourcesForReleasedTiles();
I tried to refactor RefCountedManaged but I haven't been able to get a result
that I'm happy with. I think a more significant tile garbage collection
mechanism refactor is needed to do this properly.

Let's just keep released_tiles_ as a std::vector<Tile*> and add a
STLDeleteElements(&released_tiles_) call here for now so you can land this.

[sohanjg, 2014-08-01 07:34:18]

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

https://codereview.chromium.org/366113002/diff/570001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:401: FreeResourcesForReleasedTiles();
On 2014/07/31 17:00:33, reveman wrote:
> I tried to refactor RefCountedManaged but I haven't been able to get a result
> that I'm happy with. I think a more significant tile garbage collection
> mechanism refactor is needed to do this properly.
> 
> Let's just keep released_tiles_ as a std::vector<Tile*> and add a
> STLDeleteElements(&released_tiles_) call here for now so you can land this.

Invoking STLDelete.. will still need the Tile Dtor to be made public, are we ok
with that for now ?
Or we keep Tile as ScopedVector for now (as that will have the same overhead for
public Dtor)?

[reveman, 2014-08-01 11:47:12]

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

https://codereview.chromium.org/366113002/diff/570001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:401: FreeResourcesForReleasedTiles();
On 2014/08/01 07:34:17, sohanjg wrote:
> On 2014/07/31 17:00:33, reveman wrote:
> > I tried to refactor RefCountedManaged but I haven't been able to get a
result
> > that I'm happy with. I think a more significant tile garbage collection
> > mechanism refactor is needed to do this properly.
> > 
> > Let's just keep released_tiles_ as a std::vector<Tile*> and add a
> > STLDeleteElements(&released_tiles_) call here for now so you can land this.
> 
> Invoking STLDelete.. will still need the Tile Dtor to be made public, are we
ok
> with that for now ?
> Or we keep Tile as ScopedVector for now (as that will have the same overhead
for
> public Dtor)?

How about just doing prioritized_tiles_.Clear() here and then
CleanUpReleasedTiles()?

[sohanjg, 2014-08-01 12:18:37]

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

https://codereview.chromium.org/366113002/diff/570001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:401: FreeResourcesForReleasedTiles();
On 2014/08/01 11:47:12, reveman wrote:
> On 2014/08/01 07:34:17, sohanjg wrote:
> > On 2014/07/31 17:00:33, reveman wrote:
> > > I tried to refactor RefCountedManaged but I haven't been able to get a
> result
> > > that I'm happy with. I think a more significant tile garbage collection
> > > mechanism refactor is needed to do this properly.
> > > 
> > > Let's just keep released_tiles_ as a std::vector<Tile*> and add a
> > > STLDeleteElements(&released_tiles_) call here for now so you can land
this.
> > 
> > Invoking STLDelete.. will still need the Tile Dtor to be made public, are we
> ok
> > with that for now ?
> > Or we keep Tile as ScopedVector for now (as that will have the same overhead
> for
> > public Dtor)?
> 
> How about just doing prioritized_tiles_.Clear() here and then
> CleanUpReleasedTiles()?

Done.
yea..that shud work.

[reveman, 2014-08-01 14:13:14]

Lgtm with nit

https://codereview.chromium.org/366113002/diff/590001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/590001/cc/resources/tile.h#new...
cc/resources/tile.h:171: 
Nit: no need to add this blank line

[vmpstr, 2014-08-01 16:03:16]

lgtm2

https://codereview.chromium.org/366113002/diff/590001/cc/resources/prioritize...
File cc/resources/prioritized_tile_set.cc (right):

https://codereview.chromium.org/366113002/diff/590001/cc/resources/prioritize...
cc/resources/prioritized_tile_set.cc:84: bool PrioritizedTileSet::Empty() {
nit: I think the convention is to name this "IsEmpty"

[sohanjg, 2014-08-04 06:25:04]

Thanks! :)

https://codereview.chromium.org/366113002/diff/590001/cc/resources/prioritize...
File cc/resources/prioritized_tile_set.cc (right):

https://codereview.chromium.org/366113002/diff/590001/cc/resources/prioritize...
cc/resources/prioritized_tile_set.cc:84: bool PrioritizedTileSet::Empty() {
On 2014/08/01 16:03:16, vmpstr wrote:
> nit: I think the convention is to name this "IsEmpty"

Done.
Renamed, std::vector seems to be using ::Empty, is it convention for containers
you mean here ?

https://codereview.chromium.org/366113002/diff/590001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/366113002/diff/590001/cc/resources/tile.h#new...
cc/resources/tile.h:171: 
On 2014/08/01 14:13:14, reveman wrote:
> Nit: no need to add this blank line

Done.

[sohanjg, 2014-08-04 06:25:21]

The CQ bit was checked by sohan.jyoti@samsung.com

[commit-bot: I haz the power, 2014-08-04 06:26:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/366113002/610001

[commit-bot: I haz the power, 2014-08-04 08:21:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-08-04 08:43:53]

Change committed as 287304

[vmpstr, 2014-08-04 16:04:41]

https://codereview.chromium.org/366113002/diff/590001/cc/resources/prioritize...
File cc/resources/prioritized_tile_set.cc (right):

https://codereview.chromium.org/366113002/diff/590001/cc/resources/prioritize...
cc/resources/prioritized_tile_set.cc:84: bool PrioritizedTileSet::Empty() {
On 2014/08/04 06:25:03, sohanjg wrote:
> On 2014/08/01 16:03:16, vmpstr wrote:
> > nit: I think the convention is to name this "IsEmpty"
> 
> Done.
> Renamed, std::vector seems to be using ::Empty, is it convention for
containers
> you mean here ?

Yeah cc containers or any thing that has a concept of being empty is more often
IsEmpty, stl is still ::empty :)

[sohanjg, 2015-11-18 09:43:03]

Description was changed from

==========
cc: Do not cleanup tiles with raster tasks.

During cleanup, this keeps the tiles around until the 
associated raster tasks completes running on the worker thread.
We reset the priority of the tile on the tree it lies before
dropping it.

BUG=386039

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

to

==========
cc: Do not cleanup tiles with raster tasks.

During cleanup, this keeps the tiles around until the 
associated raster tasks completes running on the worker thread.
We reset the priority of the tile on the tree it lies before
dropping it.

BUG=386039

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