Allow layers to signal that additional sequences are needed before surface destruction

Allow layers to signal that additional sequences are needed before surface destruction

A single surface can be referenced by multiple layers in separate compositors, so we need to be able to have every layer individually signal the manager that the Surface shouldn't be released until it's done with it.

When a SurfaceLayer's added to a LayerTreeHost, a new SurfaceSequence is created for that compositor and sent to the SurfaceManager for it to wait on before the Surface is destroyed. When the Surface is destroyed, that set of sequences is waited for. When the SurfaceLayer is removed from a compositor, a SwapPromise is created on that compositor that will satisfy its SurfaceSequence in the next frame (which won't reference the Surface).

BUG=411118
Committed: https://crrev.com/dbccae1ab081972ddee790058d817721d94d55dd
Cr-Commit-Position: refs/heads/master@{#303119}

[jbauman, 2014-10-22 02:21:35]

jbauman@chromium.org changed reviewers:
+ danakj@chromium.org, jamesr@chromium.org

[jbauman, 2014-10-28 17:38:46]

jamesr, ping

[jamesr, 2014-10-29 00:51:10]

everything but cc/layers/ lgtm.  I can't wrap my head around the cc/layers/
lifetime enough to understand it - is there a unit test that fails without those
changes and passes with it? maybe that'd make it easier to understand

https://codereview.chromium.org/666163006/diff/20001/cc/surfaces/surface_mana...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/666163006/diff/20001/cc/surfaces/surface_mana...
cc/surfaces/surface_manager.h:49: // A frame for a surface satisfies a set of
sequence numbers.
can you update the comment to clarify that the set of sequence numbers all come
from the specified id namespace? the decomposition of sequences into the two
pairs isn't completely obvious otherwise, imo

https://codereview.chromium.org/666163006/diff/20001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/666163006/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:132: next_surface_sequence_(1) {
these two are uints, so 0u / 1u

[jbauman, 2014-10-29 22:54:20]

On 2014/10/29 00:51:10, jamesr wrote:
> everything but cc/layers/ lgtm.  I can't wrap my head around the cc/layers/
> lifetime enough to understand it - is there a unit test that fails without
those
> changes and passes with it? maybe that'd make it easier to understand
> 
Ok, added RenderWidgetHostViewAuraTest.RecreateLayers to show where this sort of
thing is needed. Also surface_layer_unittests for some other test cases.

[jamesr, 2014-11-01 00:27:13]

Thanks, the tests are helpful. lgtm

[jbauman, 2014-11-03 21:50:59]

jbauman@chromium.org changed reviewers:
+ piman@chromium.org
- danakj@chromium.org

[jbauman, 2014-11-03 21:50:59]

Also, adding piman@ for OWNERS review of content/ and ui/compositor/

[piman, 2014-11-03 23:49:26]

Mostly nits.

https://codereview.chromium.org/666163006/diff/80001/cc/layers/surface_holder.h
File cc/layers/surface_holder.h (right):

https://codereview.chromium.org/666163006/diff/80001/cc/layers/surface_holder...
cc/layers/surface_holder.h:41: std::set<SurfaceSequence> sequences_;
nit: hash_set? Faster/less memory?
Or even, if you don't expect many of these, maybe just a simple vector?

https://codereview.chromium.org/666163006/diff/80001/cc/layers/surface_layer.cc
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/666163006/diff/80001/cc/layers/surface_layer....
cc/layers/surface_layer.cc:15: // be destroyed on either the impl or main
thread.
nit: Add ThreadChecker?

What's the plan for threaded compositing enabled (renderer)?

https://codereview.chromium.org/666163006/diff/80001/cc/layers/surface_layer.h
File cc/layers/surface_layer.h (right):

https://codereview.chromium.org/666163006/diff/80001/cc/layers/surface_layer....
cc/layers/surface_layer.h:22: void SetSurfaceId(scoped_refptr<SurfaceHolder>
surface_holder);
nit: SetSurfaceHolder?

https://codereview.chromium.org/666163006/diff/80001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/666163006/diff/80001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:405: surface_holder_ =
make_scoped_refptr(new cc::SurfaceHolder(
nit: no need for make_scoped_refptr

https://codereview.chromium.org/666163006/diff/80001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:407:
base::Bind(&SatisfyCallback, base::Unretained(manager))));
Why is Unretained safe? Can you add comments? In particular, surface_holder_ is
refcounted, so it's really hard to reason about lifetime (e.g. something posting
a task keeping the surface_holder_ alive for longer than you would expect).

[danakj, 2014-11-04 16:56:56]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2014-11-04 16:56:57]

https://codereview.chromium.org/666163006/diff/80001/cc/layers/surface_holder.h
File cc/layers/surface_holder.h (right):

https://codereview.chromium.org/666163006/diff/80001/cc/layers/surface_holder...
cc/layers/surface_holder.h:41: std::set<SurfaceSequence> sequences_;
On 2014/11/03 23:49:26, piman (Very slow to review) wrote:
> nit: hash_set? Faster/less memory?
> Or even, if you don't expect many of these, maybe just a simple vector?

hash_set may be slower on android (due to modulo operation, integer division
sucks there)? +1 to vector though for small sets.

[piman, 2014-11-04 21:11:34]

On Tue, Nov 4, 2014 at 8:56 AM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.org/666163006/diff/80001/cc/
> layers/surface_holder.h
> File cc/layers/surface_holder.h (right):
>
> https://codereview.chromium.org/666163006/diff/80001/cc/
> layers/surface_holder.h#newcode41
> cc/layers/surface_holder.h:41: std::set<SurfaceSequence> sequences_;
> On 2014/11/03 23:49:26, piman (Very slow to review) wrote:
>
>> nit: hash_set? Faster/less memory?
>> Or even, if you don't expect many of these, maybe just a simple
>>
> vector?
>
> hash_set may be slower on android (due to modulo operation, integer
> division sucks there)? +1 to vector though for small sets.
>

Is division really slower than tree walks?


> https://codereview.chromium.org/666163006/
>

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

[chromium-reviews, 2014-11-04 21:13:43]

On Tue, Nov 4, 2014 at 4:11 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Tue, Nov 4, 2014 at 8:56 AM, <danakj@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/666163006/diff/80001/cc/
>> layers/surface_holder.h
>> File cc/layers/surface_holder.h (right):
>>
>> https://codereview.chromium.org/666163006/diff/80001/cc/
>> layers/surface_holder.h#newcode41
>> cc/layers/surface_holder.h:41: std::set<SurfaceSequence> sequences_;
>> On 2014/11/03 23:49:26, piman (Very slow to review) wrote:
>>
>>> nit: hash_set? Faster/less memory?
>>> Or even, if you don't expect many of these, maybe just a simple
>>>
>> vector?
>>
>> hash_set may be slower on android (due to modulo operation, integer
>> division sucks there)? +1 to vector though for small sets.
>>
>
> Is division really slower than tree walks?
>

I think that it shows up as very costly in our perf stuff, for a small tree
I think it may be. I don't have numbers though. +aelias

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

[jbauman, 2014-11-04 23:56:47]

PTAL jamesr, piman. I completely changed this because I realized there were some
corner cases we'd have issues with in the other design.

[piman, 2014-11-05 00:36:16]

LGTM for ui/compositor/ and content/, some suggestions however (safety,
optimizations).

https://codereview.chromium.org/666163006/diff/130001/cc/layers/surface_layer.cc
File cc/layers/surface_layer.cc (right):

https://codereview.chromium.org/666163006/diff/130001/cc/layers/surface_layer...
cc/layers/surface_layer.cc:18: : sequence_(sequence),
satisfy_callback_(satisfy_callback) {}
need a virtual destructor.

https://codereview.chromium.org/666163006/diff/130001/cc/layers/surface_layer...
cc/layers/surface_layer.cc:21: void DidSwap(CompositorFrameMetadata* metadata)
override {
Can we add a ThreadChecker in constructor/destructor/DidNotSwap? Otherwise we
might call or destruct the callback on the wrong thread (both are bad).

https://codereview.chromium.org/666163006/diff/130001/cc/layers/surface_layer...
cc/layers/surface_layer.cc:31: SurfaceLayer::SatisfyCallback satisfy_callback_;
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/666163006/diff/130001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/666163006/diff/130001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:73: base::hash_set<SurfaceSequence>
destruction_dependencies_;
Could this be a vector? AFAICT the only things that write into the set is the
RequireCallback that is only called with a new sequence, and the erase (so
you're guaranteed to have unique entries, you could DCHECK it even). It's only
read sequentially too.

https://codereview.chromium.org/666163006/diff/130001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/666163006/diff/130001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:67: satisfied_sequences_.erase(*it);
drive-by: you're doing 2 tree walks (count, erase), you could look up using find
and erase by iterator.
also, hash_set :)

[jamesr, 2014-11-05 22:50:03]

https://codereview.chromium.org/666163006/diff/150001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/666163006/diff/150001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:59: std::vector<SurfaceSequence>&
destruction_dependencies() {
instead of exposing a ref to the container can you expose the operations you
want to happen? i.e. add, maybe iterators, etc.  that way you can modify the
container without having to modify all the callers and it's clearer what the set
of supported operations is

[jbauman, 2014-11-06 00:45:15]

On 2014/11/05 00:36:16, piman (Very slow to review) wrote:
> LGTM for ui/compositor/ and content/, some suggestions however (safety,
> optimizations).
> 
>
https://codereview.chromium.org/666163006/diff/130001/cc/layers/surface_layer.cc
> File cc/layers/surface_layer.cc (right):
> 
>
https://codereview.chromium.org/666163006/diff/130001/cc/layers/surface_layer...
> cc/layers/surface_layer.cc:18: : sequence_(sequence),
> satisfy_callback_(satisfy_callback) {}
> need a virtual destructor.

Done.
> 
>
https://codereview.chromium.org/666163006/diff/130001/cc/layers/surface_layer...
> cc/layers/surface_layer.cc:21: void DidSwap(CompositorFrameMetadata* metadata)
> override {
> Can we add a ThreadChecker in constructor/destructor/DidNotSwap? Otherwise we
> might call or destruct the callback on the wrong thread (both are bad).
> 
DidNotSwap could be done in any thread, so I think the callback should have to
deal with that. There's no fundamental reason why it couldn't.

>
https://codereview.chromium.org/666163006/diff/130001/cc/layers/surface_layer...
> cc/layers/surface_layer.cc:31: SurfaceLayer::SatisfyCallback
satisfy_callback_;
> nit: DISALLOW_COPY_AND_ASSIGN
> 
Done.

> https://codereview.chromium.org/666163006/diff/130001/cc/surfaces/surface.h
> File cc/surfaces/surface.h (right):
> 
>
https://codereview.chromium.org/666163006/diff/130001/cc/surfaces/surface.h#n...
> cc/surfaces/surface.h:73: base::hash_set<SurfaceSequence>
> destruction_dependencies_;
> Could this be a vector? AFAICT the only things that write into the set is the
> RequireCallback that is only called with a new sequence, and the erase (so
> you're guaranteed to have unique entries, you could DCHECK it even). It's only
> read sequentially too.

Ok, changed it to a vector, and added some methods to modify it so it isn't
exposed outside this class.
> 
>
https://codereview.chromium.org/666163006/diff/130001/cc/surfaces/surface_man...
> File cc/surfaces/surface_manager.cc (right):
> 
>
https://codereview.chromium.org/666163006/diff/130001/cc/surfaces/surface_man...
> cc/surfaces/surface_manager.cc:67: satisfied_sequences_.erase(*it);
> drive-by: you're doing 2 tree walks (count, erase), you could look up using
find
> and erase by iterator.
> also, hash_set :)

Ok, sure.

[jamesr, 2014-11-06 01:16:18]

cc/surfaces lgtm

https://codereview.chromium.org/666163006/diff/170001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/666163006/diff/170001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:19: SatisfyChecker(base::hash_set<SurfaceSequence>*
satisfied)
explicit

https://codereview.chromium.org/666163006/diff/170001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:142: SatisfyChecker(sequences)),
i think this would be a good use case for a lambda, if you're up for that

[jbauman, 2014-11-06 22:35:32]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-11-06 22:36:06]

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

[commit-bot: I haz the power, 2014-11-06 23:26:54]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2014-11-06 23:28:01]

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