Creates a live_tile_ list that manages live tiles.

Creates a live_tile_ list that manages live tiles.

This patch creates an explicit list of tiles with the is_live property.  This reduces the working set during SortTiles and other methods that loop over the tiles.


BUG=174202
NOTRY=true

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

[reveman, 2013-02-05 19:31:09]

Looks good.

Are you seeing a big difference between LiveTileCount and AllTileCount? It's not
clear to me when these two will be significantly different.

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

https://codereview.chromium.org/12213019/diff/1/cc/tile_manager.cc#newcode147
cc/tile_manager.cc:147: DCHECK(live_tiles_.size() == 0);
do you mind changing all these checks to DCHECK_EQ instead?

https://codereview.chromium.org/12213019/diff/1/cc/tile_manager.cc#newcode243
cc/tile_manager.cc:243: live_tiles_.resize(0);
nit: use live_tiles_.erase(live_tiles_.begin(), live_tiles_.end()) to be
consistent with how we reset vectors elsewhere in this file. if using resize(0)
is better then we should change it everywhere.

https://codereview.chromium.org/12213019/diff/1/cc/tile_manager.cc#newcode246
cc/tile_manager.cc:246: if ((*it)->priority( ACTIVE_TREE).is_live ||
nit: I like when things align but not sure the coding style allows you to add a
space between "(" and ACTIVE_TREE like this.

https://codereview.chromium.org/12213019/diff/1/cc/tile_manager.cc#newcode247
cc/tile_manager.cc:247: (*it)->priority(PENDING_TREE).is_live)
should we be using (*it)->combined_priority().is_live here instead?

[nduca, 2013-02-05 19:35:42]

lgtm

[commit-bot: I haz the power, 2013-02-05 21:07:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/12213019/1

[whunt, 2013-02-05 21:59:59]

https://chromiumcodereview.appspot.com/12213019/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://chromiumcodereview.appspot.com/12213019/diff/1/cc/tile_manager.cc#new...
cc/tile_manager.cc:243: live_tiles_.resize(0);
On 2013/02/05 19:31:09, David Reveman wrote:
> nit: use live_tiles_.erase(live_tiles_.begin(), live_tiles_.end()) to be
> consistent with how we reset vectors elsewhere in this file. if using
resize(0)
> is better then we should change it everywhere.

I believe that resize(0) is faster but probably not measurably so. (There's
likely a branch inside erase that checks to see if the iterator bound is equal
to end() and just calls resize in that case.)

I'll make this one consistent with the others regardless of efficiency because I
would rather touch less code for this patch.

https://chromiumcodereview.appspot.com/12213019/diff/1/cc/tile_manager.cc#new...
cc/tile_manager.cc:246: if ((*it)->priority( ACTIVE_TREE).is_live ||
On 2013/02/05 19:31:09, David Reveman wrote:
> nit: I like when things align but not sure the coding style allows you to add
a
> space between "(" and ACTIVE_TREE like this.

If the system doesn't bounce it and you don't bounce it, we're all good =). 
Style is there to make things readable and maintainable.

https://chromiumcodereview.appspot.com/12213019/diff/1/cc/tile_manager.cc#new...
cc/tile_manager.cc:247: (*it)->priority(PENDING_TREE).is_live)
On 2013/02/05 19:31:09, David Reveman wrote:
> should we be using (*it)->combined_priority().is_live here instead?

To be honest, I have no idea, should we?.  I'm happy to change it.  I did what I
believe is conservative.  I'm not well versed in why we even have more than one
priority.

[reveman, 2013-02-05 22:16:41]

https://chromiumcodereview.appspot.com/12213019/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://chromiumcodereview.appspot.com/12213019/diff/1/cc/tile_manager.cc#new...
cc/tile_manager.cc:243: live_tiles_.resize(0);
On 2013/02/05 22:00:00, whunt wrote:
> On 2013/02/05 19:31:09, David Reveman wrote:
> > nit: use live_tiles_.erase(live_tiles_.begin(), live_tiles_.end()) to be
> > consistent with how we reset vectors elsewhere in this file. if using
> resize(0)
> > is better then we should change it everywhere.
> 
> I believe that resize(0) is faster but probably not measurably so. (There's
> likely a branch inside erase that checks to see if the iterator bound is equal
> to end() and just calls resize in that case.)
> 
> I'll make this one consistent with the others regardless of efficiency because
I
> would rather touch less code for this patch.

sg, we can change all this to resize(0) later.

https://chromiumcodereview.appspot.com/12213019/diff/1/cc/tile_manager.cc#new...
cc/tile_manager.cc:247: (*it)->priority(PENDING_TREE).is_live)
On 2013/02/05 22:00:00, whunt wrote:
> On 2013/02/05 19:31:09, David Reveman wrote:
> > should we be using (*it)->combined_priority().is_live here instead?
> 
> To be honest, I have no idea, should we?.  I'm happy to change it.  I did what
I
> believe is conservative.  I'm not well versed in why we even have more than
one
> priority.

the result is the same right now. look at "TilePriority(const TilePriority&
active, const TilePriority& pending)" constructor in tile_priority.h.

I think it's fine to leave it like this for now.

https://chromiumcodereview.appspot.com/12213019/diff/1/cc/tile_manager.cc#new...
cc/tile_manager.cc:371: for(size_t i = 0; i < all_tiles_.size(); i++) {
nit: you didn't introduce this but I'd be happy if you fixed it while changing
this line: missing space before ( in for(

[commit-bot: I haz the power, 2013-02-05 22:54:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/12213019/2002

[commit-bot: I haz the power, 2013-02-05 23:08:31]

Change committed as 180811

[reveman, 2013-02-06 03:20:23]

lgtm with style nits

I'm not a huge fan of iterating over all tiles 3 times in
AssignGpuMemoryToTiles() but I like the simplicity of that approach. I assume
you have verified that we still get a similar performance improvement.

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:183: it != live_tiles_.end(); it++) {
nit: test statement should be vertically aligned with init statement instead of
having 4 space indent

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:190: it != all_tiles_.end(); it++) {
nit: vertical alignment wrong here too

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:245: it != all_tiles_.end(); ++it) {
nit: vertical alignment

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:255: it != live_tiles_.end(); ++it) {
nit: vertical alignment

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:314: it != live_tiles_.end(); ++it) {
nit: vertical alignment

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:371: for(size_t i = 0; i < all_tiles_.size(); i++) {
nit: missing space before ( in for(

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:451: it != all_tiles_.end(); ++it) {
nit: vertical alignment

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:472: it != all_tiles_.end(); ++it) {
nit: vertical alignment

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:479: it != all_tiles_.end(); ++it) {
nit: vertical alignment

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:491: it != live_tiles_.end(); ++it) {
nit: vertical alignment

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:515: it != all_tiles_.end(); ++it) {
nit: vertical alignment

https://chromiumcodereview.appspot.com/12213019/diff/2003/cc/tile_manager.cc#...
cc/tile_manager.cc:524: 
nit: extra blank line

[nduca, 2013-02-06 04:09:39]

This patch needs more work.

[nduca, 2013-02-06 04:17:17]

I dont like these added loops to all tiles. I think you should move tiles that
are non-live and using memory into a special vector and check only that during
Assign, such that that list only shrinks over time.

Moreover, I'm pretty sure you're still leaking or leaving stale state. The tiles
not included in the live tiles vector aren't being given the correct bin, nor
tree_bin [because we dont call DidBlahBlah]. Finally, we're not giving it the
right gpu_memory_manager_bin.

[reveman, 2013-02-06 04:25:08]

yes, this is not ready. you can make this work by moving live_tiles_.push_back()
to the loop where we do bin_map[mts.bin[i]] and have the other loops iterate
over all_tiles. essentially only letting SortTiles() and the loop that build the
raster vector use live_tiles.