cc: Mirror LiveTilesRect and tiles between active and recycled trees.

cc: Mirror LiveTilesRect and tiles between active and recycled trees.

When the live tiles rect changes on the active tiling, we also remove
tiles on the recycled tiling to avoid unshared tiles in the recycle
tree. But when a tile is created on the active tree, we should also
share the tile to the recycle tree. Since we're creating tiles, we
also need to update the live tiles rect so tiles do not exist outside
of it. So now the live tiles rect is simply updated on both trees at
once.

This ensures that on the next commit, if the picture layer did not
get any invalidations, and doesn't push properties, it still has all
the tiles in its live tiles rect so we can do ready-to-activate checks
correctly.

Secondly, when activating, if any tiles were present on the pending
tree but are not on the active tree (can happen due to missing
recordings on the active tree), then share those tiles to the active
tree during activation.

This patch fixes DCHECKs occuring in PictureLayerTiling's
CloneTilesAndPropertiesFrom() where the pending and active tiling were
not ending up with the same number of tiles.

R=enne, vmpstr
BUG=387116
Committed: https://crrev.com/bd6d7bc43094d50c0ce3d785aa270b49efb5959a
Cr-Commit-Position: refs/heads/master@{#308174}

[danakj, 2014-12-11 23:38:08]

https://codereview.chromium.org/799463005/diff/40001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/799463005/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:223: // Create any missing tiles from the
|twin_tiling|.
I should write a test for this change: Activate with missing recordings, then
commit with those recordings filled in and the same LiveTilesRect

[enne (OOO), 2014-12-11 23:53:26]

Do you feel like it's needless churn to create/remove things from the recycle
tree? Like, if you scroll up and down, you'll have to touch the recycle tree a
bunch for no reason, when it seems like you could just SetLiveTilesRect once to
match after commit on the pending tree?  Or is that more complicated?  That's
what I was trying to get at in irc.

[danakj, 2014-12-12 17:14:26]

On Thu, Dec 11, 2014 at 3:53 PM, <enne@chromium.org> wrote:

> Do you feel like it's needless churn to create/remove things from the
> recycle
> tree? Like, if you scroll up and down, you'll have to touch the recycle
> tree a
> bunch for no reason, when it seems like you could just SetLiveTilesRect
> once to
> match after commit on the pending tree?  Or is that more complicated?
> That's
> what I was trying to get at in irc.
>

Ah, that is true, but the reason we started removing tiles on the recycle
tree is because for the iterators we don't want any unshared resources in
the recycle tree, cuz it won't know how to evict them. So I think this is
what we want (for now?).

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2014-12-12 17:59:40]

On 2014/12/12 17:14:26, danakj wrote:
> On Thu, Dec 11, 2014 at 3:53 PM, <mailto:enne@chromium.org> wrote:
> 
> > Do you feel like it's needless churn to create/remove things from the
> > recycle
> > tree? Like, if you scroll up and down, you'll have to touch the recycle
> > tree a
> > bunch for no reason, when it seems like you could just SetLiveTilesRect
> > once to
> > match after commit on the pending tree?  Or is that more complicated?
> > That's
> > what I was trying to get at in irc.
> >
> 
> Ah, that is true, but the reason we started removing tiles on the recycle
> tree is because for the iterators we don't want any unshared resources in
> the recycle tree, cuz it won't know how to evict them. So I think this is
> what we want (for now?).
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Yeah, we shouldn't have any unshared stuff on recycled because eviction
iterators don't visit the recycle tree (as I recall, it's a mess to deal with
updating priorities since we don't really need to on recycled unshared, maybe
it's better now). If we do end up deleting from recycled, then we have to
create, right?

[vmpstr, 2014-12-12 18:01:34]

https://codereview.chromium.org/799463005/diff/40001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/799463005/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:164: recycled_twin->tiles_[key] = tile;
What happens if this tile is outside of the recycled twin's ltr? That is, when
recycled becomes pending and starts doing work, won't it trip up if it expects
nothing but gets something?

[danakj, 2014-12-12 18:01:35]

On Fri, Dec 12, 2014 at 9:59 AM, <vmpstr@chromium.org> wrote:

> On 2014/12/12 17:14:26, danakj wrote:
>
>  On Thu, Dec 11, 2014 at 3:53 PM, <mailto:enne@chromium.org> wrote:
>>
>
>  > Do you feel like it's needless churn to create/remove things from the
>> > recycle
>> > tree? Like, if you scroll up and down, you'll have to touch the recycle
>> > tree a
>> > bunch for no reason, when it seems like you could just SetLiveTilesRect
>> > once to
>> > match after commit on the pending tree?  Or is that more complicated?
>> > That's
>> > what I was trying to get at in irc.
>> >
>>
>
>  Ah, that is true, but the reason we started removing tiles on the recycle
>> tree is because for the iterators we don't want any unshared resources in
>> the recycle tree, cuz it won't know how to evict them. So I think this is
>> what we want (for now?).
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> Yeah, we shouldn't have any unshared stuff on recycled because eviction
> iterators don't visit the recycle tree (as I recall, it's a mess to deal
> with
> updating priorities since we don't really need to on recycled unshared,
> maybe
> it's better now). If we do end up deleting from recycled, then we have to
> create, right?
>

Right that's what this CL is doing (other than the change in PLT::Clone)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-12-12 18:02:48]

https://codereview.chromium.org/799463005/diff/40001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/799463005/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:164: recycled_twin->tiles_[key] = tile;
On 2014/12/12 18:01:34, vmpstr wrote:
> What happens if this tile is outside of the recycled twin's ltr? That is, when
> recycled becomes pending and starts doing work, won't it trip up if it expects
> nothing but gets something?

This is a good question.. we should maybe set the LTR on the recycled tiling
when we set it on the active?

[vmpstr, 2014-12-12 18:07:57]

On 2014/12/12 18:02:48, danakj wrote:
>
https://codereview.chromium.org/799463005/diff/40001/cc/resources/picture_lay...
> File cc/resources/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/799463005/diff/40001/cc/resources/picture_lay...
> cc/resources/picture_layer_tiling.cc:164: recycled_twin->tiles_[key] = tile;
> On 2014/12/12 18:01:34, vmpstr wrote:
> > What happens if this tile is outside of the recycled twin's ltr? That is,
when
> > recycled becomes pending and starts doing work, won't it trip up if it
expects
> > nothing but gets something?
> 
> This is a good question.. we should maybe set the LTR on the recycled tiling
> when we set it on the active?

Hmmmm... I think that would work, assuming that
pending_twin/active_twin/recycled_twin that we get when this == recycled are
valid, which I imagine they are?

[danakj, 2014-12-12 18:11:02]

On Fri, Dec 12, 2014 at 10:07 AM, <vmpstr@chromium.org> wrote:

> On 2014/12/12 18:02:48, danakj wrote:
>
> https://codereview.chromium.org/799463005/diff/40001/cc/
> resources/picture_layer_tiling.cc
>
>> File cc/resources/picture_layer_tiling.cc (right):
>>
>
>
> https://codereview.chromium.org/799463005/diff/40001/cc/
> resources/picture_layer_tiling.cc#newcode164
>
>> cc/resources/picture_layer_tiling.cc:164: recycled_twin->tiles_[key] =
>> tile;
>> On 2014/12/12 18:01:34, vmpstr wrote:
>> > What happens if this tile is outside of the recycled twin's ltr? That
>> is,
>>
> when
>
>> > recycled becomes pending and starts doing work, won't it trip up if it
>>
> expects
>
>> > nothing but gets something?
>>
>
>  This is a good question.. we should maybe set the LTR on the recycled
>> tiling
>> when we set it on the active?
>>
>
> Hmmmm... I think that would work, assuming that
> pending_twin/active_twin/recycled_twin that we get when this == recycled
> are
> valid, which I imagine they are?
>

They should be ya.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-12-12 18:46:41]

OK I've added a unit test for the change to PLT::CloneTilesAndPropertiesFrom().

I'll see about making the SetLiveTilesRect change now.

Maybe this means we can stop passing null recycle twins all over the place.

[danakj, 2014-12-12 19:19:15]

Updated for SetLTR

[vmpstr, 2014-12-12 19:55:30]

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

https://codereview.chromium.org/799463005/diff/100001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:744: for (auto it = tiles_.begin(); it !=
tiles_.end(); ++it) {
So did we want to somehow get is_recycled, and

DCHECK_IMPLIES(is_recycled, tile->is_shared())

? or did we decide against that for now?

[danakj, 2014-12-12 20:04:48]

PTAL

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

https://codereview.chromium.org/799463005/diff/100001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:744: for (auto it = tiles_.begin(); it !=
tiles_.end(); ++it) {
On 2014/12/12 19:55:30, vmpstr wrote:
> So did we want to somehow get is_recycled, and
> 
> DCHECK_IMPLIES(is_recycled, tile->is_shared())
> 
> ? or did we decide against that for now?

Oh, I guess I can add that now ya. I git reset --hard and lost it.

[vmpstr, 2014-12-12 20:10:32]

lgtm

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

https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:742: void
PictureLayerTiling::VerifyLiveTilesRect(bool is_on_recycle_tree) const {
hah, nice.

[danakj, 2014-12-12 20:54:06]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-12-12 20:55:06]

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

[commit-bot: I haz the power, 2014-12-12 22:00:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-12 22:02:21]

Patchset 7 (id:??) landed as
https://crrev.com/bd6d7bc43094d50c0ce3d785aa270b49efb5959a
Cr-Commit-Position: refs/heads/master@{#308174}

[danakj, 2014-12-30 16:44:58]

So https://code.google.com/p/chromium/issues/detail?id=442402#c5 points to this
CL as causing a input latency regression. Most of this CL is DCHECK changes or
meaningless stuff.

There's 2 places where things actually changed. I feel like the 2nd one can't be
meaningful. So it must be the first place.. but we do need to clone new tiles
from the pending to the active tree.

Maybe iterating the invalid region would be better than iterating all the tiles
(in the live rect) against my own intuition (since it requires a lot of hash
table lookups for the x/y indicies).

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

https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:224: for (const auto& tile_map_pair :
twin_tiling.tiles_) {
1 here: We walk all pending tiles, and if they are not shared, create an active
tile that is shared.

https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:731: CreateTile(key.first, key.second,
twin_tiling, recycled_twin);
2 here: We create and add a tile to the recycled tiling's hash map.

[danakj, 2014-12-30 16:46:28]

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

https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:224: for (const auto& tile_map_pair :
twin_tiling.tiles_) {
On 2014/12/30 16:44:58, danakj wrote:
> 1 here: We walk all pending tiles, and if they are not shared, create an
active
> tile that is shared.

Or maybe since we know the tile will be shared we should do this more cheaply,
like how we create a tile for the recycled twin, without needing to compare
invalidation, scale rects, and such.

[vmpstr, 2014-12-30 17:16:55]

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

https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:224: for (const auto& tile_map_pair :
twin_tiling.tiles_) {
On 2014/12/30 16:46:28, danakj wrote:
> On 2014/12/30 16:44:58, danakj wrote:
> > 1 here: We walk all pending tiles, and if they are not shared, create an
> active
> > tile that is shared.
> 
> Or maybe since we know the tile will be shared we should do this more cheaply,
> like how we create a tile for the recycled twin, without needing to compare
> invalidation, scale rects, and such.

You mean when we're creating the pending tile, if it's not shared but should be,
we set it immediately on the active tree? That seems a bit messy, since it's
like a pre-activation activation since the raster source on that tile has to be
the pending source... Alternatively we could somehow maintain a set of tile
indexes that should be created but are not due to client not creating the tile.

Something like 
... tile = client_->CreateTile(...)
if (tile) {
 ...
} else {
 want_but_dont_have_tile.insert(index);
}

Although I can already see that can be a major pain to maintain...

[danakj, 2014-12-30 17:20:47]

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

https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:224: for (const auto& tile_map_pair :
twin_tiling.tiles_) {
On 2014/12/30 17:16:55, vmpstr wrote:
> On 2014/12/30 16:46:28, danakj wrote:
> > On 2014/12/30 16:44:58, danakj wrote:
> > > 1 here: We walk all pending tiles, and if they are not shared, create an
> > active
> > > tile that is shared.
> > 
> > Or maybe since we know the tile will be shared we should do this more
cheaply,
> > like how we create a tile for the recycled twin, without needing to compare
> > invalidation, scale rects, and such.
> 
> You mean when we're creating the pending tile, if it's not shared but should
be,
> we set it immediately on the active tree? That seems a bit messy, since it's
> like a pre-activation activation since the raster source on that tile has to
be
> the pending source... Alternatively we could somehow maintain a set of tile
> indexes that should be created but are not due to client not creating the
tile.
> 
> Something like 
> ... tile = client_->CreateTile(...)
> if (tile) {
>  ...
> } else {
>  want_but_dont_have_tile.insert(index);
> }
> 
> Although I can already see that can be a major pain to maintain...

I mean like instead of

CreateTile(key, foo bar, twin, etc);

tile->set_shared(true);
tiles_[key] = tile;

Avoids the math/virtuals in CreateTile.

[vmpstr, 2014-12-30 18:03:58]

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

https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:224: for (const auto& tile_map_pair :
twin_tiling.tiles_) {
On 2014/12/30 17:20:47, danakj wrote:
> On 2014/12/30 17:16:55, vmpstr wrote:
> > On 2014/12/30 16:46:28, danakj wrote:
> > > On 2014/12/30 16:44:58, danakj wrote:
> > > > 1 here: We walk all pending tiles, and if they are not shared, create an
> > > active
> > > > tile that is shared.
> > > 
> > > Or maybe since we know the tile will be shared we should do this more
> cheaply,
> > > like how we create a tile for the recycled twin, without needing to
compare
> > > invalidation, scale rects, and such.
> > 
> > You mean when we're creating the pending tile, if it's not shared but should
> be,
> > we set it immediately on the active tree? That seems a bit messy, since it's
> > like a pre-activation activation since the raster source on that tile has to
> be
> > the pending source... Alternatively we could somehow maintain a set of tile
> > indexes that should be created but are not due to client not creating the
> tile.
> > 
> > Something like 
> > ... tile = client_->CreateTile(...)
> > if (tile) {
> >  ...
> > } else {
> >  want_but_dont_have_tile.insert(index);
> > }
> > 
> > Although I can already see that can be a major pain to maintain...
> 
> I mean like instead of
> 
> CreateTile(key, foo bar, twin, etc);
> 
> tile->set_shared(true);
> tiles_[key] = tile;
> 
> Avoids the math/virtuals in CreateTile. 

Oh I see, but still keep the loop here? That could work, although I'm not sure
it will save enough.

[danakj, 2014-12-30 19:32:12]

On 2014/12/30 18:03:58, vmpstr wrote:
>
https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
> File cc/resources/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
> cc/resources/picture_layer_tiling.cc:224: for (const auto& tile_map_pair :
> twin_tiling.tiles_) {
> On 2014/12/30 17:20:47, danakj wrote:
> > On 2014/12/30 17:16:55, vmpstr wrote:
> > > On 2014/12/30 16:46:28, danakj wrote:
> > > > On 2014/12/30 16:44:58, danakj wrote:
> > > > > 1 here: We walk all pending tiles, and if they are not shared, create
an
> > > > active
> > > > > tile that is shared.
> > > > 
> > > > Or maybe since we know the tile will be shared we should do this more
> > cheaply,
> > > > like how we create a tile for the recycled twin, without needing to
> compare
> > > > invalidation, scale rects, and such.
> > > 
> > > You mean when we're creating the pending tile, if it's not shared but
should
> > be,
> > > we set it immediately on the active tree? That seems a bit messy, since
it's
> > > like a pre-activation activation since the raster source on that tile has
to
> > be
> > > the pending source... Alternatively we could somehow maintain a set of
tile
> > > indexes that should be created but are not due to client not creating the
> > tile.
> > > 
> > > Something like 
> > > ... tile = client_->CreateTile(...)
> > > if (tile) {
> > >  ...
> > > } else {
> > >  want_but_dont_have_tile.insert(index);
> > > }
> > > 
> > > Although I can already see that can be a major pain to maintain...
> > 
> > I mean like instead of
> > 
> > CreateTile(key, foo bar, twin, etc);
> > 
> > tile->set_shared(true);
> > tiles_[key] = tile;
> > 
> > Avoids the math/virtuals in CreateTile. 
> 
> Oh I see, but still keep the loop here? That could work, although I'm not sure
> it will save enough.

Ya.. idk. Failing at running android perf test remotely for some reason. Will
have to peek again next week probly.

Keeping a list of "tiles we created on the pending tree" to create on the active
tree later might be a way to resolve this also. I think that's what you were
getting at? Is a good thought..

[vmpstr, 2014-12-30 19:33:39]

On 2014/12/30 19:32:12, danakj wrote:
> On 2014/12/30 18:03:58, vmpstr wrote:
> >
>
https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
> > File cc/resources/picture_layer_tiling.cc (right):
> > 
> >
>
https://codereview.chromium.org/799463005/diff/120001/cc/resources/picture_la...
> > cc/resources/picture_layer_tiling.cc:224: for (const auto& tile_map_pair :
> > twin_tiling.tiles_) {
> > On 2014/12/30 17:20:47, danakj wrote:
> > > On 2014/12/30 17:16:55, vmpstr wrote:
> > > > On 2014/12/30 16:46:28, danakj wrote:
> > > > > On 2014/12/30 16:44:58, danakj wrote:
> > > > > > 1 here: We walk all pending tiles, and if they are not shared,
create
> an
> > > > > active
> > > > > > tile that is shared.
> > > > > 
> > > > > Or maybe since we know the tile will be shared we should do this more
> > > cheaply,
> > > > > like how we create a tile for the recycled twin, without needing to
> > compare
> > > > > invalidation, scale rects, and such.
> > > > 
> > > > You mean when we're creating the pending tile, if it's not shared but
> should
> > > be,
> > > > we set it immediately on the active tree? That seems a bit messy, since
> it's
> > > > like a pre-activation activation since the raster source on that tile
has
> to
> > > be
> > > > the pending source... Alternatively we could somehow maintain a set of
> tile
> > > > indexes that should be created but are not due to client not creating
the
> > > tile.
> > > > 
> > > > Something like 
> > > > ... tile = client_->CreateTile(...)
> > > > if (tile) {
> > > >  ...
> > > > } else {
> > > >  want_but_dont_have_tile.insert(index);
> > > > }
> > > > 
> > > > Although I can already see that can be a major pain to maintain...
> > > 
> > > I mean like instead of
> > > 
> > > CreateTile(key, foo bar, twin, etc);
> > > 
> > > tile->set_shared(true);
> > > tiles_[key] = tile;
> > > 
> > > Avoids the math/virtuals in CreateTile. 
> > 
> > Oh I see, but still keep the loop here? That could work, although I'm not
sure
> > it will save enough.
> 
> Ya.. idk. Failing at running android perf test remotely for some reason. Will
> have to peek again next week probly.
> 
> Keeping a list of "tiles we created on the pending tree" to create on the
active
> tree later might be a way to resolve this also. I think that's what you were
> getting at? Is a good thought..

Yeah that might work. I'm just worried about it being a headache to do one thing
on the pending tree and another thing on the active tree in a lot of functions.
Whatever works I guess :)