cc: Speed up CleanUpImageDecodeTasks a bit.

cc: Speed up CleanUpImageDecodeTasks a bit.

This patch keeps the state of used layers and updates it in register
and unregister tile. This eliminates the need for CleanUpImageDecodeTasks.

This also provides a fix to the memory limits in the perftest, so by
comparison the performance should drop significantly. However, this is
only because the perftests are now properly testing the full
ManageTiles code path. The patch to remove CleanUpImageDecodeTasks
actually improves performance by about 15%.

BUG=283777
R=reveman@chromium.org,tomhudson@chromium.org

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

[vmpstr, 2013-09-03 23:51:04]

Please take a look.

[reveman, 2013-09-04 18:18:52]

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

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:628: void
TileManager::CleanUpUnusedImageDecodeTasks() {
can we clean up this function somehow? I find it hard to follow what is done
here after this change.

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:644: ++it) {
do we need this loop at all?

[tomhudson, 2013-09-04 18:38:34]

If we can't see results better than the early-out on cc_perftests, can we find
webpages where this makes a difference? Or add traces or a histogram that give
us some reason to think it's important?

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

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:644: ++it) {
On 2013/09/04 18:18:52, David Reveman wrote:
> do we need this loop at all?

Is it there just for safety? (If we somehow have a task whose layer isn't in the
used_layer_counts_ list at all...)

It'd be less necessary insurance if we didn't do the used_layer_counts_.erase()
in 654, but if the set of layer IDs can get large that's no good.

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:649: // Erase unused layers from
|image_decode_tasks_|.
Nit: comment now incomplete.

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

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.h#n...
cc/resources/tile_manager.h:179: typedef base::hash_map<int, int> LayerCountMap;
I assume we're using hash_map here instead of something directly addressable
because the range of layer IDs is large and very sparsely used?

[enne (OOO), 2013-09-04 18:45:23]

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

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.h#n...
cc/resources/tile_manager.h:179: typedef base::hash_map<int, int> LayerCountMap;
On 2013/09/04 18:38:34, tomhudson wrote:
> I assume we're using hash_map here instead of something directly addressable
> because the range of layer IDs is large and very sparsely used?

It really depends on the page.  Scrollbars in particular get attached and
detached a lot, burning through layer id space.  I think it would be unwise to
assume that layer ids are tightly packed or small in range.

[vmpstr, 2013-09-04 19:36:40]

So it turns out the perftests were not really testing the second half of manage
tiles. That is, it wasn't updated with the proper resource limits when that was
added, so we were never scheduling any work to be done. That would still be a
valid test, I guess. However, the scheduling of raster tasks is the thing that
prevents the early out in CleanUpImageDecodeTasks to be useful (I made a note on
the specific line below). 

I have "fixed" the perftest, but that basically means that we need to
re-baseline our understanding of how well or poorly it performs (since it's now
doing a bunch more work now). This particular patch improves performance by
about 15% over just the perftest fix.

Let me know if you'd like these two things to go in separately.

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

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:628: void
TileManager::CleanUpUnusedImageDecodeTasks() {
On 2013/09/04 18:18:52, David Reveman wrote:
> can we clean up this function somehow? I find it hard to follow what is done
> here after this change.

Done.

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:644: ++it) {
On 2013/09/04 18:38:34, tomhudson wrote:
> On 2013/09/04 18:18:52, David Reveman wrote:
> > do we need this loop at all?
> 
> Is it there just for safety? (If we somehow have a task whose layer isn't in
the
> used_layer_counts_ list at all...)
> 
> It'd be less necessary insurance if we didn't do the
used_layer_counts_.erase()
> in 654, but if the set of layer IDs can get large that's no good.

Yeah I think this was just insurance. The layer ids in used_layer_counts
contains all of the layer ids in image_decode_tasks (and possibly then some). So
it's sufficient to just check the "refcount" on the layer counts to determine if
the layer is not used. If it isn't, we can delete it from both maps.

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:649: // Erase unused layers from
|image_decode_tasks_|.
On 2013/09/04 18:38:34, tomhudson wrote:
> Nit: comment now incomplete.

Done.

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

https://codereview.chromium.org/23757024/diff/1/cc/resources/tile_manager.h#n...
cc/resources/tile_manager.h:179: typedef base::hash_map<int, int> LayerCountMap;
On 2013/09/04 18:45:24, enne wrote:
> On 2013/09/04 18:38:34, tomhudson wrote:
> > I assume we're using hash_map here instead of something directly addressable
> > because the range of layer IDs is large and very sparsely used?
> 
> It really depends on the page.  Scrollbars in particular get attached and
> detached a lot, burning through layer id space.  I think it would be unwise to
> assume that layer ids are tightly packed or small in range.

Right.. I think it would be a waste to grow this to the max of all layer ids (in
order to be directly addressable. From the few pages I looked at, it seems that
we have a handful of layers, but their IDs can be in the range of >1000 (for
instance on sky.com)

https://codereview.chromium.org/23757024/diff/9001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/23757024/diff/9001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:746: PixelRefTaskMap& existing_pixel_refs =
image_decode_tasks_[tile->layer_id()];
This is the line that prevents the early out from triggering in basically all
but very few (degenerate) situations.

[reveman, 2013-09-04 21:25:17]

https://codereview.chromium.org/23757024/diff/9001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/23757024/diff/9001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:647: used_layer_counts_.erase(*it);
can we move these two lines to UnregisterTile and remove this function
completely?

[vmpstr, 2013-09-04 22:03:49]

PTAL.

https://codereview.chromium.org/23757024/diff/9001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/23757024/diff/9001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:647: used_layer_counts_.erase(*it);
On 2013/09/04 21:25:17, David Reveman wrote:
> can we move these two lines to UnregisterTile and remove this function
> completely?

Good idea. Done.

[reveman, 2013-09-05 00:29:27]

https://codereview.chromium.org/23757024/diff/13001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/23757024/diff/13001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:205: int& count =
used_layer_counts_[tile->layer_id()];
I think it would be more efficient and a bit cleaner if you use an iterator here
and erase using it below.

[tomhudson, 2013-09-05 07:32:23]

https://codereview.chromium.org/23757024/diff/13001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/23757024/diff/13001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:205: int& count =
used_layer_counts_[tile->layer_id()];
On 2013/09/05 00:29:27, David Reveman wrote:
> I think it would be more efficient and a bit cleaner if you use an iterator
here
> and erase using it below.

At first I didn't understand why, but your point is that if we get an iterator
to used_layer_counts_.find(tile->layer_id()) we only have to do that O(lg n)
lookup once, instead of twice?

[reveman, 2013-09-05 16:31:48]

On 2013/09/05 07:32:23, tomhudson wrote:
>
https://codereview.chromium.org/23757024/diff/13001/cc/resources/tile_manager.cc
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/23757024/diff/13001/cc/resources/tile_manager...
> cc/resources/tile_manager.cc:205: int& count =
> used_layer_counts_[tile->layer_id()];
> On 2013/09/05 00:29:27, David Reveman wrote:
> > I think it would be more efficient and a bit cleaner if you use an iterator
> here
> > and erase using it below.
> 
> At first I didn't understand why, but your point is that if we get an iterator
> to used_layer_counts_.find(tile->layer_id()) we only have to do that O(lg n)
> lookup once, instead of twice?

right. unlikely to have a significant perf impact but I think it's still cleaner
to do it that way when possible.

[vmpstr, 2013-09-05 18:34:43]

PTAL.

[reveman, 2013-09-05 19:04:25]

lgtm. I think this is a nice cleanup even if it doesn't provide a performance
improvement.

[commit-bot: I haz the power, 2013-09-05 19:11:10]

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

[commit-bot: I haz the power, 2013-09-06 08:09:38]

Change committed as 221638