Use a lock to deal with concurrent access to the m_evictedBackings

Use a mutex to deal with concurrent access to the m_evictedBackings
list, instead of passing the list of pointers around.

In ThreadProxy::beginFrame, we unlink all evicted textures, so that
the call to updateLayers will know to re-paint the evicted layers.

In ThreadProxy::scheduleActionCommit, we determine if we can draw now
by checking to see if there exist any linked evicted textures. If there
are none, we can draw what we are in the process of committing.

This assumes that once we get to m_layerTreeHost->willBeginFrame()
in ThreadProxy::beginFrame, the next frame to be commited by
ThreadProxy::scheduleActionCommit be at or after the frame that is
being produced by that call to ThreadProxy::beginFrame.

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

[ccameron, 2012-11-29 00:58:33]

This gets rid of passing lists of pointers to backings between the threads, and
rather just negotiates the issue with a lock. I find this much easier to verify.

[ccameron, 2012-11-29 18:22:24]

This is causing lots of crashes on TOT -- ptal.

[jamesr, 2012-11-29 22:06:16]

https://codereview.chromium.org/11411251/diff/1/cc/prioritized_resource_manag...
File cc/prioritized_resource_manager.h (right):

https://codereview.chromium.org/11411251/diff/1/cc/prioritized_resource_manag...
cc/prioritized_resource_manager.h:95: void unlinkAndClearEvictedBackings();
You should document that this can be called from multiple threads

https://codereview.chromium.org/11411251/diff/1/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11411251/diff/1/cc/thread_proxy.cc#newcode574
cc/thread_proxy.cc:574:
m_layerTreeHost->contentsTextureManager()->unlinkAndClearEvictedBackings();
we don't need to know about unlinked state until updateLayers() - the animation
/ layout calls definitely don't need to know about this (and they might take
some time).  could you move the call down?

[piman, 2012-11-29 22:24:09]

https://codereview.chromium.org/11411251/diff/1/cc/prioritized_resource_manag...
File cc/prioritized_resource_manager.h (right):

https://codereview.chromium.org/11411251/diff/1/cc/prioritized_resource_manag...
cc/prioritized_resource_manager.h:95: void unlinkAndClearEvictedBackings();
On 2012/11/29 22:06:16, jamesr wrote:
> You should document that this can be called from multiple threads

Wait, this should only be called from the main thread, right?

[ccameron, 2012-11-29 22:25:34]

Thanks!!

Yes -- it's main thread.

https://codereview.chromium.org/11411251/diff/1/cc/prioritized_resource_manag...
File cc/prioritized_resource_manager.h (right):

https://codereview.chromium.org/11411251/diff/1/cc/prioritized_resource_manag...
cc/prioritized_resource_manager.h:95: void unlinkAndClearEvictedBackings();
On 2012/11/29 22:06:16, jamesr wrote:
> You should document that this can be called from multiple threads

This one can only be called on the main thread -- I've added a comment to this
effect.

https://codereview.chromium.org/11411251/diff/1/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11411251/diff/1/cc/thread_proxy.cc#newcode574
cc/thread_proxy.cc:574:
m_layerTreeHost->contentsTextureManager()->unlinkAndClearEvictedBackings();
On 2012/11/29 22:06:16, jamesr wrote:
> we don't need to know about unlinked state until updateLayers() - the
animation
> / layout calls definitely don't need to know about this (and they might take
> some time).  could you move the call down?

Good point -- and I can get rid of the conditional by putting it after
initializeRenderer.

[piman, 2012-11-29 22:34:11]

LGTM+nits

https://codereview.chromium.org/11411251/diff/3003/cc/prioritized_resource_ma...
File cc/prioritized_resource_manager.cc (right):

https://codereview.chromium.org/11411251/diff/3003/cc/prioritized_resource_ma...
cc/prioritized_resource_manager.cc:350: bool
PrioritizedResourceManager::linkedEvictedBackingsExist()
nit: you can keep this const by making m_evictedBackingsLock mutable - it's a
common idiom I think.

https://codereview.chromium.org/11411251/diff/3003/cc/prioritized_resource_ma...
File cc/prioritized_resource_manager.h (right):

https://codereview.chromium.org/11411251/diff/3003/cc/prioritized_resource_ma...
cc/prioritized_resource_manager.h:197: base::Lock m_evictedBackingsLock;
small nit: in many chrome classes, we tend to declare the lock right before the
fields it protects. That way it looks like on the calling code where you grab an
autolock before accessing the fields and has a nice symmetry.

[ccameron, 2012-11-29 23:51:43]

https://codereview.chromium.org/11411251/diff/1/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/11411251/diff/1/cc/thread_proxy.cc#newcode574
cc/thread_proxy.cc:574:
m_layerTreeHost->contentsTextureManager()->unlinkAndClearEvictedBackings();
On 2012/11/29 22:06:16, jamesr wrote:
> we don't need to know about unlinked state until updateLayers() - the
animation
> / layout calls definitely don't need to know about this (and they might take
> some time).  could you move the call down?

This turns out to be very complicated -- there are tests
(LayerTreeHostTestEvictTextures.runMultiThread, in particular) which are built
around the assumption that the call to layout() will be called after the
textures are unlinked.

This can be easily fixed by adding a new hook to LayerTreeHostClient, but that
will involve touching a dozen files (all with different owners). Doing that in
this CL will add a lot of additional time, and make merging it into M24 much
more difficult.

[ccameron, 2012-11-30 00:02:17]

Thanks!!

https://codereview.chromium.org/11411251/diff/3003/cc/prioritized_resource_ma...
File cc/prioritized_resource_manager.cc (right):

https://codereview.chromium.org/11411251/diff/3003/cc/prioritized_resource_ma...
cc/prioritized_resource_manager.cc:350: bool
PrioritizedResourceManager::linkedEvictedBackingsExist()
On 2012/11/29 22:34:11, piman wrote:
> nit: you can keep this const by making m_evictedBackingsLock mutable - it's a
> common idiom I think.

Good idea. Done.

https://codereview.chromium.org/11411251/diff/3003/cc/prioritized_resource_ma...
File cc/prioritized_resource_manager.h (right):

https://codereview.chromium.org/11411251/diff/3003/cc/prioritized_resource_ma...
cc/prioritized_resource_manager.h:197: base::Lock m_evictedBackingsLock;
On 2012/11/29 22:34:11, piman wrote:
> small nit: in many chrome classes, we tend to declare the lock right before
the
> fields it protects. That way it looks like on the calling code where you grab
an
> autolock before accessing the fields and has a nice symmetry.

Good point. Done.

[commit-bot: I haz the power, 2012-11-30 01:07:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/11411251/8011

[jamesr, 2012-11-30 02:32:45]

On 2012/11/29 23:51:43, ccameron1 wrote:
> https://codereview.chromium.org/11411251/diff/1/cc/thread_proxy.cc
> File cc/thread_proxy.cc (right):
> 
> https://codereview.chromium.org/11411251/diff/1/cc/thread_proxy.cc#newcode574
> cc/thread_proxy.cc:574:
> m_layerTreeHost->contentsTextureManager()->unlinkAndClearEvictedBackings();
> On 2012/11/29 22:06:16, jamesr wrote:
> > we don't need to know about unlinked state until updateLayers() - the
> animation
> > / layout calls definitely don't need to know about this (and they might take
> > some time).  could you move the call down?
> 
> This turns out to be very complicated -- there are tests
> (LayerTreeHostTestEvictTextures.runMultiThread, in particular) which are built
> around the assumption that the call to layout() will be called after the
> textures are unlinked.
> 
> This can be easily fixed by adding a new hook to LayerTreeHostClient, but that
> will involve touching a dozen files (all with different owners). Doing that in
> this CL will add a lot of additional time, and make merging it into M24 much
> more difficult.

OK, but could you look at cleaning this up as a follow-up?  The animate and
layout steps can take quite some time, so we're using unnecessarily stale
values.

[commit-bot: I haz the power, 2012-11-30 05:21:16]

Change committed as 170403

[ccameron, 2012-11-30 09:52:28]

On 2012/11/30 02:32:45, jamesr wrote:
> OK, but could you look at cleaning this up as a follow-up?  The animate and
> layout steps can take quite some time, so we're using unnecessarily stale
> values.

What motivates this the most for me is that the test is "doing it wrong" -- the
tests should have pretend updateLayers() calls, and not just be hijacking
layout() for "stuff that happens vaguely near layout()".  That's a pretty
pervasive problem in the tests.

In terms of optimizations, I should be clear that this patch is a strict
improvement over the existing situation (where our snapshot came from the impl
thread quite some time ago before). The path is also slated for destruction with
impl-side painting, so I'd prefer not to invest much in its further
optimization, at least while there are more pressing issues.

The correctness issue of the tests, though, is concerning (and I do wonder what
the tests will look like when impl-side painting is added -- perhaps the looser
tests will be replaced).