base: Mark MessageLoop::Post*Task API deprecated

base: Mark MessageLoop::Post*Task API deprecated

This is the first step in removing the direct MessageLoop task posting
API in favor of task_runner() and the TaskRunner APIs. See the design
doc for details[1].

BUG=465354
[1] https://docs.google.com/a/chromium.org/document/d/1qxdh2I61_aB_Uzh1QgNqvdWFBCL_E65G2smoSySw7KU/edit#heading=h.eqrpa9q6bsq5

Committed: https://crrev.com/c8c18e9f1d3ac1033f9ca308143fc3b545475ebe
Cr-Commit-Position: refs/heads/master@{#321674}

[Sami, 2015-03-16 20:27:35]

skyostil@chromium.org changed reviewers:
+ danakj@chromium.org, jam@chromium.org, sievers@chromium.org

[sadrul, 2015-03-16 20:31:28]

drive-by! Could the impls of MessageLoop::PostTask etc. could also be changed to
just call into 'task_runner()->PostTask' etc. (since it looks like it would be
functionally the same with the current code)?

[Sami, 2015-03-16 20:34:46]

On 2015/03/16 20:31:28, sadrul wrote:
> drive-by! Could the impls of MessageLoop::PostTask etc. could also be changed
to
> just call into 'task_runner()->PostTask' etc. (since it looks like it would be
> functionally the same with the current code)?

Great idea, done.

[no sievers, 2015-03-16 22:43:23]

lgtm

[Sami, 2015-03-18 18:49:18]

Thanks Daniel. John and/or Dana, do you agree?

[danakj, 2015-03-19 21:43:21]

LGTM if this isn't bad for perf.

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.cc:279: task_runner()->PostTask(from_here, task);
Note: This changes these into virtual function calls. I hope that won't be bad
for perf reasons.

[no sievers, 2015-03-19 21:47:24]

sievers@chromium.org changed reviewers:
+ wez@chromium.org

[no sievers, 2015-03-19 21:50:35]

+wez to confirm that it's ok to make the implementation of ML::Post*Task() going
through the virtual TaskRunner interface.

I was looking at the comments in base/single_thread_task_runner.h.

Does anything rely on MessageLoop and TaskRunner going to different queues?

If not, we'd like to route everything through the TaskRunner Post interfaces,
which would make it easier to override the behavior for the renderer/blink
scheduler without subclassing MessageLoop (which nobody really liked).

[Wez, 2015-03-20 04:09:33]

On 2015/03/19 21:50:35, sievers wrote:
> +wez to confirm that it's ok to make the implementation of ML::Post*Task()
going
> through the virtual TaskRunner interface.
> 
> I was looking at the comments in base/single_thread_task_runner.h.
> 
> Does anything rely on MessageLoop and TaskRunner going to different queues?
> 
> If not, we'd like to route everything through the TaskRunner Post interfaces,
> which would make it easier to override the behavior for the renderer/blink
> scheduler without subclassing MessageLoop (which nobody really liked).

In the current impl everything already goes through the same incoming queue -
see the MessageLoopProxyImpl instantiation in MessageLoop, which is passed a
reference to the ML's internal incoming work queue.

You might check w/ jar@ whether he can see any performance issues due to always
jumping through the virtual function table.

[Wez, 2015-03-20 04:13:01]

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.cc:278: DCHECK(!task.is_null()) <<
from_here.ToString();
These DCHECKs should be in the TaskRunner impl now, not in these boilerplate
methods.

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.h:158: // NOTE: Deprecated; prefer task_runner()
and the TaskRunner interfaces.
Is there a cleanup bug filed to update call-sites and eventually remove these?

[Sami, 2015-03-20 17:16:56]

Thanks all.

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.cc:278: DCHECK(!task.is_null()) <<
from_here.ToString();
On 2015/03/20 04:13:01, Wez wrote:
> These DCHECKs should be in the TaskRunner impl now, not in these boilerplate
> methods.

Good point, done. (I checked that the same DCHECKs exist there.)

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.cc:279: task_runner()->PostTask(from_here, task);
On 2015/03/19 21:43:20, danakj wrote:
> Note: This changes these into virtual function calls. I hope that won't be bad
> for perf reasons.

Good point. I checked this by hacking TaskPerfTest to call into the MessageLoop
on a Nexus 6 and found a 15% regression (http://pastebin.com/4WRhBPJA). However,
turns out that was because calling task_runner() causes refptr churn. After I
changed this to access message_loop_proxy_ directly the regression went away.

(BTW, I decided not to land the TaskPerfTest changes since we're about to
deprecate this interface.)

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.h:158: // NOTE: Deprecated; prefer task_runner()
and the TaskRunner interfaces.
On 2015/03/20 04:13:01, Wez wrote:
> Is there a cleanup bug filed to update call-sites and eventually remove these?

I was going to do as part of the linked bug. I've got a mostly working clang
tool for it: https://codereview.chromium.org/1010073002/

[danakj, 2015-03-20 17:19:13]

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.h:158: // NOTE: Deprecated; prefer task_runner()
and the TaskRunner interfaces.
On 2015/03/20 17:16:56, Sami wrote:
> On 2015/03/20 04:13:01, Wez wrote:
> > Is there a cleanup bug filed to update call-sites and eventually remove
these?
> 
> I was going to do as part of the linked bug. I've got a mostly working clang
> tool for it: https://codereview.chromium.org/1010073002/

Linking to bugs in TODO is a good practice.

https://codereview.chromium.org/1013813003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1013813003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.cc:278: message_loop_proxy_->PostTask(from_here,
task);
Maybe you can comment to connect this to task_runner() then?

[Sami, 2015-03-20 17:36:27]

> Linking to bugs in TODO is a good practice.

Good idea, added a TODO with a link to the header.

https://codereview.chromium.org/1013813003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1013813003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop.cc:278: message_loop_proxy_->PostTask(from_here,
task);
On 2015/03/20 17:19:13, danakj wrote:
> Maybe you can comment to connect this to task_runner() then?

Clarified offline: message_loop_proxy_ is going to get renamed to task_runner_
once we get rid of MessageLoopProxy, so this code won't need any special
treatment (beyond the renaming.)

[Wez, 2015-03-20 18:47:03]

On 2015/03/20 17:16:56, Sami wrote:
> Thanks all.
> 
>
https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
> File base/message_loop/message_loop.cc (right):
> 
>
https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
> base/message_loop/message_loop.cc:278: DCHECK(!task.is_null()) <<
> from_here.ToString();
> On 2015/03/20 04:13:01, Wez wrote:
> > These DCHECKs should be in the TaskRunner impl now, not in these boilerplate
> > methods.
> 
> Good point, done. (I checked that the same DCHECKs exist there.)
> 
>
https://codereview.chromium.org/1013813003/diff/20001/base/message_loop/messa...
> base/message_loop/message_loop.cc:279: task_runner()->PostTask(from_here,
task);
> On 2015/03/19 21:43:20, danakj wrote:
> > Note: This changes these into virtual function calls. I hope that won't be
bad
> > for perf reasons.
> 
> Good point. I checked this by hacking TaskPerfTest to call into the
MessageLoop
> on a Nexus 6 and found a 15% regression (http://pastebin.com/4WRhBPJA).
However,
> turns out that was because calling task_runner() causes refptr churn. After I
> changed this to access message_loop_proxy_ directly the regression went away.

Looking at the definition of task_runner(), that seems to be because it returns
scoped_refptr<> rather than const scoped_refptr<>& - changing the type of the
return value should let you call task_runner() internally without ref-count
churn, here and in other call-sites.

[Sami, 2015-03-20 20:07:57]

On 2015/03/20 18:47:03, Wez wrote:
> Looking at the definition of task_runner(), that seems to be because it
returns
> scoped_refptr<> rather than const scoped_refptr<>& - changing the type of the
> return value should let you call task_runner() internally without ref-count
> churn, here and in other call-sites.

Unfortunately that doesn't work because message_loop_proxy_'s type
(scoped_refptr<internal::MessageLoopProxyImpl>) matches neither of
scoped_refptr<MessageLoopProxy> or scoped_refptr<SingleThreadTaskRunner> so we
can't return a direct reference to it. Once we remove MessageLoopProxy the types
will match so I've added a TODO about that here.

[Wez, 2015-03-20 20:40:45]

On 2015/03/20 20:07:57, Sami wrote:
> On 2015/03/20 18:47:03, Wez wrote:
> > Looking at the definition of task_runner(), that seems to be because it
> returns
> > scoped_refptr<> rather than const scoped_refptr<>& - changing the type of
the
> > return value should let you call task_runner() internally without ref-count
> > churn, here and in other call-sites.
> 
> Unfortunately that doesn't work because message_loop_proxy_'s type
> (scoped_refptr<internal::MessageLoopProxyImpl>) matches neither of
> scoped_refptr<MessageLoopProxy> or scoped_refptr<SingleThreadTaskRunner> so we
> can't return a direct reference to it. Once we remove MessageLoopProxy the
types
> will match so I've added a TODO about that here.

Can't you just make |message_loop_proxy_| a
scoped_refptr<SingleThreadTaskRunner>?

The fact that it holds a MessageLoopProxyImpl doesn't seem to be required by the
MessageLoop itself.

[Sami, 2015-03-20 20:44:44]

On 2015/03/20 20:40:45, Wez wrote:
> On 2015/03/20 20:07:57, Sami wrote:
> > On 2015/03/20 18:47:03, Wez wrote:
> > > Looking at the definition of task_runner(), that seems to be because it
> > returns
> > > scoped_refptr<> rather than const scoped_refptr<>& - changing the type of
> the
> > > return value should let you call task_runner() internally without
ref-count
> > > churn, here and in other call-sites.
> > 
> > Unfortunately that doesn't work because message_loop_proxy_'s type
> > (scoped_refptr<internal::MessageLoopProxyImpl>) matches neither of
> > scoped_refptr<MessageLoopProxy> or scoped_refptr<SingleThreadTaskRunner> so
we
> > can't return a direct reference to it. Once we remove MessageLoopProxy the
> types
> > will match so I've added a TODO about that here.
> 
> Can't you just make |message_loop_proxy_| a
> scoped_refptr<SingleThreadTaskRunner>?
> 
> The fact that it holds a MessageLoopProxyImpl doesn't seem to be required by
the
> MessageLoop itself.

Almost -- message_loop_proxy() still needs to return a
scoped_refptr<MessageLoopProxy>. I guess I could do a downcast there but that'd
be a little ugly (and also goes a little out of scope for this patch).

[Wez, 2015-03-20 20:57:20]

On 2015/03/20 20:44:44, Sami wrote:
> On 2015/03/20 20:40:45, Wez wrote:
> > On 2015/03/20 20:07:57, Sami wrote:
> > > On 2015/03/20 18:47:03, Wez wrote:
> > > > Looking at the definition of task_runner(), that seems to be because it
> > > returns
> > > > scoped_refptr<> rather than const scoped_refptr<>& - changing the type
of
> > the
> > > > return value should let you call task_runner() internally without
> ref-count
> > > > churn, here and in other call-sites.
> > > 
> > > Unfortunately that doesn't work because message_loop_proxy_'s type
> > > (scoped_refptr<internal::MessageLoopProxyImpl>) matches neither of
> > > scoped_refptr<MessageLoopProxy> or scoped_refptr<SingleThreadTaskRunner>
so
> we
> > > can't return a direct reference to it. Once we remove MessageLoopProxy the
> > types
> > > will match so I've added a TODO about that here.
> > 
> > Can't you just make |message_loop_proxy_| a
> > scoped_refptr<SingleThreadTaskRunner>?
> > 
> > The fact that it holds a MessageLoopProxyImpl doesn't seem to be required by
> the
> > MessageLoop itself.
> 
> Almost -- message_loop_proxy() still needs to return a
> scoped_refptr<MessageLoopProxy>. I guess I could do a downcast there but
that'd
> be a little ugly (and also goes a little out of scope for this patch).

I take it that using a scoped_refptr<MessageLoopProxy> as the member variable
type wouldn't help, then?

Well, anyway, this change basically LGTM - thanks for cleaning up MessageLoop.
:)

[danakj, 2015-03-20 21:27:14]

On Fri, Mar 20, 2015 at 1:07 PM, <skyostil@chromium.org> wrote:

> On 2015/03/20 18:47:03, Wez wrote:
>
>> Looking at the definition of task_runner(), that seems to be because it
>>
> returns
>
>> scoped_refptr<> rather than const scoped_refptr<>& - changing the type of
>> the
>> return value should let you call task_runner() internally without
>> ref-count
>> churn, here and in other call-sites.
>>
>
> Unfortunately that doesn't work because message_loop_proxy_'s type
> (scoped_refptr<internal::MessageLoopProxyImpl>) matches neither of
> scoped_refptr<MessageLoopProxy> or scoped_refptr<SingleThreadTaskRunner>
> so we
> can't return a direct reference to it. Once we remove MessageLoopProxy the
> types
> will match so I've added a TODO about that here.
>

I think you have to cast it

const scoped_refptr<BaseClass>& Foo() { return
scoped_reptr<BaseClass>(sub_class_); }

Or does that return a reference to a temporary?


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

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

[Wez, 2015-03-20 21:34:19]

That form is creating a temporary rather than casting, per-se. Not sure if
you can static_cast instead, for instance, but I'd guess not.
On 20 Mar 2015 14:27, "Dana Jansens" <danakj@chromium.org> wrote:

> On Fri, Mar 20, 2015 at 1:07 PM, <skyostil@chromium.org> wrote:
>
>> On 2015/03/20 18:47:03, Wez wrote:
>>
>>> Looking at the definition of task_runner(), that seems to be because it
>>>
>> returns
>>
>>> scoped_refptr<> rather than const scoped_refptr<>& - changing the type
>>> of the
>>> return value should let you call task_runner() internally without
>>> ref-count
>>> churn, here and in other call-sites.
>>>
>>
>> Unfortunately that doesn't work because message_loop_proxy_'s type
>> (scoped_refptr<internal::MessageLoopProxyImpl>) matches neither of
>> scoped_refptr<MessageLoopProxy> or scoped_refptr<SingleThreadTaskRunner>
>> so we
>> can't return a direct reference to it. Once we remove MessageLoopProxy
>> the types
>> will match so I've added a TODO about that here.
>>
>
> I think you have to cast it
>
> const scoped_refptr<BaseClass>& Foo() { return
> scoped_reptr<BaseClass>(sub_class_); }
>
> Or does that return a reference to a temporary?
>
>
>>
>> https://codereview.chromium.org/1013813003/
>>
>
>

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

[Sami, 2015-03-20 22:53:06]

On 2015/03/20 21:34:19, Wez wrote:
> That form is creating a temporary rather than casting, per-se. Not sure if
> you can static_cast instead, for instance, but I'd guess not.

Right, I think we'd need a reinterpret_cast here since the classes are
unrelated. I'd rather fix this by eliminating the wrong type and making the
problem go away :)

[Sami, 2015-03-20 22:54:09]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-03-20 22:54:09]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1013813003/#ps80001
(title: "Add TODO to task_runner().")

[commit-bot: I haz the power, 2015-03-20 22:55:15]

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

[commit-bot: I haz the power, 2015-03-21 00:57:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-21 00:58:36]

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