Issue 1417053005: cc: Split ThreadProxy into ProxyMain and ProxyImpl

Khushal

Description was changed from ========== cc: Split ThreadProxy into ProxyMain and ProxyImpl This is the ...

5 years, 1 month ago (2015-11-04 00:50:40 UTC) #1

Khushal

khushalsagar@chromium.org changed reviewers: + dtrainor@chromium.org

5 years, 1 month ago (2015-11-04 00:50:41 UTC) #2

Wez

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test.cc#newcode117 cc/test/layer_tree_test.cc:117: // Adapts ProxyImpl for test. Injects test hooks for ...

5 years, 1 month ago (2015-11-10 01:41:51 UTC) #5

Khushal

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test.cc#newcode117 cc/test/layer_tree_test.cc:117: // Adapts ProxyImpl for test. Injects test hooks for ...

5 years, 1 month ago (2015-11-11 04:17:48 UTC) #6

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:117: // Adapts ProxyImpl for test. Injects test hooks
for testing.
On 2015/11/10 01:41:50, Wez wrote:
> nit: Suggest "Creates a ProxyImpl that notifies the supplied |test_hooks| of
> various actions."

Yeah, that makes it clearer. Done.

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:273: // Injects ProxyImplForTest for testing.
On 2015/11/10 01:41:50, Wez wrote:
> Suggest "ThreadedChannel that will notify |test_hooks| of internal ProxyImpl
> activity." or similar.

Done.

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:298: // Adapts ProxyMain for test. Injects test hooks
for testing.
On 2015/11/10 01:41:50, Wez wrote:
> See suggestions above.

Done.

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:397: // Adapts SingleThreadProxy for test. Injects
test hooks for testing.
On 2015/11/10 01:41:50, Wez wrote:
> See suggestions above re comment style.

Done.

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:278: // Used these only for ProxyMain tests in
threaded mode.
On 2015/11/10 01:41:50, Wez wrote:
> nit: Used -> Use?
> 
> If these are only for tests then can they be GetProxyFooForTest, so that
> pre-submit will check that they are only referenced in test code?

I don't think that's necessary. The file itself is only used in test code.
The LayerTreeTests can run in single-threaded or threaded mode. The comment is
to explain that these should only be used  in tests run for threaded mode since
only that mode uses ProxyMain and ProxyImpl. The implementation also has a
DCHECK to ensure that.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/channel_main.h
File cc/trees/channel_main.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/channel_main.h...
cc/trees/channel_main.h:60: virtual bool IsInitialized() const = 0;
On 2015/11/10 01:41:50, Wez wrote:
> nit: Suggest separating the three methods with blank lines, otherwise it looks
> like "Must be called before deleting the channel" may also apply to
> IsInitialized().

Done.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h#n...
cc/trees/proxy_impl.h:26: static scoped_ptr<ProxyImpl> Create(
On 2015/11/10 01:41:50, Wez wrote:
> Clarify ownership/lifetime requirements on the objects passed as bare
pointers?

Done.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h#n...
cc/trees/proxy_impl.h:34: // Accessed on the impl thread when the main thread is
blocked for a commit.
On 2015/11/10 01:41:51, Wez wrote:
> This comment is on a struct, not a member; it's not clear how the comment
> applies to it, nor how the struct itself is intended to be used?

I'm trying to follow a similar pattern to what we had with ThreadProxy's
MainThreadOnly and CompositorThreadOnly structs.

Modified the comment to make its purpose clearer. The class which uses the
struct ensures that it follows the guidelines of this comment by using an
accessor to limit access to the struct only when the main thread is blocked for
a commit.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h#n...
cc/trees/proxy_impl.h:54: // Please call these 3 functions through
On 2015/11/10 01:41:51, Wez wrote:
> nit: It's OK to mark the implementation of an interface that a class
implements,
> but which isn't really intended to be public, as private or protected (even
> though the class inheritance must be public), to effectively document that
> callers shouldn't be using it - would that be appropriate here?

I think that might be suitable for both the LayerTreeHostImplClient and
SchedulerClient implementation, since neither of them are intended to be public.
For now, I've kept this consistent with the original implementation in
ThreadProxy. I'd like it if someone from the cc team could also review it.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h#n...
cc/trees/proxy_impl.h:74: // This should only be called by
LayerTreeHostImpl::PostFrameTimingEvents.
On 2015/11/10 01:41:51, Wez wrote:
> nit: Is that restriction specific to this implementation? If not then document
> it on the interface?

Kept this consistent with the original implementation in ThreadProxy. I am not
sure about this.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h#n...
cc/trees/proxy_impl.h:110: // Callback for impl side commands received from the
channel.
On 2015/11/10 01:41:50, Wez wrote:
> nit: Callbacks?

Done.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h#n...
cc/trees/proxy_impl.h:133: // Must be called before deleting ProxyImpl.
On 2015/11/10 01:41:51, Wez wrote:
> By whom, given that this is private?

This was for the channel classes that will be added as friend classes to
ProxyMain and ProxyImpl.

The function existed in ThreadProxy to destroy the state for the impl side on
the impl thread. Since ProxyImpl represents this state, I think its suitable to
do this in the dtor itself.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h#n...
cc/trees/proxy_impl.h:145: scoped_ptr<Scheduler> scheduler_;
On 2015/11/10 01:41:51, Wez wrote:
> nit: There doesn't seem to be any obvious logic to the order of the members -
in
> the absence of an obvious set of logically-related sets of members, one
helpful
> convention is to put the construction-time parameters together, first, and
then
> the internal bits come after those.

I've kept this consistent with the implementation of the CompositorThreadOnly
struct from ThreadProxy since that struct represented the data members for this
class.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h#n...
cc/trees/proxy_impl.h:187: base::WeakPtr<ProxyImpl> impl_thread_weak_ptr_;
On 2015/11/10 01:41:51, Wez wrote:
> nit: Is this actually a WeakPtr to a thread..?

In the thread proxy implementation, we had 2 pointers and the thread suggested
the task runner each was bound to.
Since now ProxyMain and ProxyImpl live entirely on the main or impl thread, I
guess it doesn't make sense to maintain a pointer for this. Removed it.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.cc
File cc/trees/proxy_main.cc (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.cc#...
cc/trees/proxy_main.cc:80: DCHECK(Proxy::IsMainThread());
On 2015/11/10 01:41:51, Wez wrote:
> nit: Why are some of these just IsMainThread, and some Proxy::IsMainThread?

ThreadProxy implementation had that. Fixed.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.h
File cc/trees/proxy_main.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.h#n...
cc/trees/proxy_main.h:124: base::WeakPtr<ProxyMain> GetWeakPtr();
On 2015/11/10 01:41:51, Wez wrote:
> Why do you have this _and_ the |main_thread_weak_ptr_| member, below?

main_thread_weak_ptr_ is not needed anymore.
This has been put as a function so the channel classes can access it.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.h#n...
cc/trees/proxy_main.h:154: base::WeakPtr<ProxyMain> main_thread_weak_ptr_;
On 2015/11/10 01:41:51, Wez wrote:
> You appear to set this but never use it?
> 
> Also, why is it called "main_thread_weak_ptr_"..?

Again, thread proxy had this as main_thread_weak_ptr_ to denote the thread the
pointer was bound to. We can use the factory to dispense pointers directly since
ProxyMain lives only on the main thread.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
File cc/trees/threaded_channel.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
cc/trees/threaded_channel.h:68: // TODO(khushalsagar): Make this ProxyMain* and
write the initialization
On 2015/11/10 01:41:51, Wez wrote:
> nit: Looks like it _is_ now ProxyMain* ?

Done.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
cc/trees/threaded_channel.h:99: 
On 2015/11/10 01:41:51, Wez wrote:
> nit: Why is there a blank line here? Are these part of the ChannelMain
> implementation, ChannelImpl, or something else?

These are all ChannelMain implementation. Fixed.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
cc/trees/threaded_channel.h:151: scoped_ptr<ProxyImpl> proxy_impl_;
On 2015/11/10 01:41:51, Wez wrote:
> This doesn't look right; you're creating ThreadChannel on the main thread, I
> think, and then ProxyImpl on that thread as well, then posting tasks to call
> ProxyImpl on the impl thread, but then letting it get deleted back on the
> ThreadChannel thread, i.e. on the main thread?
> 
> For this to work correctly you'll need to post a task to the impl thread to
> delete the ProxyImpl on it, otherwise you may delete it on the main thread
while
> a task is processing a task on it on the impl thread - WeakPtr has DCHECKs
> against that, so I'm surprised this works.
> 
> I'd also recommend storing the WeakPtr to the impl when you create the
> ProxyImpl, to make the code a little more robust to any future refactoring
that
> might affect its lifetime.

ProxyImpl is created and destroyed only on the impl thread. ProxyImpl itself has
DCHECKS to ensure that. We post tasks to the impl thread for both these
operations. Added comments for where that is being done.

Regarding the weak ptr, I'm not sure I completely understood that. Do you mean
we should store only the weak ptr for ProxyImpl here?

The class which implements ChannelImpl has to take ownership for ProxyImpl. In
this case it is the ThreadedChannel. In case of a network channel, the
ChannelImpl implementation which lives on the client will create and control the
lifetime of ProxyImpl.

https://codereview.chromium.org/1417053005/diff/100001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.cc (right):

https://codereview.chromium.org/1417053005/diff/100001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:105: void
ThreadedChannel::InitializeImplOnImplThread(
Creates ProxyImpl on the impl thread.

https://codereview.chromium.org/1417053005/diff/100001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:131: void
ThreadedChannel::CloseImplOnImplThread(CompletionEvent* completion) {
Destroys ProxyImpl on the impl thread.

Khushal

khushalsagar@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org, vmpstr@chromium.org

5 years, 1 month ago (2015-11-12 17:24:11 UTC) #7

Wez

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test.h File cc/test/layer_tree_test.h (right): https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test.h#newcode278 cc/test/layer_tree_test.h:278: // Used these only for ProxyMain tests in threaded ...

5 years, 1 month ago (2015-11-12 22:30:18 UTC) #8

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:278: // Used these only for ProxyMain tests in
threaded mode.
On 2015/11/11 at 04:17:47, Khushal wrote:
> On 2015/11/10 01:41:50, Wez wrote:
> > nit: Used -> Use?
> > 
> > If these are only for tests then can they be GetProxyFooForTest, so that
> > pre-submit will check that they are only referenced in test code?
> 
> I don't think that's necessary. The file itself is only used in test code.
> The LayerTreeTests can run in single-threaded or threaded mode. The comment is
to explain that these should only be used  in tests run for threaded mode since
only that mode uses ProxyMain and ProxyImpl. The implementation also has a
DCHECK to ensure that.

OK, acknowledged.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_impl.h#n...
cc/trees/proxy_impl.h:54: // Please call these 3 functions through
On 2015/11/11 at 04:17:47, Khushal wrote:
> On 2015/11/10 01:41:51, Wez wrote:
> > nit: It's OK to mark the implementation of an interface that a class
implements,
> > but which isn't really intended to be public, as private or protected (even
> > though the class inheritance must be public), to effectively document that
> > callers shouldn't be using it - would that be appropriate here?
> 
> I think that might be suitable for both the LayerTreeHostImplClient and
SchedulerClient implementation, since neither of them are intended to be public.
> For now, I've kept this consistent with the original implementation in
ThreadProxy. I'd like it if someone from the cc team could also review it.

SGTM

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.h
File cc/trees/proxy_main.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.h#n...
cc/trees/proxy_main.h:154: base::WeakPtr<ProxyMain> main_thread_weak_ptr_;
On 2015/11/11 at 04:17:47, Khushal wrote:
> On 2015/11/10 01:41:51, Wez wrote:
> > You appear to set this but never use it?
> > 
> > Also, why is it called "main_thread_weak_ptr_"..?
> 
> Again, thread proxy had this as main_thread_weak_ptr_ to denote the thread the
pointer was bound to. We can use the factory to dispense pointers directly since
ProxyMain lives only on the main thread.

I'm not sure what you mean by "the thread the pointer was bound to" - do you
mean the thread that the WeakPtr must be used on, and will be invalidated on?
I'm not sure why that would need codifying in the member name - we don't do that
for other members of the class. Plus, which thread the WeakPtr must be used on
is really a function of which thread the WeakPtrFactory will be destroyed or
invalidated on.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
File cc/trees/threaded_channel.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
cc/trees/threaded_channel.h:151: scoped_ptr<ProxyImpl> proxy_impl_;
On 2015/11/11 at 04:17:47, Khushal wrote:
> On 2015/11/10 01:41:51, Wez wrote:
> > This doesn't look right; you're creating ThreadChannel on the main thread, I
> > think, and then ProxyImpl on that thread as well, then posting tasks to call
> > ProxyImpl on the impl thread, but then letting it get deleted back on the
> > ThreadChannel thread, i.e. on the main thread?
> > 
> > For this to work correctly you'll need to post a task to the impl thread to
> > delete the ProxyImpl on it, otherwise you may delete it on the main thread
while
> > a task is processing a task on it on the impl thread - WeakPtr has DCHECKs
> > against that, so I'm surprised this works.
> > 
> > I'd also recommend storing the WeakPtr to the impl when you create the
> > ProxyImpl, to make the code a little more robust to any future refactoring
that
> > might affect its lifetime.
> 
> ProxyImpl is created and destroyed only on the impl thread. ProxyImpl itself
has DCHECKS to ensure that. We post tasks to the impl thread for both these
operations. Added comments for where that is being done.

Sorry, I missed the proxy_impl_.reset() in the CloseOnImplThread().

So, I think a DCHECK(!proxy_impl_) would make sense in ~ThreadedChannel, since
having had CloseOnImplThread() called on the impl thread before the dtor runs on
the main thread is a pre-requisite for correct teardown.

> Regarding the weak ptr, I'm not sure I completely understood that. Do you mean
we should store only the weak ptr for ProxyImpl here?

Not quite; my point was mainly about avoiding calling
ProxyImpl::weak_ptr_factory_::GetWeakPtr() from multiple threads, since
referring to the factory, which lives on thread A, from thread B inherently
risks accessing it after it's been deleted, which rather defeats the object -
taking and storing a WeakPtr on thread B to use to post tasks is safer.

However, looking through the ProxyImpl & ThreadedChannel:
- ProxyImpl can dispense WeakPtrs, but it doesn't actually use them itself.
- Only the ThreadedChannel seems to use ProxyImpl WeakPtrs, to auto-orphan
pending tasks on the ProxyImpl when it gets torn down.
- But the ThreadedChannel is actually the owner of the ProxyImpl.

So you should be able to simplify all this:
1. Move the WeakPtrFactory<ProxyImpl> from the ProxyImpl itself out into
ThreadedChannel.
2. When the ProxyImpl is created, create a WeakPtrFactory<ProxyImpl> to wrap it,
and use that in your PostTask() binds.
3. When the ProxyImpl is deleted, delete the WeakPtrFactory to it immediately
beforehand.

This avoids polluting the ProxyImpl with WeakPtr-dispensing stuff, and keeps it
all in the class that actually uses it.

> The class which implements ChannelImpl has to take ownership for ProxyImpl. In
this case it is the ThreadedChannel. In case of a network channel, the
ChannelImpl implementation which lives on the client will create and control the
lifetime of ProxyImpl.

SGTM

Khushal

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.h File cc/trees/proxy_main.h (right): https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.h#newcode154 cc/trees/proxy_main.h:154: base::WeakPtr<ProxyMain> main_thread_weak_ptr_; On 2015/11/12 22:30:18, Wez wrote: > On ...

5 years, 1 month ago (2015-11-13 04:02:17 UTC) #9

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.h
File cc/trees/proxy_main.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/proxy_main.h#n...
cc/trees/proxy_main.h:154: base::WeakPtr<ProxyMain> main_thread_weak_ptr_;
On 2015/11/12 22:30:18, Wez wrote:
> On 2015/11/11 at 04:17:47, Khushal wrote:
> > On 2015/11/10 01:41:51, Wez wrote:
> > > You appear to set this but never use it?
> > > 
> > > Also, why is it called "main_thread_weak_ptr_"..?
> > 
> > Again, thread proxy had this as main_thread_weak_ptr_ to denote the thread
the
> pointer was bound to. We can use the factory to dispense pointers directly
since
> ProxyMain lives only on the main thread.
> 
> I'm not sure what you mean by "the thread the pointer was bound to" - do you
> mean the thread that the WeakPtr must be used on, and will be invalidated on?
> I'm not sure why that would need codifying in the member name - we don't do
that
> for other members of the class. Plus, which thread the WeakPtr must be used on
> is really a function of which thread the WeakPtrFactory will be destroyed or
> invalidated on.

Yeah. It was meant to denote the thread the WeakPtr must be de-referenced on. We
needed that with ThreadProxy since we maintained 2 factorys, living on the main
and impl thread and the name referred the factory the pointer was created from
and the thread it should be used on.

I had missed removing it here earlier. We don't need to do this anymore since
ProxyMain lives only on the main thread.

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
File cc/trees/threaded_channel.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
cc/trees/threaded_channel.h:151: scoped_ptr<ProxyImpl> proxy_impl_;
On 2015/11/12 22:30:18, Wez wrote:
> On 2015/11/11 at 04:17:47, Khushal wrote:
> > On 2015/11/10 01:41:51, Wez wrote:
> > > This doesn't look right; you're creating ThreadChannel on the main thread,
I
> > > think, and then ProxyImpl on that thread as well, then posting tasks to
call
> > > ProxyImpl on the impl thread, but then letting it get deleted back on the
> > > ThreadChannel thread, i.e. on the main thread?
> > > 
> > > For this to work correctly you'll need to post a task to the impl thread
to
> > > delete the ProxyImpl on it, otherwise you may delete it on the main thread
> while
> > > a task is processing a task on it on the impl thread - WeakPtr has DCHECKs
> > > against that, so I'm surprised this works.
> > > 
> > > I'd also recommend storing the WeakPtr to the impl when you create the
> > > ProxyImpl, to make the code a little more robust to any future refactoring
> that
> > > might affect its lifetime.
> > 
> > ProxyImpl is created and destroyed only on the impl thread. ProxyImpl itself
> has DCHECKS to ensure that. We post tasks to the impl thread for both these
> operations. Added comments for where that is being done.
> 
> Sorry, I missed the proxy_impl_.reset() in the CloseOnImplThread().
> 
> So, I think a DCHECK(!proxy_impl_) would make sense in ~ThreadedChannel, since
> having had CloseOnImplThread() called on the impl thread before the dtor runs
on
> the main thread is a pre-requisite for correct teardown.
> 

Yeah, Sorry I missed that. It should be there. It seemed like a bad idea to have
IsInitialized access proxy_impl_ from the main thread. While it only checks if
ProxyImpl exists, and the initialization and destruction of proxy impl are
blocking operations, I figured its better to restrict access to variables to the
thread they belong to. Added a variable on the main thread which makes sure that
the channel is not initialized for correct teardown.

> > Regarding the weak ptr, I'm not sure I completely understood that. Do you
mean
> we should store only the weak ptr for ProxyImpl here?
> 
> Not quite; my point was mainly about avoiding calling
> ProxyImpl::weak_ptr_factory_::GetWeakPtr() from multiple threads, since
> referring to the factory, which lives on thread A, from thread B inherently
> risks accessing it after it's been deleted, which rather defeats the object -
> taking and storing a WeakPtr on thread B to use to post tasks is safer.

Oh. I actually kept the factory in ProxyImpl since I assumed if ProxyImpl owns
the factory, it lowers the risk of accessing it after its been deleted since the
lifetime of the factory and its pointers is tied to the lifetime of and
controlled by ProxyImpl itself. It explicitly invalidates all the weak pointers
in its dtor.

I think you're right that it would be safer if we avoided calling
weak_factory::GetWeakPtr() from multiple threads. I've added a DCHECK to make
sure that the caller asks for a weak pointer on the correct thread and we save
it in the channel. Is that better?

> 
> However, looking through the ProxyImpl & ThreadedChannel:
> - ProxyImpl can dispense WeakPtrs, but it doesn't actually use them itself.
> - Only the ThreadedChannel seems to use ProxyImpl WeakPtrs, to auto-orphan
> pending tasks on the ProxyImpl when it gets torn down.
> - But the ThreadedChannel is actually the owner of the ProxyImpl.
> 
> So you should be able to simplify all this:
> 1. Move the WeakPtrFactory<ProxyImpl> from the ProxyImpl itself out into
> ThreadedChannel.
> 2. When the ProxyImpl is created, create a WeakPtrFactory<ProxyImpl> to wrap
it,
> and use that in your PostTask() binds.
> 3. When the ProxyImpl is deleted, delete the WeakPtrFactory to it immediately
> beforehand.
> 
> This avoids polluting the ProxyImpl with WeakPtr-dispensing stuff, and keeps
it
> all in the class that actually uses it.
> 

I was a bit unsure about moving the weak factory for ProxyImpl to
ThreadedChannel since the pattern for using weakptrfactory is to always have the
class be composed of a factory for itself. I guess this is to make sure that the
factory never outlives the object and the class has control over how it exposes
weak ptrs to itself.

We could also have ProxyImpl extend SupportsWeakPtr, but I'm not sure if that
would be better. Wdyt?

> > The class which implements ChannelImpl has to take ownership for ProxyImpl.
In
> this case it is the ThreadedChannel. In case of a network channel, the
> ChannelImpl implementation which lives on the client will create and control
the
> lifetime of ProxyImpl.
> 
> SGTM

vmpstr

vmpstr@chromium.org changed reviewers: + brianderson@chromium.org

5 years, 1 month ago (2015-11-13 19:24:19 UTC) #10

vmpstr

This looks pretty good! I think the split is awesome. +brianderson as well. https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_instrumentation.h File ...

5 years, 1 month ago (2015-11-13 19:24:19 UTC) #11

Khushal

https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_instrumentation.h File cc/debug/benchmark_instrumentation.h (right): https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_instrumentation.h#newcode23 cc/debug/benchmark_instrumentation.h:23: const char kSendBeginFrame[] = "ProxyImpl::ScheduledActionSendBeginMainFrame"; On 2015/11/13 19:24:19, vmpstr ...

5 years, 1 month ago (2015-11-13 22:10:30 UTC) #12

vmpstr

On 2015/11/13 22:10:30, Khushal wrote: > https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_instrumentation.h > File cc/debug/benchmark_instrumentation.h (right): > > https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_instrumentation.h#newcode23 > ...

5 years, 1 month ago (2015-11-16 21:37:40 UTC) #13

On 2015/11/13 22:10:30, Khushal wrote:
>
https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_ins...
> File cc/debug/benchmark_instrumentation.h (right):
> 
>
https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_ins...
> cc/debug/benchmark_instrumentation.h:23: const char kSendBeginFrame[] =
> "ProxyImpl::ScheduledActionSendBeginMainFrame";
> On 2015/11/13 19:24:19, vmpstr wrote:
> > The reason we had this class initially is because there are systems outside
of
> > chrome that rely on the name of these being what they are. See the comment
on
> > line 14-16. I don't know how much of this is still true, but I think it's
> worth
> > investigating before changing.
> > 
> 
> Sorry I missed this earlier. I think this is still being used for telemetry
perf
> tests. I've updated the name there as well.
> 
> > Also, if nothing relies on this, then we should start transitioning away
from
> > using this class (this is unrelated, but you could file a bug and assign it
to
> > me if nothing uses this).
> 
> https://codereview.chromium.org/1417053005/diff/140001/cc/trees/proxy_impl.h
> File cc/trees/proxy_impl.h (right):
> 
>
https://codereview.chromium.org/1417053005/diff/140001/cc/trees/proxy_impl.h#...
> cc/trees/proxy_impl.h:184: ChannelImpl* channel_impl_;
> On 2015/11/13 19:24:19, vmpstr wrote:
> > I think maybe now is the time to start thinking of what to name this
variable
> to
> > reflect what it represents, not just what type it is :) I think
> > channel_to_the_other_side is a bit ominous; do you have any other
suggestions?
> 
> If we want to change only the variable name, then we could just rename the
> variables, channel_ in ProxyMain and ProxyImpl? Each proxy has a channel to
talk
> to the other side.
> Do the current names for the classes, ProxyMain, ProxyImpl, ChannelMain and
> ChannelImpl seem apt?

Yeah the class names are fine I think. We can keep variables distinct for now.
It's an easy change later if we decide this is too verbose or something.

vmpstr

On 2015/11/16 21:37:40, vmpstr wrote: > On 2015/11/13 22:10:30, Khushal wrote: > > > https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_instrumentation.h ...

5 years, 1 month ago (2015-11-16 21:45:53 UTC) #14

On 2015/11/16 21:37:40, vmpstr wrote:
> On 2015/11/13 22:10:30, Khushal wrote:
> >
>
https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_ins...
> > File cc/debug/benchmark_instrumentation.h (right):
> > 
> >
>
https://codereview.chromium.org/1417053005/diff/140001/cc/debug/benchmark_ins...
> > cc/debug/benchmark_instrumentation.h:23: const char kSendBeginFrame[] =
> > "ProxyImpl::ScheduledActionSendBeginMainFrame";
> > On 2015/11/13 19:24:19, vmpstr wrote:
> > > The reason we had this class initially is because there are systems
outside
> of
> > > chrome that rely on the name of these being what they are. See the comment
> on
> > > line 14-16. I don't know how much of this is still true, but I think it's
> > worth
> > > investigating before changing.
> > > 
> > 
> > Sorry I missed this earlier. I think this is still being used for telemetry
> perf
> > tests. I've updated the name there as well.
> > 
> > > Also, if nothing relies on this, then we should start transitioning away
> from
> > > using this class (this is unrelated, but you could file a bug and assign
it
> to
> > > me if nothing uses this).
> > 
> > https://codereview.chromium.org/1417053005/diff/140001/cc/trees/proxy_impl.h
> > File cc/trees/proxy_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/1417053005/diff/140001/cc/trees/proxy_impl.h#...
> > cc/trees/proxy_impl.h:184: ChannelImpl* channel_impl_;
> > On 2015/11/13 19:24:19, vmpstr wrote:
> > > I think maybe now is the time to start thinking of what to name this
> variable
> > to
> > > reflect what it represents, not just what type it is :) I think
> > > channel_to_the_other_side is a bit ominous; do you have any other
> suggestions?
> > 
> > If we want to change only the variable name, then we could just rename the
> > variables, channel_ in ProxyMain and ProxyImpl? Each proxy has a channel to
> talk
> > to the other side.
> > Do the current names for the classes, ProxyMain, ProxyImpl, ChannelMain and
> > ChannelImpl seem apt?
> 
> Yeah the class names are fine I think. We can keep variables distinct for now.
> It's an easy change later if we decide this is too verbose or something.

Err, let me read the (email) thread first before I reply :)

Khushal

https://codereview.chromium.org/1417053005/diff/200001/cc/trees/threaded_channel.cc File cc/trees/threaded_channel.cc (right): https://codereview.chromium.org/1417053005/diff/200001/cc/trees/threaded_channel.cc#newcode131 cc/trees/threaded_channel.cc:131: main().initialized = true; Its better to have the channel ...

5 years, 1 month ago (2015-11-17 19:03:51 UTC) #15

Wez

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_channel.h File cc/trees/threaded_channel.h (right): https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_channel.h#newcode151 cc/trees/threaded_channel.h:151: scoped_ptr<ProxyImpl> proxy_impl_; On 2015/11/13 at 04:02:17, Khushal wrote: > ...

5 years, 1 month ago (2015-11-18 01:50:35 UTC) #16

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
File cc/trees/threaded_channel.h (right):

https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
cc/trees/threaded_channel.h:151: scoped_ptr<ProxyImpl> proxy_impl_;
On 2015/11/13 at 04:02:17, Khushal wrote:
> On 2015/11/12 22:30:18, Wez wrote:
> > On 2015/11/11 at 04:17:47, Khushal wrote:
> > > On 2015/11/10 01:41:51, Wez wrote:
> > > > This doesn't look right; you're creating ThreadChannel on the main
thread, I
> > > > think, and then ProxyImpl on that thread as well, then posting tasks to
call
> > > > ProxyImpl on the impl thread, but then letting it get deleted back on
the
> > > > ThreadChannel thread, i.e. on the main thread?
> > > > 
> > > > For this to work correctly you'll need to post a task to the impl thread
to
> > > > delete the ProxyImpl on it, otherwise you may delete it on the main
thread
> > while
> > > > a task is processing a task on it on the impl thread - WeakPtr has
DCHECKs
> > > > against that, so I'm surprised this works.
> > > > 
> > > > I'd also recommend storing the WeakPtr to the impl when you create the
> > > > ProxyImpl, to make the code a little more robust to any future
refactoring
> > that
> > > > might affect its lifetime.
> > > 
> > > ProxyImpl is created and destroyed only on the impl thread. ProxyImpl
itself
> > has DCHECKS to ensure that. We post tasks to the impl thread for both these
> > operations. Added comments for where that is being done.
> > 
> > Sorry, I missed the proxy_impl_.reset() in the CloseOnImplThread().
> > 
> > So, I think a DCHECK(!proxy_impl_) would make sense in ~ThreadedChannel,
since
> > having had CloseOnImplThread() called on the impl thread before the dtor
runs on
> > the main thread is a pre-requisite for correct teardown.
> > 
> 
> Yeah, Sorry I missed that. It should be there. It seemed like a bad idea to
have IsInitialized access proxy_impl_ from the main thread. While it only checks
if ProxyImpl exists, and the initialization and destruction of proxy impl are
blocking operations, I figured its better to restrict access to variables to the
thread they belong to. Added a variable on the main thread which makes sure that
the channel is not initialized for correct teardown.

Not quite - it's absolutely fine to check whether |proxy_impl_| is set, from
code on the main thread, _so long as that is the thread that controls the
lifetime of the ProxyImpl object_, which is the case here - the ThreadedChannel
owns the ProxyImpl and controls its lifetime, so so long as ThreadedChannel
correctly Pass()es or release()s it when it's time to post to the impl thread to
delete it, everything should be fine - the only thing you wouldn't want to do is
actually call into the ProxyImpl from the main thread, since it's not designed
to be called from multiple threads.

> 
> > > Regarding the weak ptr, I'm not sure I completely understood that. Do you
mean
> > we should store only the weak ptr for ProxyImpl here?
> > 
> > Not quite; my point was mainly about avoiding calling
> > ProxyImpl::weak_ptr_factory_::GetWeakPtr() from multiple threads, since
> > referring to the factory, which lives on thread A, from thread B inherently
> > risks accessing it after it's been deleted, which rather defeats the object
-
> > taking and storing a WeakPtr on thread B to use to post tasks is safer.
> 
> Oh. I actually kept the factory in ProxyImpl since I assumed if ProxyImpl owns
the factory, it lowers the risk of accessing it after its been deleted since the
lifetime of the factory and its pointers is tied to the lifetime of and
controlled by ProxyImpl itself. It explicitly invalidates all the weak pointers
in its dtor.
> 
> I think you're right that it would be safer if we avoided calling
weak_factory::GetWeakPtr() from multiple threads. I've added a DCHECK to make
sure that the caller asks for a weak pointer on the correct thread and we save
it in the channel. Is that better?
> 
> > 
> > However, looking through the ProxyImpl & ThreadedChannel:
> > - ProxyImpl can dispense WeakPtrs, but it doesn't actually use them itself.
> > - Only the ThreadedChannel seems to use ProxyImpl WeakPtrs, to auto-orphan
> > pending tasks on the ProxyImpl when it gets torn down.
> > - But the ThreadedChannel is actually the owner of the ProxyImpl.
> > 
> > So you should be able to simplify all this:
> > 1. Move the WeakPtrFactory<ProxyImpl> from the ProxyImpl itself out into
> > ThreadedChannel.
> > 2. When the ProxyImpl is created, create a WeakPtrFactory<ProxyImpl> to wrap
it,
> > and use that in your PostTask() binds.
> > 3. When the ProxyImpl is deleted, delete the WeakPtrFactory to it
immediately
> > beforehand.
> > 
> > This avoids polluting the ProxyImpl with WeakPtr-dispensing stuff, and keeps
it
> > all in the class that actually uses it.
> > 
> 
> I was a bit unsure about moving the weak factory for ProxyImpl to
ThreadedChannel since the pattern for using weakptrfactory is to always have the
class be composed of a factory for itself. I guess this is to make sure that the
factory never outlives the object and the class has control over how it exposes
weak ptrs to itself.
> 
> We could also have ProxyImpl extend SupportsWeakPtr, but I'm not sure if that
would be better. Wdyt?

SupportsWeakPtr should ideally go away! The problem is that, unlike
WeakPtrFactory, the dtor only runs _after_ the derived class' members have been
torn down - so some part of the teardown causes a WeakPtr to the object to be
dereferenced, it will still dereference even while the object is half gone -
normally not an issue, but also no fun to have to diagnose... The other problem
is simply that it tempts callers to think they can take WeakPtrs, which means
they may create implicit threading dependencies without meaning to.

Having WeakPtrFactory inside the ProxyImpl benefits from implicitly invalidating
WeakPtrs on deletion, but it also means polluting ProxyImpl with GetWeakPtr()
even though it's really a ThreadedChannel implementation detail. There's a
similar risk of callers (ab)using that in confusing ways as with
SupportsWeakPtr.

(This is a case where it'd actually be ideal to have a scoped_ptr<>
specialization that embeds a WeakPtrFactory, to benefit from automatic
invalidation without requiring objects themselves be polluted.)

> 
> > > The class which implements ChannelImpl has to take ownership for
ProxyImpl. In
> > this case it is the ThreadedChannel. In case of a network channel, the
> > ChannelImpl implementation which lives on the client will create and control
the
> > lifetime of ProxyImpl.
> > 
> > SGTM

danakj

On Tue, Nov 17, 2015 at 5:50 PM, <wez@chromium.org> wrote: > > > https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_channel.h > ...

5 years, 1 month ago (2015-11-18 19:26:28 UTC) #17

On Tue, Nov 17, 2015 at 5:50 PM, <wez@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
> File cc/trees/threaded_channel.h (right):
>
>
>
https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
> cc/trees/threaded_channel.h:151: scoped_ptr<ProxyImpl> proxy_impl_;
> On 2015/11/13 at 04:02:17, Khushal wrote:
>
>> On 2015/11/12 22:30:18, Wez wrote:
>> > On 2015/11/11 at 04:17:47, Khushal wrote:
>> > > On 2015/11/10 01:41:51, Wez wrote:
>> > > > This doesn't look right; you're creating ThreadChannel on the
>>
> main thread, I
>
>> > > > think, and then ProxyImpl on that thread as well, then posting
>>
> tasks to call
>
>> > > > ProxyImpl on the impl thread, but then letting it get deleted
>>
> back on the
>
>> > > > ThreadChannel thread, i.e. on the main thread?
>> > > >
>> > > > For this to work correctly you'll need to post a task to the
>>
> impl thread to
>
>> > > > delete the ProxyImpl on it, otherwise you may delete it on the
>>
> main thread
>
>> > while
>> > > > a task is processing a task on it on the impl thread - WeakPtr
>>
> has DCHECKs
>
>> > > > against that, so I'm surprised this works.
>> > > >
>> > > > I'd also recommend storing the WeakPtr to the impl when you
>>
> create the
>
>> > > > ProxyImpl, to make the code a little more robust to any future
>>
> refactoring
>
>> > that
>> > > > might affect its lifetime.
>> > >
>> > > ProxyImpl is created and destroyed only on the impl thread.
>>
> ProxyImpl itself
>
>> > has DCHECKS to ensure that. We post tasks to the impl thread for
>>
> both these
>
>> > operations. Added comments for where that is being done.
>> >
>> > Sorry, I missed the proxy_impl_.reset() in the CloseOnImplThread().
>> >
>> > So, I think a DCHECK(!proxy_impl_) would make sense in
>>
> ~ThreadedChannel, since
>
>> > having had CloseOnImplThread() called on the impl thread before the
>>
> dtor runs on
>
>> > the main thread is a pre-requisite for correct teardown.
>> >
>>
>
> Yeah, Sorry I missed that. It should be there. It seemed like a bad
>>
> idea to have IsInitialized access proxy_impl_ from the main thread.
> While it only checks if ProxyImpl exists, and the initialization and
> destruction of proxy impl are blocking operations, I figured its better
> to restrict access to variables to the thread they belong to. Added a
> variable on the main thread which makes sure that the channel is not
> initialized for correct teardown.
>
> Not quite - it's absolutely fine to check whether |proxy_impl_| is set,
> from code on the main thread, _so long as that is the thread that
> controls the lifetime of the ProxyImpl object_, which is the case here -
> the ThreadedChannel owns the ProxyImpl and controls its lifetime, so so
> long as ThreadedChannel correctly Pass()es or release()s it when it's
> time to post to the impl thread to delete it, everything should be fine
> - the only thing you wouldn't want to do is actually call into the
> ProxyImpl from the main thread, since it's not designed to be called
> from multiple threads.
>
>
>
> > > Regarding the weak ptr, I'm not sure I completely understood that.
>>
> Do you mean
>
>> > we should store only the weak ptr for ProxyImpl here?
>> >
>> > Not quite; my point was mainly about avoiding calling
>> > ProxyImpl::weak_ptr_factory_::GetWeakPtr() from multiple threads,
>>
> since
>
>> > referring to the factory, which lives on thread A, from thread B
>>
> inherently
>
>> > risks accessing it after it's been deleted, which rather defeats the
>>
> object -
>
>> > taking and storing a WeakPtr on thread B to use to post tasks is
>>
> safer.
>
> Oh. I actually kept the factory in ProxyImpl since I assumed if
>>
> ProxyImpl owns the factory, it lowers the risk of accessing it after its
> been deleted since the lifetime of the factory and its pointers is tied
> to the lifetime of and controlled by ProxyImpl itself. It explicitly
> invalidates all the weak pointers in its dtor.
>
> I think you're right that it would be safer if we avoided calling
>>
> weak_factory::GetWeakPtr() from multiple threads. I've added a DCHECK to
> make sure that the caller asks for a weak pointer on the correct thread
> and we save it in the channel. Is that better?
>
> >
>> > However, looking through the ProxyImpl & ThreadedChannel:
>> > - ProxyImpl can dispense WeakPtrs, but it doesn't actually use them
>>
> itself.
>
>> > - Only the ThreadedChannel seems to use ProxyImpl WeakPtrs, to
>>
> auto-orphan
>
>> > pending tasks on the ProxyImpl when it gets torn down.
>> > - But the ThreadedChannel is actually the owner of the ProxyImpl.
>> >
>> > So you should be able to simplify all this:
>> > 1. Move the WeakPtrFactory<ProxyImpl> from the ProxyImpl itself out
>>
> into
>
>> > ThreadedChannel.
>> > 2. When the ProxyImpl is created, create a WeakPtrFactory<ProxyImpl>
>>
> to wrap it,
>
>> > and use that in your PostTask() binds.
>> > 3. When the ProxyImpl is deleted, delete the WeakPtrFactory to it
>>
> immediately
>
>> > beforehand.
>> >
>> > This avoids polluting the ProxyImpl with WeakPtr-dispensing stuff,
>>
> and keeps it
>
>> > all in the class that actually uses it.
>> >
>>
>
> I was a bit unsure about moving the weak factory for ProxyImpl to
>>
> ThreadedChannel since the pattern for using weakptrfactory is to always
> have the class be composed of a factory for itself. I guess this is to
> make sure that the factory never outlives the object and the class has
> control over how it exposes weak ptrs to itself.
>
> We could also have ProxyImpl extend SupportsWeakPtr, but I'm not sure
>>
> if that would be better. Wdyt?
>
> SupportsWeakPtr should ideally go away! The problem is that, unlike
> WeakPtrFactory, the dtor only runs _after_ the derived class' members
> have been torn down - so some part of the teardown causes a WeakPtr to
> the object to be dereferenced, it will still dereference even while the
> object is half gone - normally not an issue, but also no fun to have to
> diagnose... The other problem is simply that it tempts callers to think
> they can take WeakPtrs, which means they may create implicit threading
> dependencies without meaning to.
>

You could InvalidateWeakPtrs in the destructor if this is a problem in
practice?


>
> Having WeakPtrFactory inside the ProxyImpl benefits from implicitly
> invalidating WeakPtrs on deletion, but it also means polluting ProxyImpl
> with GetWeakPtr() even though it's really a ThreadedChannel
> implementation detail. There's a similar risk of callers (ab)using that
> in confusing ways as with SupportsWeakPtr.
>
> (This is a case where it'd actually be ideal to have a scoped_ptr<>
> specialization that embeds a WeakPtrFactory, to benefit from automatic
> invalidation without requiring objects themselves be polluted.)
>
>
>
> > > The class which implements ChannelImpl has to take ownership for
>>
> ProxyImpl. In
>
>> > this case it is the ThreadedChannel. In case of a network channel,
>>
> the
>
>> > ChannelImpl implementation which lives on the client will create and
>>
> control the
>
>> > lifetime of ProxyImpl.
>> >
>> > SGTM
>>
>
> https://codereview.chromium.org/1417053005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Wez

Yes, you can InvalidateWeakPtrs in the destructor, if you remember to, to address the dtor-time ...

5 years, 1 month ago (2015-11-18 19:36:29 UTC) #18

Yes, you can InvalidateWeakPtrs in the destructor, if you remember to, to
address the dtor-time issue, but then you've lost the value of having the
WeakPtr implementation dtor do that for you, as you'd have with
WeakPtrFactory as a member, and you still have the problem of having an
interface that let's new callers come along and take WeakPtrs to use in
ways you never intended.

On 18 November 2015 at 11:26, Dana Jansens <danakj@chromium.org> wrote:

> On Tue, Nov 17, 2015 at 5:50 PM, <wez@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
>> File cc/trees/threaded_channel.h (right):
>>
>>
>>
https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
>> cc/trees/threaded_channel.h:151: scoped_ptr<ProxyImpl> proxy_impl_;
>> On 2015/11/13 at 04:02:17, Khushal wrote:
>>
>>> On 2015/11/12 22:30:18, Wez wrote:
>>> > On 2015/11/11 at 04:17:47, Khushal wrote:
>>> > > On 2015/11/10 01:41:51, Wez wrote:
>>> > > > This doesn't look right; you're creating ThreadChannel on the
>>>
>> main thread, I
>>
>>> > > > think, and then ProxyImpl on that thread as well, then posting
>>>
>> tasks to call
>>
>>> > > > ProxyImpl on the impl thread, but then letting it get deleted
>>>
>> back on the
>>
>>> > > > ThreadChannel thread, i.e. on the main thread?
>>> > > >
>>> > > > For this to work correctly you'll need to post a task to the
>>>
>> impl thread to
>>
>>> > > > delete the ProxyImpl on it, otherwise you may delete it on the
>>>
>> main thread
>>
>>> > while
>>> > > > a task is processing a task on it on the impl thread - WeakPtr
>>>
>> has DCHECKs
>>
>>> > > > against that, so I'm surprised this works.
>>> > > >
>>> > > > I'd also recommend storing the WeakPtr to the impl when you
>>>
>> create the
>>
>>> > > > ProxyImpl, to make the code a little more robust to any future
>>>
>> refactoring
>>
>>> > that
>>> > > > might affect its lifetime.
>>> > >
>>> > > ProxyImpl is created and destroyed only on the impl thread.
>>>
>> ProxyImpl itself
>>
>>> > has DCHECKS to ensure that. We post tasks to the impl thread for
>>>
>> both these
>>
>>> > operations. Added comments for where that is being done.
>>> >
>>> > Sorry, I missed the proxy_impl_.reset() in the CloseOnImplThread().
>>> >
>>> > So, I think a DCHECK(!proxy_impl_) would make sense in
>>>
>> ~ThreadedChannel, since
>>
>>> > having had CloseOnImplThread() called on the impl thread before the
>>>
>> dtor runs on
>>
>>> > the main thread is a pre-requisite for correct teardown.
>>> >
>>>
>>
>> Yeah, Sorry I missed that. It should be there. It seemed like a bad
>>>
>> idea to have IsInitialized access proxy_impl_ from the main thread.
>> While it only checks if ProxyImpl exists, and the initialization and
>> destruction of proxy impl are blocking operations, I figured its better
>> to restrict access to variables to the thread they belong to. Added a
>> variable on the main thread which makes sure that the channel is not
>> initialized for correct teardown.
>>
>> Not quite - it's absolutely fine to check whether |proxy_impl_| is set,
>> from code on the main thread, _so long as that is the thread that
>> controls the lifetime of the ProxyImpl object_, which is the case here -
>> the ThreadedChannel owns the ProxyImpl and controls its lifetime, so so
>> long as ThreadedChannel correctly Pass()es or release()s it when it's
>> time to post to the impl thread to delete it, everything should be fine
>> - the only thing you wouldn't want to do is actually call into the
>> ProxyImpl from the main thread, since it's not designed to be called
>> from multiple threads.
>>
>>
>>
>> > > Regarding the weak ptr, I'm not sure I completely understood that.
>>>
>> Do you mean
>>
>>> > we should store only the weak ptr for ProxyImpl here?
>>> >
>>> > Not quite; my point was mainly about avoiding calling
>>> > ProxyImpl::weak_ptr_factory_::GetWeakPtr() from multiple threads,
>>>
>> since
>>
>>> > referring to the factory, which lives on thread A, from thread B
>>>
>> inherently
>>
>>> > risks accessing it after it's been deleted, which rather defeats the
>>>
>> object -
>>
>>> > taking and storing a WeakPtr on thread B to use to post tasks is
>>>
>> safer.
>>
>> Oh. I actually kept the factory in ProxyImpl since I assumed if
>>>
>> ProxyImpl owns the factory, it lowers the risk of accessing it after its
>> been deleted since the lifetime of the factory and its pointers is tied
>> to the lifetime of and controlled by ProxyImpl itself. It explicitly
>> invalidates all the weak pointers in its dtor.
>>
>> I think you're right that it would be safer if we avoided calling
>>>
>> weak_factory::GetWeakPtr() from multiple threads. I've added a DCHECK to
>> make sure that the caller asks for a weak pointer on the correct thread
>> and we save it in the channel. Is that better?
>>
>> >
>>> > However, looking through the ProxyImpl & ThreadedChannel:
>>> > - ProxyImpl can dispense WeakPtrs, but it doesn't actually use them
>>>
>> itself.
>>
>>> > - Only the ThreadedChannel seems to use ProxyImpl WeakPtrs, to
>>>
>> auto-orphan
>>
>>> > pending tasks on the ProxyImpl when it gets torn down.
>>> > - But the ThreadedChannel is actually the owner of the ProxyImpl.
>>> >
>>> > So you should be able to simplify all this:
>>> > 1. Move the WeakPtrFactory<ProxyImpl> from the ProxyImpl itself out
>>>
>> into
>>
>>> > ThreadedChannel.
>>> > 2. When the ProxyImpl is created, create a WeakPtrFactory<ProxyImpl>
>>>
>> to wrap it,
>>
>>> > and use that in your PostTask() binds.
>>> > 3. When the ProxyImpl is deleted, delete the WeakPtrFactory to it
>>>
>> immediately
>>
>>> > beforehand.
>>> >
>>> > This avoids polluting the ProxyImpl with WeakPtr-dispensing stuff,
>>>
>> and keeps it
>>
>>> > all in the class that actually uses it.
>>> >
>>>
>>
>> I was a bit unsure about moving the weak factory for ProxyImpl to
>>>
>> ThreadedChannel since the pattern for using weakptrfactory is to always
>> have the class be composed of a factory for itself. I guess this is to
>> make sure that the factory never outlives the object and the class has
>> control over how it exposes weak ptrs to itself.
>>
>> We could also have ProxyImpl extend SupportsWeakPtr, but I'm not sure
>>>
>> if that would be better. Wdyt?
>>
>> SupportsWeakPtr should ideally go away! The problem is that, unlike
>> WeakPtrFactory, the dtor only runs _after_ the derived class' members
>> have been torn down - so some part of the teardown causes a WeakPtr to
>> the object to be dereferenced, it will still dereference even while the
>> object is half gone - normally not an issue, but also no fun to have to
>> diagnose... The other problem is simply that it tempts callers to think
>> they can take WeakPtrs, which means they may create implicit threading
>> dependencies without meaning to.
>>
>
> You could InvalidateWeakPtrs in the destructor if this is a problem in
> practice?
>
>
>>
>> Having WeakPtrFactory inside the ProxyImpl benefits from implicitly
>> invalidating WeakPtrs on deletion, but it also means polluting ProxyImpl
>> with GetWeakPtr() even though it's really a ThreadedChannel
>> implementation detail. There's a similar risk of callers (ab)using that
>> in confusing ways as with SupportsWeakPtr.
>>
>> (This is a case where it'd actually be ideal to have a scoped_ptr<>
>> specialization that embeds a WeakPtrFactory, to benefit from automatic
>> invalidation without requiring objects themselves be polluted.)
>>
>>
>>
>> > > The class which implements ChannelImpl has to take ownership for
>>>
>> ProxyImpl. In
>>
>>> > this case it is the ThreadedChannel. In case of a network channel,
>>>
>> the
>>
>>> > ChannelImpl implementation which lives on the client will create and
>>>
>> control the
>>
>>> > lifetime of ProxyImpl.
>>> >
>>> > SGTM
>>>
>>
>> https://codereview.chromium.org/1417053005/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Khushal

On 2015/11/18 01:50:35, Wez wrote: > https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_channel.h > File cc/trees/threaded_channel.h (right): > > https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_channel.h#newcode151 > ...

5 years, 1 month ago (2015-11-18 21:53:51 UTC) #19

On 2015/11/18 01:50:35, Wez wrote:
>
https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
> File cc/trees/threaded_channel.h (right):
> 
>
https://codereview.chromium.org/1417053005/diff/40001/cc/trees/threaded_chann...
> cc/trees/threaded_channel.h:151: scoped_ptr<ProxyImpl> proxy_impl_;
> On 2015/11/13 at 04:02:17, Khushal wrote:
> > On 2015/11/12 22:30:18, Wez wrote:
> > > On 2015/11/11 at 04:17:47, Khushal wrote:
> > > > On 2015/11/10 01:41:51, Wez wrote:
> > > > > This doesn't look right; you're creating ThreadChannel on the main
> thread, I
> > > > > think, and then ProxyImpl on that thread as well, then posting tasks
to
> call
> > > > > ProxyImpl on the impl thread, but then letting it get deleted back on
> the
> > > > > ThreadChannel thread, i.e. on the main thread?
> > > > > 
> > > > > For this to work correctly you'll need to post a task to the impl
thread
> to
> > > > > delete the ProxyImpl on it, otherwise you may delete it on the main
> thread
> > > while
> > > > > a task is processing a task on it on the impl thread - WeakPtr has
> DCHECKs
> > > > > against that, so I'm surprised this works.
> > > > > 
> > > > > I'd also recommend storing the WeakPtr to the impl when you create the
> > > > > ProxyImpl, to make the code a little more robust to any future
> refactoring
> > > that
> > > > > might affect its lifetime.
> > > > 
> > > > ProxyImpl is created and destroyed only on the impl thread. ProxyImpl
> itself
> > > has DCHECKS to ensure that. We post tasks to the impl thread for both
these
> > > operations. Added comments for where that is being done.
> > > 
> > > Sorry, I missed the proxy_impl_.reset() in the CloseOnImplThread().
> > > 
> > > So, I think a DCHECK(!proxy_impl_) would make sense in ~ThreadedChannel,
> since
> > > having had CloseOnImplThread() called on the impl thread before the dtor
> runs on
> > > the main thread is a pre-requisite for correct teardown.
> > > 
> > 
> > Yeah, Sorry I missed that. It should be there. It seemed like a bad idea to
> have IsInitialized access proxy_impl_ from the main thread. While it only
checks
> if ProxyImpl exists, and the initialization and destruction of proxy impl are
> blocking operations, I figured its better to restrict access to variables to
the
> thread they belong to. Added a variable on the main thread which makes sure
that
> the channel is not initialized for correct teardown.
> 
> Not quite - it's absolutely fine to check whether |proxy_impl_| is set, from
> code on the main thread, _so long as that is the thread that controls the
> lifetime of the ProxyImpl object_, which is the case here - the
ThreadedChannel
> owns the ProxyImpl and controls its lifetime, so so long as ThreadedChannel
> correctly Pass()es or release()s it when it's time to post to the impl thread
to
> delete it, everything should be fine - the only thing you wouldn't want to do
is
> actually call into the ProxyImpl from the main thread, since it's not designed
> to be called from multiple threads.
> 

I am trying to follow the pattern similar to ThreadProxy so its obvious to
anyone reading the code that ProxyImpl should not be used on the main thread. We
just need it to check that the ThreadedChannel is destroyed correctly. The
initialized flag should do that for us. :)

> > 
> > > > Regarding the weak ptr, I'm not sure I completely understood that. Do
you
> mean
> > > we should store only the weak ptr for ProxyImpl here?
> > > 
> > > Not quite; my point was mainly about avoiding calling
> > > ProxyImpl::weak_ptr_factory_::GetWeakPtr() from multiple threads, since
> > > referring to the factory, which lives on thread A, from thread B
inherently
> > > risks accessing it after it's been deleted, which rather defeats the
object
> -
> > > taking and storing a WeakPtr on thread B to use to post tasks is safer.
> > 
> > Oh. I actually kept the factory in ProxyImpl since I assumed if ProxyImpl
owns
> the factory, it lowers the risk of accessing it after its been deleted since
the
> lifetime of the factory and its pointers is tied to the lifetime of and
> controlled by ProxyImpl itself. It explicitly invalidates all the weak
pointers
> in its dtor.
> > 
> > I think you're right that it would be safer if we avoided calling
> weak_factory::GetWeakPtr() from multiple threads. I've added a DCHECK to make
> sure that the caller asks for a weak pointer on the correct thread and we save
> it in the channel. Is that better?
> > 
> > > 
> > > However, looking through the ProxyImpl & ThreadedChannel:
> > > - ProxyImpl can dispense WeakPtrs, but it doesn't actually use them
itself.
> > > - Only the ThreadedChannel seems to use ProxyImpl WeakPtrs, to auto-orphan
> > > pending tasks on the ProxyImpl when it gets torn down.
> > > - But the ThreadedChannel is actually the owner of the ProxyImpl.
> > > 
> > > So you should be able to simplify all this:
> > > 1. Move the WeakPtrFactory<ProxyImpl> from the ProxyImpl itself out into
> > > ThreadedChannel.
> > > 2. When the ProxyImpl is created, create a WeakPtrFactory<ProxyImpl> to
wrap
> it,
> > > and use that in your PostTask() binds.
> > > 3. When the ProxyImpl is deleted, delete the WeakPtrFactory to it
> immediately
> > > beforehand.
> > > 
> > > This avoids polluting the ProxyImpl with WeakPtr-dispensing stuff, and
keeps
> it
> > > all in the class that actually uses it.
> > > 
> > 
> > I was a bit unsure about moving the weak factory for ProxyImpl to
> ThreadedChannel since the pattern for using weakptrfactory is to always have
the
> class be composed of a factory for itself. I guess this is to make sure that
the
> factory never outlives the object and the class has control over how it
exposes
> weak ptrs to itself.
> > 
> > We could also have ProxyImpl extend SupportsWeakPtr, but I'm not sure if
that
> would be better. Wdyt?
> 
> SupportsWeakPtr should ideally go away! The problem is that, unlike
> WeakPtrFactory, the dtor only runs _after_ the derived class' members have
been
> torn down - so some part of the teardown causes a WeakPtr to the object to be
> dereferenced, it will still dereference even while the object is half gone -
> normally not an issue, but also no fun to have to diagnose... The other
problem
> is simply that it tempts callers to think they can take WeakPtrs, which means
> they may create implicit threading dependencies without meaning to.
> 
> Having WeakPtrFactory inside the ProxyImpl benefits from implicitly
invalidating
> WeakPtrs on deletion, but it also means polluting ProxyImpl with GetWeakPtr()
> even though it's really a ThreadedChannel implementation detail. There's a
> similar risk of callers (ab)using that in confusing ways as with
> SupportsWeakPtr.

Done. Moved the weak ptr factories to the ThreadedChannel for both ProxyMain and
ProxyImpl.

> 
> (This is a case where it'd actually be ideal to have a scoped_ptr<>
> specialization that embeds a WeakPtrFactory, to benefit from automatic
> invalidation without requiring objects themselves be polluted.)
> 
> > 
> > > > The class which implements ChannelImpl has to take ownership for
> ProxyImpl. In
> > > this case it is the ThreadedChannel. In case of a network channel, the
> > > ChannelImpl implementation which lives on the client will create and
control
> the
> > > lifetime of ProxyImpl.
> > > 
> > > SGTM

brianderson

https://codereview.chromium.org/1417053005/diff/220001/cc/test/layer_tree_test.h File cc/test/layer_tree_test.h (right): https://codereview.chromium.org/1417053005/diff/220001/cc/test/layer_tree_test.h#newcode252 cc/test/layer_tree_test.h:252: // TODO(khushalsagar): Add mode for running remote channel tests. ...

5 years, 1 month ago (2015-11-19 21:18:15 UTC) #20

Khushal

https://codereview.chromium.org/1417053005/diff/220001/cc/test/layer_tree_test.h File cc/test/layer_tree_test.h (right): https://codereview.chromium.org/1417053005/diff/220001/cc/test/layer_tree_test.h#newcode252 cc/test/layer_tree_test.h:252: // TODO(khushalsagar): Add mode for running remote channel tests. ...

5 years, 1 month ago (2015-11-19 22:43:49 UTC) #21

https://codereview.chromium.org/1417053005/diff/220001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/1417053005/diff/220001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.h:252: // TODO(khushalsagar): Add mode for running
remote channel tests.
On 2015/11/19 21:18:15, brianderson wrote:
> Sounds like huge TODO! =)
> 
> It'll be interesting to see how many flaky tests this exposes. It may be a
> worthwhile experiment to run all these tests locally with a fake local channel
> that delays all messages.

This is just to run the unit-tests that will check that the channel serializes
and deserializes objects properly.

The main thread on the client is essentially an extension of the main thread on
the server, so they'll run exactly as you said, a fake RemoteProtoChannel which
directly passes the protos to the RemoteProtoChannel::Receiver on the other
side.

For now we'll use it to only run smoke tests for the RemoteChannelMain and
RemoteChannelImpl, similar to ThreadedChannelTests.

https://codereview.chromium.org/1417053005/diff/220001/cc/trees/channel_main.h
File cc/trees/channel_main.h (right):

https://codereview.chromium.org/1417053005/diff/220001/cc/trees/channel_main....
cc/trees/channel_main.h:63: virtual bool IsInitialized() const = 0;
On 2015/11/19 21:18:15, brianderson wrote:
> Is this needed? Looks like it's only used internally by ThreadedChannel.

Probably not right now. Removed.

https://codereview.chromium.org/1417053005/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1417053005/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:147:
proxy_main->SetChannel(threaded_channel.Pass());
On 2015/11/19 21:18:15, brianderson wrote:
> Is the channel going to change dynamically at runtime? If not, can you remove
> SetChannel and pass the channel into the Proxy constructor here and elsewhere?

The channel needs the proxy and the proxy needs the channel. :P
I can swap it, build the proxy with a channel and then set proxy on the channel.
Would that be better?

Wez

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/channel_main.h File cc/trees/channel_main.h (right): https://codereview.chromium.org/1417053005/diff/260001/cc/trees/channel_main.h#newcode63 cc/trees/channel_main.h:63: virtual ~ChannelMain() {} nit: dtor should be declared before ...

5 years ago (2015-11-21 01:00:30 UTC) #22

Khushal

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/channel_main.h File cc/trees/channel_main.h (right): https://codereview.chromium.org/1417053005/diff/260001/cc/trees/channel_main.h#newcode63 cc/trees/channel_main.h:63: virtual ~ChannelMain() {} On 2015/11/21 01:00:29, Wez wrote: > ...

5 years ago (2015-11-23 20:07:20 UTC) #23

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/channel_main.h
File cc/trees/channel_main.h (right):

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/channel_main....
cc/trees/channel_main.h:63: virtual ~ChannelMain() {}
On 2015/11/21 01:00:29, Wez wrote:
> nit: dtor should be declared before members.

Done.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:147:
proxy_main->SetChannel(threaded_channel.Pass());
On 2015/11/21 01:00:29, Wez wrote:
> nit: Doesn't look like you need the |threaded_channel| local var here - just
> pass the result of Create() directly to SetChannel?

Done.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:116: TRACE_EVENT1("cc",
"ProxyImpl::SetVisibleOnImplThread", "visible", visible);
On 2015/11/21 01:00:29, Wez wrote:
> Thread check here and function below?

Done.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:288: void
ProxyImpl::ScheduledActionSendBeginMainFrame(const BeginFrameArgs& args) {
On 2015/11/21 01:00:29, Wez wrote:
> nit: Thread check?

Done.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:311: channel_impl_->BeginMainFrameNotExpectedSoon();
On 2015/11/21 01:00:29, Wez wrote:
> nit: Thread check?
> 
> The method definitions don't seem to match the declaration order from the
> header, which makes them confusing to follow; can we order them to match,
i.e.
> group according to the interface they belong to?

I have kept the order consistent with ThreadProxy. I think the functions were
ordered based on a logical call order for operations. For instance, in
ScheduledActionBeginMainFrame, we send BeginMainFrame to the main side and after
this we would expect either BeginMainFrameAbortedOnImpl or StartCommitOnImpl.
I'd rather the order stay as it was originally.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:338: // Ideally, we should inform to impl thread when
BeginMainFrame is started.
On 2015/11/21 01:00:29, Wez wrote:
> Not clear what this means; we're on the impl thread, right? What does it mean
to
> "inform to impl thread" that we're not already doing?

I think this means that we should be doing a PostTask to tell the scheduler that
the main frame is started at the beginning of ProxyMain::BeginMainFrame but we
avoid the thread hop.
This has been kept consistent with ThreadProxy.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:425: DrawResult result;
On 2015/11/21 01:00:29, Wez wrote:
> Declare this immediately before its first use, i.e. before the if(CanDraw)
> block.

Keeping this consistent with ThreadProxy.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:454: draw_frame = forced_draw || result == DRAW_SUCCESS;
On 2015/11/21 01:00:29, Wez wrote:
> In the if(draw_frame) case below we always force result to DRAW_SUCCESS, so
you
> could get rid of draw_frame and instead have:
> 
> if (CanDraw()) {
>   result = PrepareToDraw()
>   if (forced_draw) result == DRAW_SUCCESS; // Ignore prepare failures if
forced
> to draw.
> 
>   if (result == DRAW_SUCCESS) {
>     DrawLayers()
>   }
> } else {
>   result = DRAW_ABORTED;
> }
> 
> and the later draw_frame tests also become == DRAW_SUCCESS conditions.

Keeping this consistent with ThreadProxy.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:497: return DrawSwapInternal(forced_draw);
On 2015/11/21 01:00:29, Wez wrote:
> nit: Presumably the variable is for clarity? Can we replace it with a static
> enum to avoid the need for that?

Keeping this consistent with ThreadProxy.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:513: layer_tree_host_impl_->DidFinishImplFrame();
On 2015/11/21 01:00:29, Wez wrote:
> Thread check?

Done.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:688: return !!commit_completion_event_;
On 2015/11/21 01:00:29, Wez wrote:
> nit: Prefer foo != nullptr rather than !!foo

Done.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:23: // This class is a proxy to the impl side of the
compositor. It is created and
On 2015/11/21 01:00:30, Wez wrote:
> nit: Can we clarify in what sense it is a "proxy to the impl side"; when I
think
> of a "proxy" I expect a class that implements some particular interface and
> proxies calls to that e.g. through IPC, or across a thread boundary, to a real
> implementation of that same interface, somewhere else.

ProxyImpl now has the code from ThreadProxy that glues together the
LayerTreeHostImpl and Scheduler and provides an interface that the Channel
implementations use to proxy calls from the main side of the compositor to this
glue code.
Would that be an apt description?

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.cc (right):

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:114: void
ThreadedChannel::SynchronouslyInitializeImpl(
On 2015/11/21 01:00:30, Wez wrote:
> Why does this need to be Synchronous? Does the Impl-thread init require
calling
> back into Main-thread-owned objects...?

The initialization uses the LayerTreeHost which is a main-thread owned object.
One reason I can see for having it blocking is that the input_handler is set
during initialization here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
And the embedder can try to grab it immediately after creating the
LayerTreeHost. There may be other reasons that I don't know.

I think it would be better if we let this patch only refactor the plumbing of
calls between the thread boundary. Changing any calls from synchronous to
asynchronous should be addressed in separate patches so we can carefully review
any assumptions that different components in the system might have made about
that call being synchronous.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:139: external_begin_frame_source.Pass());
On 2015/11/21 01:00:30, Wez wrote:
> AFAICT the only reason that this needs to run on the Impl thread is that the
> ProxyImpl constructor calls into the supplied LayerTreeHost, which is
> Impl-thread-only?  But I see you're doing that with the Main thread blocked -
so

The LayerTreeHost is main-thread-only and should be accessed on the impl thread
only when the main thread is blocked.

> are those calls actually ones that would make sense to do on the Main thread? 
> 
> The model I was originally proposing was to have ThreadedChannel create an
> object (possibly ProxyImpl) on the Main thread, and then for all subsequent
> actions, including initialization, to take place on the Impl thread. That way
> you can create the object, create a WeakPtrFactory to wrap it and take a
WeakPtr
> directly in the Main-thread init function, then just PostTask the ProxyImpl &
> WeakPtrFactory to the Impl thread to be torn down.  That way the
ThreadedChannel
> gets to operate exclusively on the Main thread.

Its okay to have ThreadedChannel operate on both threads. I think it would be
cleaner to have a single class implement thread-hopping for both the sides.
Splitting it will not change the synchronous nature of any calls between the
threads.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.h (right):

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/threaded_chan...
cc/trees/threaded_channel.h:159: void InitializeImplOnImplThread(
On 2015/11/21 01:00:30, Wez wrote:
> I thought we had agreed to break ThreadedChannel into the external
> ThreadedChannel that runs exclusively on the Main thread, and an internal
> sub-component that runs on the Impl thread?

For this patch, It would make sense to keep the split as-is. It makes it easy to
follow that we have pulled out the parts from ThreadProxy that separate the
thread boundary into the ThreadedChannel.

The idea of ThreadProxy was to separate the main and impl side and have
ThreadProxy as the class that exists on both threads and glues them together.
Now we've pushed the logic that was exclusive to each thread into ProxyMain and
ProxyImpl and have ThreadedChannel as the class that exists on both threads and
deals exclusively with thread hopping. I think the current split makes it easier
to follow the aim of this refactoring and would keep it readable. If there is a
need to split ThreadedChannel further, how about we address that in a subsequent
patch?

Wez

Apologies for the delay... https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc File cc/trees/proxy_impl.cc (right): https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc#newcode311 cc/trees/proxy_impl.cc:311: channel_impl_->BeginMainFrameNotExpectedSoon(); On 2015/11/23 at 20:07:19, ...

5 years ago (2015-11-24 22:37:00 UTC) #24

Khushal

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc File cc/trees/proxy_impl.cc (right): https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc#newcode311 cc/trees/proxy_impl.cc:311: channel_impl_->BeginMainFrameNotExpectedSoon(); On 2015/11/24 22:36:59, Wez wrote: > On 2015/11/23 ...

5 years ago (2015-11-25 06:48:12 UTC) #25

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:311: channel_impl_->BeginMainFrameNotExpectedSoon();
On 2015/11/24 22:36:59, Wez wrote:
> On 2015/11/23 at 20:07:19, Khushal wrote:
> > On 2015/11/21 01:00:29, Wez wrote:
> > > nit: Thread check?
> > > 
> > > The method definitions don't seem to match the declaration order from the
> > > header, which makes them confusing to follow; can we order them to match,
> i.e.
> > > group according to the interface they belong to?
> > 
> > I have kept the order consistent with ThreadProxy. I think the functions
were
> ordered based on a logical call order for operations. For instance, in
> ScheduledActionBeginMainFrame, we send BeginMainFrame to the main side and
after
> this we would expect either BeginMainFrameAbortedOnImpl or StartCommitOnImpl.
> I'd rather the order stay as it was originally.
> 
> I get the desire not to create churn by fixing all the style violations here,
> but in the case of method ordering and placement of variable declaration
> (below), I don't see what the benefit is of keeping the existing violations,
> given that (1) within this .cc file things would be consistent and (2) this
code
> file is not a copy/modification of ThreadProxy, at least as far as codereview
is
> concerned, so there doesn't seem to be benefit in terms of minimizing diffs.

Fair enough. I have re-ordered both proxy_*.cc files to match the order in which
the functions are declared.

https://codereview.chromium.org/1417053005/diff/260001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:425: DrawResult result;
On 2015/11/21 01:00:29, Wez wrote:
> Declare this immediately before its first use, i.e. before the if(CanDraw)
> block.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:69: kSmoothnessTakesPriorityExpirationDelay * 1000)),
On 2015/11/24 22:36:59, Wez wrote:
> nit: Use TimeDelta::FromSeconds rather than FromMilliseconds( .. * 1000) :D

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:77: DCHECK(layer_tree_host);
On 2015/11/24 22:36:59, Wez wrote:
> This check is redundant, since you already de-ref'd layer_tree_host during
> member initialization, to get its id(), and you're about to call into it to
> create the LayerTreeHostImpl, below.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:93: DCHECK_EQ(scheduler_->visible(),
layer_tree_host_impl_->visible());
On 2015/11/24 22:36:59, Wez wrote:
> nit: Explanatory comment would help on this.

This has been moved from ThreadProxy.
+vmpstr, +danakj would be able to explain better.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:350: // But, we can avoid a PostTask in here.
On 2015/11/24 22:36:59, Wez wrote:
> As per your comments, it sounds like you're saying that ideally you'd notify
the
> scheduler when BeingMainFrame is started, but doing it here, while strictly
> wrong, avoids the overhead of.. a thread hop? Or just a MessageLoop spin?

This has been moved from ThreadProxy too.
+brianderson, since this concerns the Scheduler, he'll be able to explain
better.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:364: DCHECK(scheduler_);
On 2015/11/24 22:36:59, Wez wrote:
> No point DCHECKing |scheduler_| if you're about to deref it anyway.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:389: DCHECK(blocked_main_commit().layer_tree_host);
On 2015/11/24 22:36:59, Wez wrote:
> nit: You're about to deref this, below, so DCHECK is redundant.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:25: // impl thread.
On 2015/11/24 22:36:59, Wez wrote:
> As discussed offline, this class is effectively an aggregator for all
> interactions that the Main/content side of the compositor needs to have with
the
> actual Impl - so not a "proxy" in the Chromium sense.  So, as discussed, fine
to
> keep the naming as-is for consistency, but let's make sure the comment
> accurately reflects the function it performs?

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:43: };
On 2015/11/24 22:36:59, Wez wrote:
> Why does this struct declaration need to be public rather than private? 
> Regardless of which section it's in, it should be declared at the top of that
> section, as per style-guide.

It doesn't need to be public. Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:102: }
On 2015/11/24 22:36:59, Wez wrote:
> nit: Since these aren't just implementing an interface, suggest a block
comment
> to clarify why they're needed / who calls them.
> 
> Also, why is HasCommitCompletionEvent not a simple getter, given that all it
> does it check whether a pointer is null?

These are needed only for testing. Updated the method names so presubmit makes
sure that they are used only in test code.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:108: scoped_ptr<BeginFrameSource>
external_begin_frame_source);
On 2015/11/24 22:36:59, Wez wrote:
> nit: Comment to explain why this needs to be protected rather than private?

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:115: // virtual for testing.
On 2015/11/24 22:36:59, Wez wrote:
> This _is_ the ProxyImpl, so I'm not sure what you mean by "ProxyImpl interface
> for the channel." - are you saying that this is the interface that the

In other cc classes, a comment like this before a block of methods is used to
make it clear who calls them. For instance,
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

> ThreadedChannel uses?  Surely this is the interface that should be "public",
for
> Channel implementations to use, not shared with them via friend?
> 
> Similarly, you have the LayerTreeHostClient and SchedulerClient interfaces
> public, above, even though those are really an implementation detail of
> ProxyImpl (since ProxyImpl owns the Scheduler and LayerTreeHostImpl, and can
> implement the *Client interfaces any way it wants) - the ProxyImpl's
> creator/caller shouldn't be calling those APIs.

In previous changes, it was decided to have Channel implementations be declared
as friend classes so these methods are not declared public.

I would be fine to have them as public and implement the SchedulerClient and
LayerTreeHostImplClient privately. In that case, should we consider having Proxy
implemented privately in ProxyMain and having the API that ChannelMain sees
public?

+vmpstr, What do you think?

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:141: DrawResult DrawSwapInternal(bool forced_draw);
On 2015/11/24 22:36:59, Wez wrote:
> nit: DrawAndSwapInternal, for consistency with the naming of the methods that
> call it.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_main.cc
File cc/trees/proxy_main.cc (right):

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:55:
external_begin_frame_source_(external_begin_frame_source.Pass()) {
On 2015/11/24 22:36:59, Wez wrote:
> nit: Given all the thread-hopping involved, consider DCHECKing e.g. that
> external_begin_frame_source is non-null, or use_external_being_frame_source is
> false in the layer-tree-host settings, here.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:59: DCHECK(layer_tree_host_);
On 2015/11/24 22:36:59, Wez wrote:
> nit: No point DCHECKing, already deref'd, above.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:147: renderer_capabilities_main_thread_copy_ =
capabilities;
On 2015/11/24 22:36:59, Wez wrote:
> DCHECK(IsMainThread())?

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:454:
channel_main_->UpdateTopControlsStateOnImpl(constraints, current, animate);
On 2015/11/24 22:36:59, Wez wrote:
> nit: Thread DCHECK?

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_main.h
File cc/trees/proxy_main.h (right):

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_main.h#...
cc/trees/proxy_main.h:102: CommitPipelineStage required_stage);
On 2015/11/24 22:36:59, Wez wrote:
> nit: Why is this before the ProxyMain "interface", but IsMainThread below it?

It doesn't need to be before it.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_main.h#...
cc/trees/proxy_main.h:105: // virtual for testing.
On 2015/11/24 22:36:59, Wez wrote:
> If this is the ProxyMain's interface presented to the channel then surely it
> should be public, so the [Threaded?]Channel can see it, rather than using
friend
> to hack visibility?

We decided on using friend classes for this in a previous change. I would be
fine in having these as public and implementing Proxy privately.

+vmpstr, wdyt?

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_main.h#...
cc/trees/proxy_main.h:148: RendererCapabilities
renderer_capabilities_main_thread_copy_;
On 2015/11/24 22:36:59, Wez wrote:
> Can we have one-liner comments to explain this and the bools above, plz?

Looks like prepare_tiles_pending_ is not used anymore.

These have been moved from ThreadProxy too.
+vmpstr, +danakj would be able to comment better.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.cc (right):

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:38: ImplThreadTaskRunner()->PostTask(
On 2015/11/24 22:36:59, Wez wrote:
> Here and below, if the expectation is that this all is always made on the Main
> thread then DCHECK() that fact to make it an explicit assumption.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:128: proxy_main_weak_ptr_ =
main().proxy_main_weak_factory.GetWeakPtr();
On 2015/11/24 22:36:59, Wez wrote:
> Is it safe to do this _after_ the InitializeImplOnImplThread? Or might
something
> in ProxyImpl cause a call back in to ThreadedChannel that in turns needs to
post
> a task using |proxy_main_weak_ptr_|, in which case that task would be silently
> dropped?
> 
> In fact, this assignment is racey, since normally proxy_main_weak_ptr_ is only
> read from the Impl thread.

You're right, this assignment can be racey. I have moved it to
CompositorThreadOnly so it is set when the ThreadedChannel is constructed and
the pointer is invalidated when the impl side is destroyed.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.h (right):

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/threaded_chan...
cc/trees/threaded_channel.h:64: //
----------------------------------------------------------------------------
On 2015/11/24 22:37:00, Wez wrote:
> This comment needs to be explicit that having a single ThreadedChannel object,
> with parts called on Main and parts called on Impl, is safe because (1) the
only
> Impl-threaded caller(s) of ThreadedChannel are ThreadedChannel itself, and the
> ProxyImpl, which it owns, and (2) because the ThreadedImpl actually blocks the
> Main thread to wait for Impl-thread teardown to complete, so there's no risk
of
> any queued tasks touching it on the Impl thread after it's been deleted on
Main.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/threaded_chan...
cc/trees/threaded_channel.h:150: // virtual for testing.
On 2015/11/24 22:36:59, Wez wrote:
> nit: Virtual

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/threaded_chan...
cc/trees/threaded_channel.h:158: // called on impl thread.
On 2015/11/24 22:37:00, Wez wrote:
> nit: Called
> 
> Isn't it implicit that anything with OnImpl in its name is on the Impl thread,
> as for the ChannelMain interface above?  Consider removing the Thread suffix
> from these, for consistency w/ ChannelMain.

Done.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/threaded_chan...
cc/trees/threaded_channel.h:182: base::WeakPtr<ProxyMain> proxy_main_weak_ptr_;
On 2015/11/24 22:37:00, Wez wrote:
> Comment e.g: "Used on the Impl thread to safely queue calls to ProxyMain on
the
> Main thread."
> 
> Main thing is we want to explain whether this is about thread safety, or about
> abandoning tasks posted from the same thread, because of the object maybe
> getting deleted.
> 
> Why is this member not in CompositorThreadOnly?

Done. It should be in CompositorThreadOnly. Moved it there.

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/threaded_chan...
cc/trees/threaded_channel.h:184: base::WeakPtr<ProxyImpl> proxy_impl_weak_ptr_;
On 2015/11/24 22:36:59, Wez wrote:
> Comment e.g. Used on the Main thread to safely queue calls to the ProxyImpl on
> the Impl thread.
> 
> However, why do we need this? ThreadedChannel already controls the lifetime of
> ProxyImpl - it deletes it in CloseImplOnImplThread - so there is no
possibility
> of it posting any further tasks to it after it has been deleted, surely?

We need a reference to ProxyImpl that can be safely accessed on the main thread
only for the purpose of posting tasks to be run on the impl thread. Added a
comment explaining this as well.

vmpstr

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h File cc/trees/proxy_impl.h (right): https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#newcode115 cc/trees/proxy_impl.h:115: // virtual for testing. On 2015/11/25 06:48:12, Khushal wrote: ...

5 years ago (2015-11-25 19:30:39 UTC) #26

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:115: // virtual for testing.
On 2015/11/25 06:48:12, Khushal wrote:
> On 2015/11/24 22:36:59, Wez wrote:
> > This _is_ the ProxyImpl, so I'm not sure what you mean by "ProxyImpl
interface
> > for the channel." - are you saying that this is the interface that the
> 
> In other cc classes, a comment like this before a block of methods is used to
> make it clear who calls them. For instance,
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> 
> > ThreadedChannel uses?  Surely this is the interface that should be "public",
> for
> > Channel implementations to use, not shared with them via friend?
> > 
> > Similarly, you have the LayerTreeHostClient and SchedulerClient interfaces
> > public, above, even though those are really an implementation detail of
> > ProxyImpl (since ProxyImpl owns the Scheduler and LayerTreeHostImpl, and can
> > implement the *Client interfaces any way it wants) - the ProxyImpl's
> > creator/caller shouldn't be calling those APIs.
> 
> In previous changes, it was decided to have Channel implementations be
declared
> as friend classes so these methods are not declared public.
> 
> I would be fine to have them as public and implement the SchedulerClient and
> LayerTreeHostImplClient privately. In that case, should we consider having
Proxy
> implemented privately in ProxyMain and having the API that ChannelMain sees
> public?
> 
> +vmpstr, What do you think?

We've decided to make these private precisely because only the channels access
them. If they are public functions, then anything that knows that this class is
a proxy class can access them. That being said, I think we've moved away from
anything knowing this. That is, we only expose client interfaces so I would be
OK with making this public (but please double check that nothing can access this
inadvertently)

Khushal

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h File cc/trees/proxy_impl.h (right): https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#newcode115 cc/trees/proxy_impl.h:115: // virtual for testing. On 2015/11/25 19:30:39, vmpstr wrote: ...

5 years ago (2015-11-25 22:42:25 UTC) #27

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/280001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:115: // virtual for testing.
On 2015/11/25 19:30:39, vmpstr wrote:
> On 2015/11/25 06:48:12, Khushal wrote:
> > On 2015/11/24 22:36:59, Wez wrote:
> > > This _is_ the ProxyImpl, so I'm not sure what you mean by "ProxyImpl
> interface
> > > for the channel." - are you saying that this is the interface that the
> > 
> > In other cc classes, a comment like this before a block of methods is used
to
> > make it clear who calls them. For instance,
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> > 
> > > ThreadedChannel uses?  Surely this is the interface that should be
"public",
> > for
> > > Channel implementations to use, not shared with them via friend?
> > > 
> > > Similarly, you have the LayerTreeHostClient and SchedulerClient interfaces
> > > public, above, even though those are really an implementation detail of
> > > ProxyImpl (since ProxyImpl owns the Scheduler and LayerTreeHostImpl, and
can
> > > implement the *Client interfaces any way it wants) - the ProxyImpl's
> > > creator/caller shouldn't be calling those APIs.
> > 
> > In previous changes, it was decided to have Channel implementations be
> declared
> > as friend classes so these methods are not declared public.
> > 
> > I would be fine to have them as public and implement the SchedulerClient and
> > LayerTreeHostImplClient privately. In that case, should we consider having
> Proxy
> > implemented privately in ProxyMain and having the API that ChannelMain sees
> > public?
> > 
> > +vmpstr, What do you think?
> 
> We've decided to make these private precisely because only the channels access
> them. If they are public functions, then anything that knows that this class
is
> a proxy class can access them. That being said, I think we've moved away from
> anything knowing this. That is, we only expose client interfaces so I would be
> OK with making this public (but please double check that nothing can access
this
> inadvertently)

Done. Thanks Vlad!

ProxyMain is seen only by the ChannelMain implementations. The Proxy interface
is implemented privately so the Channels can not call those methods and
LayerTreeHost only sees the Proxy interface.

For ProxyImpl, the LayerTreeHostImpl and Scheduler only see the client
interfaces which are implemented privately, so the Channel classes can not call
these methods. The public ProxyImpl API is visible to the ChannelImpl
implementations only since only those classes will create and hold a reference
to ProxyImpl.

Wez

LGTM w/ nits. *does a little dance* https://codereview.chromium.org/1417053005/diff/340001/cc/trees/proxy_impl.h File cc/trees/proxy_impl.h (right): https://codereview.chromium.org/1417053005/diff/340001/cc/trees/proxy_impl.h#newcode25 cc/trees/proxy_impl.h:25: // ChannelImpl ...

5 years ago (2015-11-26 00:39:47 UTC) #28

Khushal

On 2015/11/26 00:39:47, Wez wrote: > LGTM w/ nits. > > *does a little dance* ...

5 years ago (2015-11-30 18:34:37 UTC) #29

Khushal

On 2015/11/30 18:34:37, Khushal wrote: > On 2015/11/26 00:39:47, Wez wrote: > > LGTM w/ ...

5 years ago (2015-12-03 20:23:13 UTC) #30

vmpstr

This mostly looks good, just some minor comments and questions. brianderson, can you please take ...

5 years ago (2015-12-04 00:25:41 UTC) #31

Khushal

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_test.cc#newcode119 cc/test/layer_tree_test.cc:119: class ProxyImplForTest : public ProxyImpl { On 2015/12/04 00:25:40, ...

5 years ago (2015-12-04 22:45:23 UTC) #32

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:119: class ProxyImplForTest : public ProxyImpl {
On 2015/12/04 00:25:40, vmpstr wrote:
> Maybe just TestProxyImpl?

I kept it consistent with the current naming scheme for
SingleThreadProxyForTest. If we are renaming this, then how about XForTesting
instead for all classes here, to match the LayerTreeHostForTesting, etc. Looks
like generally inherited classes for tests in cc code are suffixed with
ForTesting.

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:261: ProxyImplForTest(TestHooks* test_hooks,
On 2015/12/04 00:25:40, vmpstr wrote:
> Can you move this to the top of the private block

Done.

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:321: TestHooks* test_hooks_;
On 2015/12/04 00:25:40, vmpstr wrote:
> Can you put functions before variables

Done.

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:771:
proxy_main->SetChannel(std::move(threaded_channel));
On 2015/12/04 00:25:40, vmpstr wrote:
> Can you inline the creation in the function? It doesn't really matter, but I
> feel like it would look better:
> 
> proxy_main->SetChannel(ThreadedChannelForTest::Create(test_hooks,
> proxy_main.get(), task_runner_provider.get()));

Done.

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:1228: ProxyImpl* LayerTreeTest::GetProxyImpl() const
{
On 2015/12/04 00:25:40, vmpstr wrote:
> Should this return ProxyImplForTest, which can also expose whatever
> functionality is needed for tests (such as some getters that are used in this
> patch).

Sure. It seemed to me that the test classes in this file were meant to be an
internal detail of the LayerTreeTest, which is why they are not declared in the
header either.

If we are exposing ProxyImplForTest to all the LayerTreeTests, should we do this
for ProxyMainForTest as well?

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_proxy.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_proxy.cc:294:
GetProxyImpl()->GetNextCommitWaitsForActivationForTesting());
On 2015/12/04 00:25:40, vmpstr wrote:
> These can be exposed by the TestPRoxyImpl.

Will do.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/12/04 00:25:40, vmpstr wrote:
> I assume this is mostly a copy of the thread proxy?

Yes. This has been moved from ThreadProxy as-is.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:37: // Virtual for testing.
On 2015/12/04 00:25:40, vmpstr wrote:
> Do all of these need to be virtual for testing?

We want to make sure that all channel implementations actually route the calls
between the main and the impl side. We could run those tests by verifying the
effect of a call but the intent was to have them as smoke tests, independent of
the internal details of ProxyMain/Impl. So these needed to be made virtual to
let TestHooks know about each call in the tests.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:67: // protected for testing.
On 2015/12/04 00:25:40, vmpstr wrote:
> If we're going to expose more variables and if the only override for this is
in
> test, then I would be ok with making the rest of the functionality here
> protected instead of private as well.

Since the variables and all the methods will be exposed through
ProxyImplForTest, we can keep this all private and just have that as the only
friend class.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_main.cc
File cc/trees/proxy_main.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/12/04 00:25:41, vmpstr wrote:
> Again, this is mostly thread proxy? (I skimmed it, but it's a lot to review if
> it's new code). 

Yup. All the same code. :)

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_main.h
File cc/trees/proxy_main.h (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_main.h#...
cc/trees/proxy_main.h:51: // Virtual for testing.
On 2015/12/04 00:25:41, vmpstr wrote:
> All of them? (I'm genuinely asking :) ) 

The channel tests verify that each call reaches the other side.
I hope I didn't go over-board with this. :P

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_main.h#...
cc/trees/proxy_main.h:87: friend class ProxyMainForTest;
On 2015/12/04 00:25:41, vmpstr wrote:
> If this is the only override, then maybe just make it all protected.

I think having just the test class as friend class would make it more obvious
that only the tests need special access to them.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:14:
ThreadedChannel::MainThreadOnly::MainThreadOnly(ProxyMain* proxy_main)
On 2015/12/04 00:25:41, vmpstr wrote:
> Here, I don't think we have any particular style guide, but I personally put
> implementation of inner classes after the implementation of the outer class :)
> It's up to you though.

Done. ProxyImpl has the same order so keeps it consistent too!

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:314: impl().proxy_impl_weak_factory =
make_scoped_ptr(
On 2015/12/04 00:25:41, vmpstr wrote:
> I still find this a bit awkward...

We need it only to ensure thread safety in this class actually. We can use the
raw ProxyImpl ptr to do the post tasks, since the shutdown blocks the main
thread so any tasks calling ProxyImpl on the impl thread will be executed before
it is destroyed. Using the CompositorThreadOnly struct makes it obvious in this
class that ProxyImpl should never be called directly from the main thread.

So its probably better to keep it in this class rather than adding it to
ProxyImpl.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.h (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
cc/trees/threaded_channel.h:162: scoped_ptr<base::WeakPtrFactory<ProxyImpl>>
proxy_impl_weak_factory;
On 2015/12/04 00:25:41, vmpstr wrote:
> Can you leave a comment here explaining that this is initialized after
> construction (hence scoped ptr to weak ptr factor) and also explaining why the
> weak ptr factory is here and not in proxy impl.

Done.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
cc/trees/threaded_channel.h:171: explicit
CompositorThreadOnly(base::WeakPtr<ProxyMain> proxy_main_weak_ptr);
On 2015/12/04 00:25:41, vmpstr wrote:
> Move these to the top please

Done.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
cc/trees/threaded_channel.h:194: const MainThreadOnly& main() const;
On 2015/12/04 00:25:41, vmpstr wrote:
> I understand the logical grouping, but I think we should still have functions
> before variables. It makes it easier to find functions.

Done.

vmpstr

lgtm % brianderson https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_test.cc#newcode119 cc/test/layer_tree_test.cc:119: class ProxyImplForTest : public ProxyImpl { ...

5 years ago (2015-12-07 22:14:42 UTC) #33

lgtm % brianderson

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:119: class ProxyImplForTest : public ProxyImpl {
On 2015/12/04 22:45:22, Khushal wrote:
> On 2015/12/04 00:25:40, vmpstr wrote:
> > Maybe just TestProxyImpl?
> 
> I kept it consistent with the current naming scheme for
> SingleThreadProxyForTest. If we are renaming this, then how about XForTesting
> instead for all classes here, to match the LayerTreeHostForTesting, etc. Looks
> like generally inherited classes for tests in cc code are suffixed with
> ForTesting.

Ok, let's keep it as it is for now. There's way to make FakeBlah, TestBlah, and
BlahForTesting to really decide and change in this CL.

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:1228: ProxyImpl* LayerTreeTest::GetProxyImpl() const
{
On 2015/12/04 22:45:22, Khushal wrote:
> On 2015/12/04 00:25:40, vmpstr wrote:
> > Should this return ProxyImplForTest, which can also expose whatever
> > functionality is needed for tests (such as some getters that are used in
this
> > patch).
> 
> Sure. It seemed to me that the test classes in this file were meant to be an
> internal detail of the LayerTreeTest, which is why they are not declared in
the
> header either.
> 
> If we are exposing ProxyImplForTest to all the LayerTreeTests, should we do
this
> for ProxyMainForTest as well?

If that's helpful, we can do that. It's usually much better to expose an
internal test-only class in test-only code than to write test-only functions in
non-test-code. Especially if the test-only class is only exposing extra getters,
nothing more.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:37: // Virtual for testing.
On 2015/12/04 22:45:22, Khushal wrote:
> On 2015/12/04 00:25:40, vmpstr wrote:
> > Do all of these need to be virtual for testing?
> 
> We want to make sure that all channel implementations actually route the calls
> between the main and the impl side. We could run those tests by verifying the
> effect of a call but the intent was to have them as smoke tests, independent
of
> the internal details of ProxyMain/Impl. So these needed to be made virtual to
> let TestHooks know about each call in the tests.

Sounds good. Thanks for explaining.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:67: // protected for testing.
On 2015/12/04 22:45:22, Khushal wrote:
> On 2015/12/04 00:25:40, vmpstr wrote:
> > If we're going to expose more variables and if the only override for this is
> in
> > test, then I would be ok with making the rest of the functionality here
> > protected instead of private as well.
> 
> Since the variables and all the methods will be exposed through
> ProxyImplForTest, we can keep this all private and just have that as the only
> friend class.

I was thinking along the lines of someone writing more tests somewhere and
writing their own version of FakeProxyImpl or something... In that case, making
everything protected seems kind of better.

Since we don't have other uses though, it's your call.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:314: impl().proxy_impl_weak_factory =
make_scoped_ptr(
On 2015/12/04 22:45:22, Khushal wrote:
> On 2015/12/04 00:25:41, vmpstr wrote:
> > I still find this a bit awkward...
> 
> We need it only to ensure thread safety in this class actually. We can use the
> raw ProxyImpl ptr to do the post tasks, since the shutdown blocks the main
> thread so any tasks calling ProxyImpl on the impl thread will be executed
before
> it is destroyed. Using the CompositorThreadOnly struct makes it obvious in
this
> class that ProxyImpl should never be called directly from the main thread.
> 
> So its probably better to keep it in this class rather than adding it to
> ProxyImpl.

I understand the reason, it still seems a bit awkward. I don't have a better
suggestion, unfortunately. I mean in STL, weak_ptrs are kind of different in
that you create them out of shared ptrs, and you can create shared ptrs out of
unique ptrs, which avoids the whole factory thing.

I guess now that I write that down, this sort of mimics that behavior.

Khushal

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_test.cc#newcode1228 cc/test/layer_tree_test.cc:1228: ProxyImpl* LayerTreeTest::GetProxyImpl() const { On 2015/12/07 22:14:42, vmpstr wrote: ...

5 years ago (2015-12-08 02:19:35 UTC) #34

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:1228: ProxyImpl* LayerTreeTest::GetProxyImpl() const
{
On 2015/12/07 22:14:42, vmpstr wrote:
> On 2015/12/04 22:45:22, Khushal wrote:
> > On 2015/12/04 00:25:40, vmpstr wrote:
> > > Should this return ProxyImplForTest, which can also expose whatever
> > > functionality is needed for tests (such as some getters that are used in
> this
> > > patch).
> > 
> > Sure. It seemed to me that the test classes in this file were meant to be an
> > internal detail of the LayerTreeTest, which is why they are not declared in
> the
> > header either.
> > 
> > If we are exposing ProxyImplForTest to all the LayerTreeTests, should we do
> this
> > for ProxyMainForTest as well?
> 
> If that's helpful, we can do that. It's usually much better to expose an
> internal test-only class in test-only code than to write test-only functions
in
> non-test-code. Especially if the test-only class is only exposing extra
getters,
> nothing more.

Done.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:67: // protected for testing.
On 2015/12/07 22:14:42, vmpstr wrote:
> On 2015/12/04 22:45:22, Khushal wrote:
> > On 2015/12/04 00:25:40, vmpstr wrote:
> > > If we're going to expose more variables and if the only override for this
is
> > in
> > > test, then I would be ok with making the rest of the functionality here
> > > protected instead of private as well.
> > 
> > Since the variables and all the methods will be exposed through
> > ProxyImplForTest, we can keep this all private and just have that as the
only
> > friend class.
> 
> I was thinking along the lines of someone writing more tests somewhere and
> writing their own version of FakeProxyImpl or something... In that case,
making
> everything protected seems kind of better.
> 
> Since we don't have other uses though, it's your call.

Ah, makes sense. I'll keep it private for now. We can change it if we end up
adding more test classes.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:314: impl().proxy_impl_weak_factory =
make_scoped_ptr(
On 2015/12/07 22:14:42, vmpstr wrote:
> On 2015/12/04 22:45:22, Khushal wrote:
> > On 2015/12/04 00:25:41, vmpstr wrote:
> > > I still find this a bit awkward...
> > 
> > We need it only to ensure thread safety in this class actually. We can use
the
> > raw ProxyImpl ptr to do the post tasks, since the shutdown blocks the main
> > thread so any tasks calling ProxyImpl on the impl thread will be executed
> before
> > it is destroyed. Using the CompositorThreadOnly struct makes it obvious in
> this
> > class that ProxyImpl should never be called directly from the main thread.
> > 
> > So its probably better to keep it in this class rather than adding it to
> > ProxyImpl.
> 
> I understand the reason, it still seems a bit awkward. I don't have a better
> suggestion, unfortunately. I mean in STL, weak_ptrs are kind of different in
> that you create them out of shared ptrs, and you can create shared ptrs out of
> unique ptrs, which avoids the whole factory thing.
> 
> I guess now that I write that down, this sort of mimics that behavior. 

Exactly. I get that it still does seem a bit awkward when you see it though...

Khushal

Description was changed from ========== cc: Split ThreadProxy into ProxyMain and ProxyImpl This is the ...

5 years ago (2015-12-08 03:15:43 UTC) #35

Khushal

khushalsagar@chromium.org changed reviewers: + ernstm@chromium.org, sullivan@chromium.org

5 years ago (2015-12-08 03:15:44 UTC) #36

Khushal

+ernstm, could you review the change in cc/debug. +sullivan, could you review the change in ...

5 years ago (2015-12-08 18:19:15 UTC) #37

vmpstr

vmpstr@chromium.org changed reviewers: + nduca@chromium.org - ernstm@chromium.org

5 years ago (2015-12-08 18:57:47 UTC) #38

vmpstr

On 2015/12/08 18:19:15, Khushal wrote: > +ernstm, could you review the change in cc/debug. > ...

5 years ago (2015-12-08 21:20:31 UTC) #39

Khushal

On 2015/12/08 21:20:31, vmpstr wrote: > On 2015/12/08 18:19:15, Khushal wrote: > > +ernstm, could ...

5 years ago (2015-12-08 21:22:15 UTC) #41

sullivan

sullivan@chromium.org changed reviewers: + nednguyen@chromium.org

5 years ago (2015-12-08 21:24:21 UTC) #42

nednguyen

On 2015/12/08 21:24:22, sullivan wrote: > Adding nednguyen as telemetry reviewer Can you move telemetry/ ...

5 years ago (2015-12-08 21:31:08 UTC) #44

Khushal

khushalsagar@chromium.org changed reviewers: + nednguyen@google.com - nednguyen@chromium.org

5 years ago (2015-12-08 21:32:17 UTC) #45

nednguyen

+Sami for rendering frame change (frame_queuing_durations) metrics. Talked offline, if we don't do any change ...

5 years ago (2015-12-08 21:55:36 UTC) #47

Khushal

On 2015/12/08 21:55:36, nednguyen wrote: > +Sami for rendering frame change (frame_queuing_durations) metrics. > > ...

5 years ago (2015-12-08 22:06:51 UTC) #48

nednguyen

On 2015/12/08 22:06:51, Khushal wrote: > On 2015/12/08 21:55:36, nednguyen wrote: > > +Sami for ...

5 years ago (2015-12-08 22:30:24 UTC) #49

vmpstr

On 2015/12/08 22:30:24, nednguyen wrote: > On 2015/12/08 22:06:51, Khushal wrote: > > On 2015/12/08 ...

5 years ago (2015-12-08 22:36:30 UTC) #50

brianderson

lgtm, pending the benchmark issues. All my comments are nits. I imagine this patch is ...

5 years ago (2015-12-09 02:53:30 UTC) #51

lgtm, pending the benchmark issues. All my comments are nits. I imagine this
patch is a pain to rebase, so feel free to land first and address nits with a
quick followup.

Note that skyostil is out and wont get to review this patch until after the new
year.

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:314: impl().proxy_impl_weak_factory =
make_scoped_ptr(
On 2015/12/08 02:19:35, Khushal wrote:
> On 2015/12/07 22:14:42, vmpstr wrote:
> > On 2015/12/04 22:45:22, Khushal wrote:
> > > On 2015/12/04 00:25:41, vmpstr wrote:
> > > > I still find this a bit awkward...
> > > 
> > > We need it only to ensure thread safety in this class actually. We can use
> the
> > > raw ProxyImpl ptr to do the post tasks, since the shutdown blocks the main
> > > thread so any tasks calling ProxyImpl on the impl thread will be executed
> > before
> > > it is destroyed. Using the CompositorThreadOnly struct makes it obvious in
> > this
> > > class that ProxyImpl should never be called directly from the main thread.
> > > 
> > > So its probably better to keep it in this class rather than adding it to
> > > ProxyImpl.
> > 
> > I understand the reason, it still seems a bit awkward. I don't have a better
> > suggestion, unfortunately. I mean in STL, weak_ptrs are kind of different in
> > that you create them out of shared ptrs, and you can create shared ptrs out
of
> > unique ptrs, which avoids the whole factory thing.
> > 
> > I guess now that I write that down, this sort of mimics that behavior. 
> 
> Exactly. I get that it still does seem a bit awkward when you see it though...

This works as-is, so I don't care about changing it before landing, but to bike
shed:

ProxyImpl::smoothness_priority_expiration_notifier_ also vends WeakPtrs of
ProxyImpl indirectly.

Would it make sense to merge the WeakPtrFactorys in DelayedUniqueNotifier and
ThreadedChannelImpl into a single factory owned by ProxyImpl?

Relatedly, it looks like DelayedUniqueNotifier vends WeakPtrs for every PostTask
instead of reusing a single WeakPtr like the code in this patch does.

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:148: ThreadedChannel::Create(proxy_main.get(),
task_runner_provider_.get()));
LTH shouldn't have to know about or include ThreadedChannel. Can you move the
SetChannel logic to ProxyMain::Create and ProxyMainForTest::Create?

Then, to bikeshed:
In those Create functions, create the channel first and pass it into the
ProxyMain constructor. Remove ProxyMain::SetChannel and instead call
ThreadedChannel::SetProxyMain or SetClient. I think it's a more common pattern
in cc for the owner to call a SetClient type method than the other way around.
That will mess with the base::WeakPtrFactory<ProxyMain> proxy_main_weak_factory.
You'll have to make it a scoped_ptr or move the factory ownership to ProxyMain.
I personally like if both ProxyMain and ProxyImpl own their own factories, which
also seems to be a more common pattern.

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:31: #include
"gpu/command_buffer/client/gles2_interface.h"
Are all these includes still needed for both proxy_impl and proxy_main now?

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:158: bool did_commit_after_animating_;
Looks like this is unused. Delete?

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_main.h
File cc/trees/proxy_main.h (right):

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_main.h#...
cc/trees/proxy_main.h:148: RendererCapabilities
renderer_capabilities_main_thread_copy_;
Probably doesn't need the "_main_thread_copy_" suffix anymore.

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_main.h#...
cc/trees/proxy_main.h:152: scoped_ptr<BeginFrameSource>
external_begin_frame_source_;
Looks like it will be easy to remove this temporary member variable after your
patch. Can you add a TODO? (Or just remove it if it's not too disruptive.)
https://code.google.com/p/chromium/issues/detail?id=567930

Khushal

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_channel.cc File cc/trees/threaded_channel.cc (right): https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_channel.cc#newcode314 cc/trees/threaded_channel.cc:314: impl().proxy_impl_weak_factory = make_scoped_ptr( On 2015/12/09 02:53:30, brianderson wrote: > ...

5 years ago (2015-12-09 08:18:56 UTC) #52

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
File cc/trees/threaded_channel.cc (right):

https://codereview.chromium.org/1417053005/diff/360001/cc/trees/threaded_chan...
cc/trees/threaded_channel.cc:314: impl().proxy_impl_weak_factory =
make_scoped_ptr(
On 2015/12/09 02:53:30, brianderson wrote:
> On 2015/12/08 02:19:35, Khushal wrote:
> > On 2015/12/07 22:14:42, vmpstr wrote:
> > > On 2015/12/04 22:45:22, Khushal wrote:
> > > > On 2015/12/04 00:25:41, vmpstr wrote:
> > > > > I still find this a bit awkward...
> > > > 
> > > > We need it only to ensure thread safety in this class actually. We can
use
> > the
> > > > raw ProxyImpl ptr to do the post tasks, since the shutdown blocks the
main
> > > > thread so any tasks calling ProxyImpl on the impl thread will be
executed
> > > before
> > > > it is destroyed. Using the CompositorThreadOnly struct makes it obvious
in
> > > this
> > > > class that ProxyImpl should never be called directly from the main
thread.
> > > > 
> > > > So its probably better to keep it in this class rather than adding it to
> > > > ProxyImpl.
> > > 
> > > I understand the reason, it still seems a bit awkward. I don't have a
better
> > > suggestion, unfortunately. I mean in STL, weak_ptrs are kind of different
in
> > > that you create them out of shared ptrs, and you can create shared ptrs
out
> of
> > > unique ptrs, which avoids the whole factory thing.
> > > 
> > > I guess now that I write that down, this sort of mimics that behavior. 
> > 
> > Exactly. I get that it still does seem a bit awkward when you see it
though...
> 
> This works as-is, so I don't care about changing it before landing, but to
bike
> shed:
> 
> ProxyImpl::smoothness_priority_expiration_notifier_ also vends WeakPtrs of
> ProxyImpl indirectly.
> 
> Would it make sense to merge the WeakPtrFactorys in DelayedUniqueNotifier and
> ThreadedChannelImpl into a single factory owned by ProxyImpl?
> 

I think the DelayedUniqueNotifier would still need its own weak factory to post
tasks on itself. Right now it is designed to take a closure and schedule/cancel
runs on it. May be we can take up how that should be changed in a bug?

> Relatedly, it looks like DelayedUniqueNotifier vends WeakPtrs for every
PostTask
> instead of reusing a single WeakPtr like the code in this patch does. 

Here we explicitly wanted to avoid calling weak_factory.GetWeakPtr() from
multiple threads, so instead we set the weak ptr for the impl thread to use
during construction, before the impl side is initialized, and let it reuse this
pointer.

The DelayedUniqueNotifier always runs on the same thread, so I guess having it
vend weak ptrs for each task should not be a problem.

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:148: ThreadedChannel::Create(proxy_main.get(),
task_runner_provider_.get()));
On 2015/12/09 02:53:30, brianderson wrote:
> LTH shouldn't have to know about or include ThreadedChannel. Can you move the
> SetChannel logic to ProxyMain::Create and ProxyMainForTest::Create?
> 

Done.

> Then, to bikeshed:
> In those Create functions, create the channel first and pass it into the
> ProxyMain constructor. Remove ProxyMain::SetChannel and instead call

I have made SetChannel private. I think this would keep it consistent with the
initialization pattern for the LayerTreeHost as well, where we build the proxy
and then pass it with LayerTreeHost::InitializeProxy.

> ThreadedChannel::SetProxyMain or SetClient. I think it's a more common pattern
> in cc for the owner to call a SetClient type method than the other way around.
> That will mess with the base::WeakPtrFactory<ProxyMain>
proxy_main_weak_factory.
> You'll have to make it a scoped_ptr or move the factory ownership to
ProxyMain.
> I personally like if both ProxyMain and ProxyImpl own their own factories,
which
> also seems to be a more common pattern.

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_impl.cc...
cc/trees/proxy_impl.cc:31: #include
"gpu/command_buffer/client/gles2_interface.h"
On 2015/12/09 02:53:30, brianderson wrote:
> Are all these includes still needed for both proxy_impl and proxy_main now?

I guess not. I have fixed the includes for both.

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_impl.h
File cc/trees/proxy_impl.h (right):

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_impl.h#...
cc/trees/proxy_impl.h:158: bool did_commit_after_animating_;
On 2015/12/09 02:53:30, brianderson wrote:
> Looks like this is unused. Delete?

Done.

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_main.h
File cc/trees/proxy_main.h (right):

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_main.h#...
cc/trees/proxy_main.h:148: RendererCapabilities
renderer_capabilities_main_thread_copy_;
On 2015/12/09 02:53:30, brianderson wrote:
> Probably doesn't need the "_main_thread_copy_" suffix anymore.

Done.

https://codereview.chromium.org/1417053005/diff/420001/cc/trees/proxy_main.h#...
cc/trees/proxy_main.h:152: scoped_ptr<BeginFrameSource>
external_begin_frame_source_;
On 2015/12/09 02:53:30, brianderson wrote:
> Looks like it will be easy to remove this temporary member variable after your
> patch. Can you add a TODO? (Or just remove it if it's not too disruptive.)
> https://code.google.com/p/chromium/issues/detail?id=567930

To remove the temporary variable, the LayerTreeHost will need to pass it using
Proxy::Start. I've added a comment about this on the bug and the TODO here. Will
add this in a follow up patch.

Khushal

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org, vmpstr@chromium.org, brianderson@chromium.org Link ...

5 years ago (2015-12-09 08:19:19 UTC) #54

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417053005/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417053005/460001

5 years ago (2015-12-09 08:20:22 UTC) #55

commit-bot: I haz the power

Description was changed from ========== cc: Split ThreadProxy into ProxyMain and ProxyImpl This is the ...

5 years ago (2015-12-09 10:30:29 UTC) #56

commit-bot: I haz the power

Description was changed from ========== cc: Split ThreadProxy into ProxyMain and ProxyImpl This is the ...

5 years ago (2015-12-09 10:31:30 UTC) #58

commit-bot: I haz the power

Patchset 24 (id:??) landed as https://crrev.com/0a226af5cb4c5ca5b3bac3e3d857601cc356dc33 Cr-Commit-Position: refs/heads/master@{#364034}

5 years ago (2015-12-09 10:31:31 UTC) #59

engedy

5 years ago (2015-12-09 13:33:04 UTC) #60

Message was sent while issue was closed.

A revert of this CL (patchset #24 id:460001) has been created in
https://codereview.chromium.org/1506023008/ by engedy@chromium.org.

The reason for reverting is: Quite likely causing substantial flakiness in
cc_unittests on 'Win7 Tests (dbg)(1)'.

LayerTreeHostCopyRequestCompletionCausesCommit.RunMultiThread_DirectRenderer
(run #1):
[ RUN      ]
LayerTreeHostCopyRequestCompletionCausesCommit.RunMultiThread_DirectRenderer
c:uild\slave\win_builder__dbg_uild\src\cc	rees\layer_tree_host_unittest_copyrequest.cc(222):
error: Value of: callback_count_
  Actual: 0
Expected: 1
[  FAILED  ]
LayerTreeHostCopyRequestCompletionCausesCommit.RunMultiThread_DirectRenderer (23
ms)

See:
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....

Issue 1417053005: cc: Split ThreadProxy into ProxyMain and ProxyImpl (Closed)

Description

Patch Set 1 #

Patch Set 2 : Rebase. #

Patch Set 3 : #

Patch Set 4 : Rebase. #

Patch Set 5 : Address comments. #

Patch Set 6 : Minor comment fix. #

Patch Set 7 : Rebase and minor comment updates. #

Patch Set 8 : Addressing comments. #

Patch Set 9 : Update name in telemetry. #

Patch Set 10 : Rebase. #

Patch Set 11 : Update initialization and shutdown implementation. #

Patch Set 12 : Move weak ptr factories to ThreadedChannel #

Patch Set 13 : Remove IsInitialized from channel interface. #

Patch Set 14 : Change ThreadedChannel::GetProxyImpl to GetProxyImplForTesting #

Patch Set 15 : Addressed Wez's comments. #

Patch Set 16 : Rebase. #

Patch Set 17 : Addressing comments. #

Patch Set 18 : Update ProxyImpl and ProxyMain class structure. #

Patch Set 19 : Address comments from #18. #

Patch Set 20 : Addressing comments. #

Patch Set 21 : Rebase. #

Patch Set 22 : Addressed vmpstr@'s comments. #

Patch Set 23 : Rebase. #

Patch Set 24 : Addressing brianderson@'s comments, remove benchmark name change. #

Messages