Check if priority actually changed in Tile::SetPriority()

Check if priority actually changed in Tile::SetPriority()

To prevent unnecessary TileManager::WillModifyTilePriority() calls which
blocks ActivatePendingTree(), commits and the whole event queue.

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

[Xianzhu, 2013-03-20 00:03:39]

A question: what's TilePriority::current_screen_quad for? It's set in
TilePriority::set_current_screen_quad() but only accessed in
TilePriority::AsValue(). Is this for future use or can it be removed?

[enne (OOO), 2013-03-20 17:05:57]

https://codereview.chromium.org/12912008/diff/1/cc/resources/tile.cc
File cc/resources/tile.cc (right):

https://codereview.chromium.org/12912008/diff/1/cc/resources/tile.cc#newcode49
cc/resources/tile.cc:49: if (priority_[tree] != priority) {
style nit: early out return here instead.

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

https://codereview.chromium.org/12912008/diff/1/cc/resources/tile_priority.h#...
cc/resources/tile_priority.h:131: current_screen_quad ==
other.current_screen_quad;
The current screen quad is debug information.  You shouldn't need to compare it
here.  If it changes, then time to visible / distance to visible will also
change.

[nduca, 2013-03-20 17:07:51]

Should we be addressing the bigger issue of computing priority too often,
sometimes?

[Xianzhu, 2013-03-20 17:24:45]

On 2013/03/20 17:07:51, nduca wrote:
> Should we be addressing the bigger issue of computing priority too often,
> sometimes?

Yes, I think so. Would filing another bug for it work?

https://codereview.chromium.org/12912008/diff/1/cc/resources/tile.cc
File cc/resources/tile.cc (right):

https://codereview.chromium.org/12912008/diff/1/cc/resources/tile.cc#newcode49
cc/resources/tile.cc:49: if (priority_[tree] != priority) {
On 2013/03/20 17:05:57, enne wrote:
> style nit: early out return here instead.

Done.

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

https://codereview.chromium.org/12912008/diff/1/cc/resources/tile_priority.h#...
cc/resources/tile_priority.h:131: current_screen_quad ==
other.current_screen_quad;
On 2013/03/20 17:05:57, enne wrote:
> The current screen quad is debug information.  You shouldn't need to compare
it
> here.  If it changes, then time to visible / distance to visible will also
> change.

Done.

[nduca, 2013-03-20 17:26:13]

> Yes, I think so. Would filing another bug for it work?
Yeah totally. Was using this as a soapbox :)

[brianderson, 2013-03-20 18:11:25]

Does this completely fix 181708? Or is it still possible for animations to
prevent activation of a pending tree after this patch?

[Xianzhu, 2013-03-20 18:38:38]

On 2013/03/20 18:11:25, Brian Anderson wrote:
> Does this completely fix 181708? Or is it still possible for animations to
> prevent activation of a pending tree after this patch?

Based on my test, it fixes the ingress.com/intel issue. Haven't found other
cases triggering the issue.

[enne (OOO), 2013-03-20 19:59:57]

Oh, then this doesn't fix the entire bug.  If you manage tiles and nothing gets
scheduled, then I think you need to do the same notification that happens when a
visible tile gets completed.  It's possible that you could update priorities for
things that don't get gpu memory, and you would fall into the same bucket.

Can you add a unit test for these things?

[Xianzhu, 2013-03-20 20:16:23]

On 2013/03/20 19:59:57, enne wrote:
> Oh, then this doesn't fix the entire bug.  If you manage tiles and nothing
gets
> scheduled, then I think you need to do the same notification that happens when
a
> visible tile gets completed.  It's possible that you could update priorities
for
> things that don't get gpu memory, and you would fall into the same bucket.
> 

How about landing this simple change and keeping the bug open? The bug is Pri-1
only because of ingress.com/intel. I'd lower the priority of the bug after
ingress issue is fixed.

> Can you add a unit test for these things?

Sure.

[Xianzhu, 2013-03-20 20:38:16]

On 2013/03/20 19:59:57, enne wrote:
> Oh, then this doesn't fix the entire bug.  If you manage tiles and nothing
gets
> scheduled, then I think you need to do the same notification that happens when
a
> visible tile gets completed.
> 

Could you point me the source code about "the same notification that happens
when a visible tile gets completed"? I'm not familiar with it.

> It's possible that you could update priorities for
> things that don't get gpu memory, and you would fall into the same bucket.

Will this situation also block pending tree activation throughout the whole
period that the animation is active (that is, constantly there are tiles with
changed priority in each cycle)?

[enne (OOO), 2013-03-20 20:55:44]

On 2013/03/20 20:38:16, Xianzhu wrote:
> On 2013/03/20 19:59:57, enne wrote:
> > Oh, then this doesn't fix the entire bug.  If you manage tiles and nothing
> gets
> > scheduled, then I think you need to do the same notification that happens
when
> a
> > visible tile gets completed.
> > 
> 
> Could you point me the source code about "the same notification that happens
> when a visible tile gets completed"? I'm not familiar with it.

I was thinking of TileManager::DidFinishTileInitialization which eventually
SetsNeedsRedraw.

Can you explain what's causing the hang in more detail?  What condition is being
waited on that's not happening?  I had assumed it was this, but maybe I
misunderstand this bug.

[Xianzhu, 2013-03-20 21:16:21]

On 2013/03/20 20:55:44, enne wrote:
> 
> Can you explain what's causing the hang in more detail?  What condition is
being
> waited on that's not happening?  I had assumed it was this, but maybe I
> misunderstand this bug.

Added comment #23 in the bug.

[enne (OOO), 2013-03-20 23:28:34]

Even if there are more fixes to be had, I think this is worth landing,
regardless.

lgtm

[commit-bot: I haz the power, 2013-03-20 23:49:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12912008/7002

[commit-bot: I haz the power, 2013-03-21 02:49:45]

Retried try job too often on win_rel for step(s) browser_tests,
content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-21 03:11:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/12912008/7002

[commit-bot: I haz the power, 2013-03-21 07:10:34]

Change committed as 189543