Added a vector to TileManager to explicitly track live or allocated tiles.

Added a vector to TileManager to explicitly track live or allocated tiles.

The new vector should be exactly the elements that AssignGpuMemoryToTiles cares about.  In addition this patch fuses some loops (4 loops fused into 2).  I'm measuring about a 70% performance boost to ManageTiles on my linux machine.


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

[whunt, 2013-02-06 23:08:49]

Spiratually similar to a previous patch of mine with the _live_tiles.  This one
also tracks tiles that have memory allocated to them.  I fused some loops that
looked like they could be easily fused, which improves performance.  I'm curious
about the fact that we call DidTileBinChange twice for each element, is this the
intended behavior?

[nduca, 2013-02-06 23:54:02]

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

https://codereview.chromium.org/12226046/diff/5001/cc/tile_manager.cc#newcode234
cc/tile_manager.cc:234: std::sort(live_or_allocated_tiles_.begin(),
you might need to rebase on origin/master so you can delete your sort speeder
upper stuff?

https://codereview.chromium.org/12226046/diff/5001/cc/tile_manager.cc#newcode278
cc/tile_manager.cc:278: switch (tree_priority) {
can we speed this step by considering alive ness?

https://codereview.chromium.org/12226046/diff/5001/cc/tile_manager.cc#newcode296
cc/tile_manager.cc:296: mts.bin[HIGH_PRIORITY_BIN] =
BinFromTilePriority(prio[HIGH_PRIORITY_BIN]);
if neither tile is alive, is this even needed? We kind of know its going into
never, and so we can skip the whole Binning

https://codereview.chromium.org/12226046/diff/5001/cc/tile_manager.cc#newcode301
cc/tile_manager.cc:301: BinFromTilePriority(tile->priority(ACTIVE_TREE)),
this bit seems heavy to take in the non-alive case?

https://codereview.chromium.org/12226046/diff/5001/cc/tile_manager.cc#newcode369
cc/tile_manager.cc:369: for (size_t i = 0; i < all_tiles_.size(); i++) {
does this really need all_tiles?

https://codereview.chromium.org/12226046/diff/5001/cc/tile_manager.cc#newcode457
cc/tile_manager.cc:457: // By clearing the tiles_that_need_to_be_rasterized_
vector and
You should update and fuse all these comments together.

https://codereview.chromium.org/12226046/diff/5001/cc/tile_manager.cc#newcode783
cc/tile_manager.cc:783: void TileManager::DidTileBinChange(Tile* tile,
while you're here, can you change this DidTileTreeBinChange? This ONLY reads the
tree_bin right?

https://codereview.chromium.org/12226046/diff/5001/cc/tile_manager.cc#newcode784
cc/tile_manager.cc:784: TileManagerBin bin,
would this be better titled new_tree_bin?

[whunt, 2013-02-07 00:30:48]

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

https://codereview.chromium.org/12226046/diff/5001/cc/tile_manager.cc#newcode369
cc/tile_manager.cc:369: for (size_t i = 0; i < all_tiles_.size(); i++) {
On 2013/02/06 23:54:02, nduca wrote:
> does this really need all_tiles?

It shouldn't, I'll change that.

[whunt, 2013-02-11 19:01:45]

Did mail for this ever go out?

[reveman, 2013-02-11 22:42:28]

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc#newcode52
cc/tile_manager.cc:52: if (prio.time_to_visible_in_seconds >=
std::numeric_limits<float>::max())
why did you change this?

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:349: tile->GetResourceId() != 0) {
what if the tile is being initialized? GetResourceId() will only return non zero
when tile has finished initializing.

[whunt, 2013-02-11 22:58:41]

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc#newcode52
cc/tile_manager.cc:52: if (prio.time_to_visible_in_seconds >=
std::numeric_limits<float>::max())
On 2013/02/11 22:42:28, David Reveman wrote:
> why did you change this?

The code that calculates distance actually returns
std::numeric_limits<float>::infinity() when things don't intersect.  That case
should be caught early.  I'm not actually sure why
std::numeric_limits<float>::max() was ever used, I don't think it's correct. 
However in order to preserve the behavior in case max *is* ever used I just made
it a >= rather than == to catch both cases.

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:349: tile->GetResourceId() != 0) {
On 2013/02/11 22:42:28, David Reveman wrote:
> what if the tile is being initialized? GetResourceId() will only return non
zero
> when tile has finished initializing.

Then we can't free it this round, we'll have to wait until a frame on which it
has been initialized before it'll get freed (which makes sense).

[reveman, 2013-02-11 23:53:57]

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc#newcode52
cc/tile_manager.cc:52: if (prio.time_to_visible_in_seconds >=
std::numeric_limits<float>::max())
On 2013/02/11 22:58:41, whunt wrote:
> On 2013/02/11 22:42:28, David Reveman wrote:
> > why did you change this?
> 
> The code that calculates distance actually returns
> std::numeric_limits<float>::infinity() when things don't intersect.  That case
> should be caught early.  I'm not actually sure why
> std::numeric_limits<float>::max() was ever used, I don't think it's correct. 
> However in order to preserve the behavior in case max *is* ever used I just
made
> it a >= rather than == to catch both cases.

Ok, nice catch. I'm seeing use of both max() and infinity() in
tile_priority.h/picture_layer_tiling.cc. This can be causing some of
prioritizing issues we're seeing and I think it deserves it's own cl. Can you
quickly create a separate patch for this and instead of using >= here, create a
TilePriority::kMaxTimeToVisibleInSeconds constant and make sure it's used where
appropriate?

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:349: tile->GetResourceId() != 0) {
On 2013/02/11 22:58:41, whunt wrote:
> On 2013/02/11 22:42:28, David Reveman wrote:
> > what if the tile is being initialized? GetResourceId() will only return non
> zero
> > when tile has finished initializing.
> 
> Then we can't free it this round, we'll have to wait until a frame on which it
> has been initialized before it'll get freed (which makes sense).

but you're not adding it to the vector, which means it wont be included when
computing unreleasable_bytes in AssignGpuMemoryToTiles()? That doesn't seem
right.

[nduca, 2013-02-12 05:37:45]

Heads up: We need something for this in by Tuesday build.

[whunt, 2013-02-12 19:15:36]

updating

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/12226046/diff/11001/cc/tile_manager.cc#newcod...
cc/tile_manager.cc:349: tile->GetResourceId() != 0) {
On 2013/02/11 23:53:57, David Reveman wrote:
> On 2013/02/11 22:58:41, whunt wrote:
> > On 2013/02/11 22:42:28, David Reveman wrote:
> > > what if the tile is being initialized? GetResourceId() will only return
non
> > zero
> > > when tile has finished initializing.
> > 
> > Then we can't free it this round, we'll have to wait until a frame on which
it
> > has been initialized before it'll get freed (which makes sense).
> 
> but you're not adding it to the vector, which means it wont be included when
> computing unreleasable_bytes in AssignGpuMemoryToTiles()? That doesn't seem
> right.

I added || !tile->managed_state().can_be_freed

[whunt, 2013-02-12 19:39:37]

Does this address the issue David?

[reveman, 2013-02-12 20:07:12]

lgtm

[nduca, 2013-02-12 20:23:33]

lgtm but how do we increase our confidence in this patch such that we can ship
it in tonights beta?

[commit-bot: I haz the power, 2013-02-12 21:16:21]

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

[nduca, 2013-02-13 03:30:12]

Committed manually as r182105 (presubmit successful).