Removing NewRunnableMethod() and NewCallback()

[Albert J. Wong (王重傑)]

If you use NewRunnableMethod or NewCallback, this mail concerns you!

We're looking for people to help convert all the NewRunnableMethod() and NewCallback() calls over to base::Bind().  Because these constructs are so widely used. we need all the help we can get!

Luckily, the syntax change should be fairly easy. Recipes for common conversions and a few caveats are listed at the end of this mail.  The general procedure should be:

  1) Pick a file.

  2) Apply recipes below.

  3) Fix compile errors and memory suppressions as necessary.

  4) Rinse. Repeat with next file.

I've created a spreadsheet listing all the files that have constructs needing changes.  If you decide to help out, please put your name next to the file you are working on here (make sure to be signed in with your @chromium.org account):

    http://goo.gl/89Chr

When you are finish with a file, add an "x" in the done column.  I'll periodically go through and shift the finished files down to the bottom of the list.

Note that I am also working on Clang plugin (http://codereview.chromium.org/7886056/) to aide in this, but haven't gotten the kinks worked out yet.  When/if I get that working, I'll send an update.

Let me know if there are any questions or concerns!

-Albert

== NewCallback ==

  1) Callback2<int, double>::Type* callback  ==> base::Callback<void(int, double)> callback;

  2) callback = NewCallback(this, &Foo::func)  ==>  

      callback = base::Bind(&Foo::func, base::Unretained(this));

Be sure to add base::Unretained() around the object that give to Bind() when replacing a NewCallback call.  Otherwise, base::Bind() will take a reference on the object which will change the memory handling behavior.

== NewRunnableMethod() ==

  1) Task* task  ==> base::Closure task

  2) task = NewRunnableMethod(this, &Foo::func, a, b)  ==> 

      task = base::Bind(&Foo::func, this, a, b)

  3) ml->PostTask(FROM_HERE, NewRunnableMethod(this, &Foo::func, a, b)  ==> 

      ml->PostTask(FROM_HERE, base::Bind(&Foo::DoSomething, this, a, b)

IMPORTANT: When using base::Bind() with PostTask, it is vital that all bound arguments are safe to delete from the originalting thread, and the thread that it is being posted onto.  Unlike NewRunnableMethod(), the result of base::Bind() is refcounted, and may be deleted on either thread.  This should not be an issue if the parameters (eg., a and b above) are primitive types, or things like scoped_refptrs.  However, if they are custom, copyable objects, then be careful.

== ScopedRunnableMethodFactory and ScopedCallbackFactory ==

   1) ScopedRunnableMethodFactory<Foo> callback_factory_  ==> WeakPtrFactory<Foo> weak_factory_

   2) callback_factory_.NewRunnableMethod(this, &Foo::func, a, b)  ==>

       base::Bind(&Foo::func, weak_factory_.GetWeakPtr(), a, b)

   3) callback_factory.RevokeAll()  ==>  weak_factory_->InvalidateWeakPtrs()

The ScopedCallbackFactory conversion is almost exactly the same.

Note, you cannot cancel individual callbacks created from the factory yet. If you really need that feature, please let me know.

== Custom Implementations of Task ==

Most of the time people inherit from Task(), they do so in order to carry some context around.  Consider trying to remove the derived class and instead use currying with base::Bind() on a normal function to get the desired behavior.

Feel free to e-mail back to this thread with questions if you can't figure out how to replicate a custom Task class using base::Bind.

== More Info ==

For detail documentation on base::Bind and base::Callback, please see file comments here:

  http://src.chromium.org/viewvc/chrome/trunk/src/base/callback.h?view=markup

  http://src.chromium.org/viewvc/chrome/trunk/src/base/bind_helpers.h?view=markup

The bind_unittest also has lots of examples for how to use this API.

  http://src.chromium.org/viewvc/chrome/trunk/src/base/bind_unittest.cc?view=markup

[John Abd-El-Malek]

Can you update the threading wiki on http://www.chromium.org/developers/design-documents/threading with the new recommended way?

I have been mostly ignoring this work, so I've been using the old way because that's what I'm used for (and I have some stuff I wasn't sure about). A few questions below. If you can also put that in the wiki, that'd be great :)

On Tue, Sep 27, 2011 at 9:37 AM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

If you use NewRunnableMethod or NewCallback, this mail concerns you!

We're looking for people to help convert all the NewRunnableMethod() and NewCallback() calls over to base::Bind().  Because these constructs are so widely used. we need all the help we can get!

Luckily, the syntax change should be fairly easy. Recipes for common conversions and a few caveats are listed at the end of this mail.  The general procedure should be:

  1) Pick a file.

  2) Apply recipes below.

  3) Fix compile errors and memory suppressions as necessary.

  4) Rinse. Repeat with next file.

I've created a spreadsheet listing all the files that have constructs needing changes.  If you decide to help out, please put your name next to the file you are working on here (make sure to be signed in with your @chromium.org account):

    http://goo.gl/89Chr

When you are finish with a file, add an "x" in the done column.  I'll periodically go through and shift the finished files down to the bottom of the list.

Note that I am also working on Clang plugin (http://codereview.chromium.org/7886056/) to aide in this, but haven't gotten the kinks worked out yet.  When/if I get that working, I'll send an update.

Let me know if there are any questions or concerns!

-Albert

== NewCallback ==

  1) Callback2<int, double>::Type* callback  ==> base::Callback<void(int, double)> callback;

  2) callback = NewCallback(this, &Foo::func)  ==>  

      callback = base::Bind(&Foo::func, base::Unretained(this));

Be sure to add base::Unretained() around the object that give to Bind() when replacing a NewCallback call.  Otherwise, base::Bind() will take a reference on the object which will change the memory handling behavior.

So just to be clear, obviously this will only take a reference if the object is ref-counted. What if the object isn't, what happens in that case? i.e. what if someone tries to call the callback after the object is gone, will it crash or will it do nothing?

 

== NewRunnableMethod() ==

  1) Task* task  ==> base::Closure task

  2) task = NewRunnableMethod(this, &Foo::func, a, b)  ==> 

      task = base::Bind(&Foo::func, this, a, b)

  3) ml->PostTask(FROM_HERE, NewRunnableMethod(this, &Foo::func, a, b)  ==> 

      ml->PostTask(FROM_HERE, base::Bind(&Foo::DoSomething, this, a, b)

IMPORTANT: When using base::Bind() with PostTask, it is vital that all bound arguments are safe to delete from the originalting thread, and the thread that it is being posted onto.  Unlike NewRunnableMethod(), the result of base::Bind() is refcounted, and may be deleted on either thread.  This should not be an issue if the parameters (eg., a and b above) are primitive types, or things like scoped_refptrs.  However, if they are custom, copyable objects, then be careful.

== ScopedRunnableMethodFactory and ScopedCallbackFactory ==

   1) ScopedRunnableMethodFactory<Foo> callback_factory_  ==> WeakPtrFactory<Foo> weak_factory_

   2) callback_factory_.NewRunnableMethod(this, &Foo::func, a, b)  ==>

       base::Bind(&Foo::func, weak_factory_.GetWeakPtr(), a, b)

   3) callback_factory.RevokeAll()  ==>  weak_factory_->InvalidateWeakPtrs()

The ScopedCallbackFactory conversion is almost exactly the same.

Note, you cannot cancel individual callbacks created from the factory yet. If you really need that feature, please let me know.

== Custom Implementations of Task ==

Most of the time people inherit from Task(), they do so in order to carry some context around.  Consider trying to remove the derived class and instead use currying with base::Bind() on a normal function to get the desired behavior.

This is a _very_ cool feature that I've seen in some reviews (rsesek). It would be great to give some examples in the wiki as well.

Feel free to e-mail back to this thread with questions if you can't figure out how to replicate a custom Task class using base::Bind.

== More Info ==

For detail documentation on base::Bind and base::Callback, please see file comments here:

  http://src.chromium.org/viewvc/chrome/trunk/src/base/callback.h?view=markup

  http://src.chromium.org/viewvc/chrome/trunk/src/base/bind_helpers.h?view=markup

The bind_unittest also has lots of examples for how to use this API.

  http://src.chromium.org/viewvc/chrome/trunk/src/base/bind_unittest.cc?view=markup

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[William Chan (陈智昌)]

I will let Albert update the wiki as he sees fit :)

On Tue, Sep 27, 2011 at 11:22 AM, John Abd-El-Malek <j...@chromium.org> wrote:

Can you update the threading wiki on http://www.chromium.org/developers/design-documents/threading with the new recommended way?

I have been mostly ignoring this work, so I've been using the old way because that's what I'm used for (and I have some stuff I wasn't sure about). A few questions below. If you can also put that in the wiki, that'd be great :)

On Tue, Sep 27, 2011 at 9:37 AM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

If you use NewRunnableMethod or NewCallback, this mail concerns you!

We're looking for people to help convert all the NewRunnableMethod() and NewCallback() calls over to base::Bind().  Because these constructs are so widely used. we need all the help we can get!

Luckily, the syntax change should be fairly easy. Recipes for common conversions and a few caveats are listed at the end of this mail.  The general procedure should be:

  1) Pick a file.

  2) Apply recipes below.

  3) Fix compile errors and memory suppressions as necessary.

  4) Rinse. Repeat with next file.

I've created a spreadsheet listing all the files that have constructs needing changes.  If you decide to help out, please put your name next to the file you are working on here (make sure to be signed in with your @chromium.org account):

    http://goo.gl/89Chr

When you are finish with a file, add an "x" in the done column.  I'll periodically go through and shift the finished files down to the bottom of the list.

Note that I am also working on Clang plugin (http://codereview.chromium.org/7886056/) to aide in this, but haven't gotten the kinks worked out yet.  When/if I get that working, I'll send an update.

Let me know if there are any questions or concerns!

-Albert

== NewCallback ==

  1) Callback2<int, double>::Type* callback  ==> base::Callback<void(int, double)> callback;

  2) callback = NewCallback(this, &Foo::func)  ==>  

      callback = base::Bind(&Foo::func, base::Unretained(this));

Be sure to add base::Unretained() around the object that give to Bind() when replacing a NewCallback call.  Otherwise, base::Bind() will take a reference on the object which will change the memory handling behavior.

So just to be clear, obviously this will only take a reference if the object is ref-counted. What if the object isn't, what happens in that case? i.e. what if someone tries to call the callback after the object is gone, will it crash or will it do nothing?

See MaybeRefCount in base/bind_helpers.h. If the function pointer is to a member function, then we call AddRef() or Release() in the constructor/destructor of the internal closure object (InvokerStorageFoo), EXCEPT if the pointer has been wrapped by base::Unretained(). So you have to explicitly disable refcounting. And if someone tries to call the callback after the object is gone, it's a bug. Don't do that. If you want to auto-cancel the running of the function, you can use base::WeakPtr. base::Bind() knows about base::WeakPtr, and will check to make sure it's not NULL prior to invoking the member function.

 

[Chris Bentzel]

On Tue, Sep 27, 2011 at 12:37 PM, Albert J. Wong (王重傑)
<ajw...@chromium.org> wrote:

> Note that I am also working on Clang
> plugin (http://codereview.chromium.org/7886056/) to aide in this, but
> haven't gotten the kinks worked out yet.  When/if I get that working, I'll
> send an update.

Should folks delay work on manually converting over to the new
callback system while you work on the automated converter?

I'm guessing no given this email, but it's hard to get motivated to do
work that robots might be able to do for us.

[William Chan (陈智昌)]

Albert had worked on it for some time previously but did not successfully get it working. He sounds less confident now about it, which is I believe why he's announced the transition.

[Peter Kasting]

On Tue, Sep 27, 2011 at 9:37 AM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

== NewCallback ==

  1) Callback2<int, double>::Type* callback  ==> base::Callback<void(int, double)> callback;

Variation on this:

typedef Callback2<int, double>::Type MyCallback;

void Foo(MyCallback* callback);

-->

typedef base::Callback<void(int, double)> MyCallback;

void Foo(const MyCallback& callback);

On Tue, Sep 27, 2011 at 9:37 AM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

Let me know if there are any questions or concerns!

It looks like content/browser/cancelable_request.h is a large chunk of functionality built atop the old callback system.  It's not easy for me as a third party to figure out how this should be changed, or if it can be eliminated entirely in favor of new capabilities of your system.  Because this system currently forces users to use and typedef the old callback types, fixing this will involve changing both this file and all its users.

Unfortunately, this is used by (among other things) the history system, so there are large chunks of Chrome code that can't be converted without this.

PK

[Albert J. Wong (王重傑)]

Answering multiple mails.

John: I'll update the threading sites page in a bit.  Will's answers to the other questions are correct.

Chris: I wouldn't wait on the clang plugin.  After having sunk a week into it, I've hit a brick wall so we're back to manual migration. Also, some usages, like what Peter mentioned in his mail, would yield cleaner solutions if a human look at the overall design again.

That's a pretty complex piece of code.  Breaking down the API, essentially you want to be able to run a piece of work on a background thread (aka, the Provider thread), and return the result to the originating thread (aka. the consumer thread).

I believe a lot of this functionality might be achievable via the MessageLoopProxy::PostTaskAndReply() and BrowserThread::PostTaskAndReply() functions.  See comment for PostTaskAndReply() in here http://src.chromium.org/viewvc/chrome/trunk/src/base/message_loop_proxy.h?view=markup.

Let me try to migrate a small example (going to try IconManager::LoadIcon and DownloadItemGtk()) and see what I can come up with.  My gut is we want to completely remove the CancelableRequest concept and replace it with PostTaskAndReply().

-Albert

[Darin Fisher]

Yup.  In a separate (likely private) thread, I had explained a solution for this to Brett.

Copy/pasting from that thread:

I had been meaning to reply for a while now.  I think you could leverage PostTaskAndReply

for the history system by either sending a ref-counted bool with each task that the task

knows to check before doing its work, or you could send an integer, and each task check

that integer against a global counter that is incremented each time we wish to dispose of

old work.  It depends on what kinds of cancellation semantics you need.

The point is that the proxying of a request onto a background thread and then back again

to the originating thread is non-trivial, and we should not need to repeat that code.

-Darin 

-Albert

[Michael Nordman]

I've got some reading up todo on all this new infrastructure. PostTaskAndReply as a base function might be nice.

2011/9/27 Darin Fisher <da...@chromium.org>

[William Chan (陈智昌)]

On Tue, Sep 27, 2011 at 4:54 PM, Michael Nordman <mich...@google.com> wrote:

I've got some reading up todo on all this new infrastructure. PostTaskAndReply as a base function might be nice.

IMHO, PostTaskAndReply() + base::WeakPtr<T> is particularly sexy. We just need a PassOwnPtr<T> equivalent for parameters and we'll be set.

[Scott Violet]

On Tue, Sep 27, 2011 at 9:37 AM, Albert J. Wong (王重傑)
<ajw...@chromium.org> wrote:

IMHO the old names were clearer from the perspective of a developer
using them. It's pretty clear ScopedRunnableMethodFactory is used to
generate something that is going to invoke a method. Similarly empty()
is consistent with stl (for better or for worse). RevokeAll is also
pretty clear.

Compare that with base::WeakPtrFactory, HasWeakPtrs and
InvalidateWeakPtrs. Without being in the know WeakPtrFactory is an
extremely generic name and not particularly obvious what it is used
for.

-Scott

[Scott Hess]

On Wed, Sep 28, 2011 at 9:19 AM, Scott Violet <s...@chromium.org> wrote:
> IMHO the old names were clearer from the perspective of a developer
> using them. It's pretty clear ScopedRunnableMethodFactory is used to
> generate something that is going to invoke a method. Similarly empty()
> is consistent with stl (for better or for worse). RevokeAll is also
> pretty clear.
>
> Compare that with base::WeakPtrFactory, HasWeakPtrs and
> InvalidateWeakPtrs. Without being in the know WeakPtrFactory is an
> extremely generic name and not particularly obvious what it is used
> for.

I did find it confusing that the ability to have a cancelable callback
is buried in the sense of whether you passed in a weak ptr at P1. It
does map directly from the old code (there is no
ScopedRunnableFunctionFactory<>, after all), but it feels like a wart
in the new code. It seems like "canceled" should be an entirely
separate concept from "bound parameters".

-scott

[Robert Sesek]

Agreed; I filed this a few weeks ago around this idea: http://code.google.com/p/chromium/issues/detail?id=96749 

rsesek / @chromium.org

[Albert J. Wong (王重傑)]

Part of me just hoping it'd become idiomatic enough to not matter.  Conceptually, the idea is that you're dispatching a message to a receiver object, and if the receiver object is no longer around, do nothing with it.  This is kinda-sorta in line with how things like "nil" work in objective-C.  But yes, this does feel less clear than ScopedRunnableMethodFactory.  Also, I think there's a class of cancellation problems that we might be able to write cleaner constructs for.

Feel free to add suggestions for syntactic sugar, or examples of difficult to do cancellations, on to the bug rsesek cited.

-Albert

[Albert J. Wong (王重傑)]

Monday update!

Last week we migrated just over 110 files, and I see CLs in progress for another 50!   Thanks everyone who's helped out so far.

We still have a long way to go though. If you get a few minutes, please consider migrating one or two files in your area.  

Files with only NewRunnableMethod (NRM) or NewRunnableFunction (NRF) are easy pickings, usually only taking 1-2 minutes per file.  The tracking spreadsheet shows which files only NRM and NRF calls:

   http://goo.gl/89Chr

Also, as of http://src.chromium.org/viewvc/chrome?view=rev&revision=103691, CancelableRequest now supports base::Callback<>.  Please see the icon_manager.h diffs for an example of how to migrate.

Lastly, if you do migrate files, please remember to update the spreadsheet with your name and mark the "Done" column when you're finished.  After this is all done, I'll try to think of something nice to do for everyone that's helped out. :)

-Albert

[Albert J. Wong (王重傑)]

== 2.5 week stats, Callback usage tips, and upcoming features ==

We're now down to  *491 files*  (from 1093) without owners meaning we're over half way done!!! Yay!  (•‿•)

Special props go out to csilv, fischman, jhawkins, sergeyu, tfarina, and tzik.  Each of them have touched more than 50 files!  jhawkins and in csilv in particular have been tearing through APIs, defeating the monstrous history.h amongst others.  They've crushed 130 files and 66 files respectively (stats are from spreadsheet).

If you have sometime now, or post M16, please consider adopting a file.  Spreadsheet is here:

    http://goo.gl/89Chr

In particular, we could use help with chrome/browser/chromeos, chrome/browser/extensions, and everything under webkit.

If you need help feel free to contact willchan@, akalin@, or me.  I will have spotty e-mail over the next 2 weeks, so until Halloween, prefer Will and Fred.

Usage tips + upcoming features are below.

-Albert

Here's a few less-trivial CLs that how Bind() can be used to greatly simplify async and cross-thread structures.

* [tzik] Removing custom callback queueing in the Quota API.

   http://codereview.chromium.org/8230012/

* [jhawkins] Chaining multiple async calls.

   http://codereview.chromium.org/8212006/diff/23001/chrome/browser/automation/automation_provider_gtk.cc

* [jhawkins] Use PostTaskAndReply() instead of helper class to to safely execute task on background thread

   when posting object may be deleted.

   http://codereview.chromium.org/8113025/diff/3004/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc

New features that being considered:

 * Owned(). Specifies that a non-refcounted object is Owned() entirely by the Callback.  Think scoped_ptr<> for callback.

   http://codereview.chromium.org/8209001/

 * IgnoreResult(). Removes the return type of a bound function. Most useful when trying to use PostTask.  There is

   already partial support, but it doesn't work right with WeakPtrs<> and the syntax will be modified slightly.

   http://crbug.com/98919