cc: Clean up tilings in UpdateTiles as well as in AppendQuads.

cc: Clean up tilings in UpdateTiles as well as in AppendQuads.

Sometimes we get into situations where we get frequent UpdateTiles,
but no AppendQuads. As a result we can start accumulating tilings
(during pinch zoom for example), but we never free any of them, since
the code to free tilings happens only during AppendQuads. This patch
remedies this by ensuring that if AppendQuads doesn't clean up the
tilings, and we've added some since last time we ran UpdateTiles,
we clean them up.

R=danakj
BUG=452304
Committed: https://crrev.com/5c115e7db2f2845b5fe4a687dc33d5f2340b5d60
Cr-Commit-Position: refs/heads/master@{#313428}

[vmpstr, 2015-01-27 21:34:48]

Please take a look.

[danakj, 2015-01-27 21:56:59]

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:459: // a chance to do so, as inidicated by
|need_tiling_cleanup_|.
can you say why you only clean up if AppendQuads didn't?

this would make us cleanup if we added a new tiling above this in
AddTilingsForRasterScale immediately, before append quads got a chance to clean
up. maybe that's okay? it doesn't seem intentional/obvious from the way the code
is organized tho..

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1006: const std::vector<PictureLayerTiling*>&
used_tilings) {
why bother passing it in if it's a member on the class anyways?

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:920:
active_layer_->AddUsedTiling(active_layer_->tilings()->tiling_at(0));
Maybe a MarkAllTilingsUsed() and MarkTilingUsed(Tiling*)?

[vmpstr, 2015-01-27 22:53:59]

PTAL

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:459: // a chance to do so, as inidicated by
|need_tiling_cleanup_|.
On 2015/01/27 21:56:59, danakj wrote:
> can you say why you only clean up if AppendQuads didn't?
> 
> this would make us cleanup if we added a new tiling above this in
> AddTilingsForRasterScale immediately, before append quads got a chance to
clean
> up. maybe that's okay? it doesn't seem intentional/obvious from the way the
code
> is organized tho..

You're right that it's okay, but you're also right that this was unintentional.
I moved it above the adding tiling and expanded the comment a bit.

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:1006: const std::vector<PictureLayerTiling*>&
used_tilings) {
On 2015/01/27 21:56:59, danakj wrote:
> why bother passing it in if it's a member on the class anyways?

It makes it far easier to test this function.. I can work to change it, but I
would prefer to keep it this way. Wdyt?

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/881083002/diff/40001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:920:
active_layer_->AddUsedTiling(active_layer_->tilings()->tiling_at(0));
On 2015/01/27 21:56:59, danakj wrote:
> Maybe a MarkAllTilingsUsed() and MarkTilingUsed(Tiling*)?

Done.

[danakj, 2015-01-27 23:02:29]

https://codereview.chromium.org/881083002/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/881083002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:451: // active layer. Additionally, only clean
up if AppendQuads didn't have
This is just an optimization because it wouldn't actually remove anything then
right? Sorry I was hoping for a more "why" than an explanation about
need_tiling_cleanup_, which I think is clear.

https://codereview.chromium.org/881083002/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:794: need_tiling_cleanup_ = true;
What about when we UpdateRasterSource and clone the tiling from the active tree?
That makes a tiling here but doesn't set need_tiling_cleanup_. But then you
wouldn't want to cleanup after UpdateRasterSource until after AppendQuads had a
chance to go. (I a-little-bit feel like this bool is making things more
complicated than it's worth worrying about? Or maybe we could just set this to
true in UpdateTiles after it attempts to do the cleanup?)

[vmpstr, 2015-01-27 23:14:41]

Please take a look. I removed the bool, since it was just an optimization. If we
discovered that this regresses performance, then I will work to add the bool or
some other method to ensure we don't call the function too frequently.

[danakj, 2015-01-28 00:12:21]

LGTM

https://codereview.chromium.org/881083002/diff/80001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/881083002/diff/80001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:3648:
active_layer_->MarkAllTilingsUsed();
twice just for sure?

[vmpstr, 2015-01-28 00:16:07]

https://codereview.chromium.org/881083002/diff/80001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/881083002/diff/80001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:3648:
active_layer_->MarkAllTilingsUsed();
On 2015/01/28 00:12:20, danakj wrote:
> twice just for sure?

<_< thanks for catching this.

[vmpstr, 2015-01-28 00:16:16]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-01-28 00:16:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/881083002/100001

[commit-bot: I haz the power, 2015-01-28 04:09:57]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-28 04:11:14]

Patchset 6 (id:??) landed as
https://crrev.com/5c115e7db2f2845b5fe4a687dc33d5f2340b5d60
Cr-Commit-Position: refs/heads/master@{#313428}