Proof of concept for tr1-based Task replacement.

Proof of concept for tr1-based Task replacement.

BUG=
TEST=

[awong, 2010-12-24 03:39:16]

Surprise Holiday Present: proof of concept implementation of redoing Task using
tr1.

There's some ugly hacks to keep me from having to mess with MessageLoop
implementation much.  Also, the templated constructors could likely be
simplified a little.

However, this shows it working for a simple use of NewRunnableMethod.

Let me know if this seems worth my looking into it further, and what kinda of
usage people would want me to try porting over next to see how this API works
out.

[awong, 2010-12-28 23:24:02]

Added some examples of replacing CreateFunctor, CompletionCallback, and
ScopedRunnableMethodFactory.

This should be pretty complete coverage of the use cases I think.

[brettw, 2011-01-04 19:12:17]

http://codereview.chromium.org/6052006/diff/7001/base/closure.h
File base/closure.h (right):

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode1
base/closure.h:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
Copyright is 2 years out of date.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode30
base/closure.h:30: explicit ClosureState(tracked_objects::Tracked* tracked)
I think we'd want the implementation of all of the non-template functions to be
in a .cc file to avoid bloat.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode44
base/closure.h:44: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode68
base/closure.h:68: // object.  That is, this will not compile:
I find the existence of this "wrong" code confusing since I often just scan for
code samples and copy them. I'd just delete the "wrong" example and say
  // object. You have to pass in a pointer to it:
  //
     <right code>

It would be nice if there was also an example of using a Closure with
parameters.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode84
base/closure.h:84: // --- Traits object defintions --
I'd make sure everything inside the class is indented 2 spaces. I've not seen us
do this non-indented comment style anywhere else.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode98
base/closure.h:98: static ::std::tr1::function<void()> RefThunk(O* o) {
I've never seen us write "::std" (everywhere we just do "std::"). It seems just
as clear so I'd use that instead.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode143
base/closure.h:143: : state_(new ClosureState(new tracked_objects::Tracked())) {
All these wrapped initializers should be indented 2 more spaces (: indented 4
spaces).

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode199
base/closure.h:199: : state_(new ClosureState(new tracked_objects::Tracked())) {
bind makes an object on the heap underneath, right? And here you're making two
more heap objects. For something as simple as Closure(&Foo::func, &f) I wouldn't
necessarily expect 3 heap allocations, especially since I'm allocating the
Closure itself on the stack.

I don't think NewRunnableMethod allocates *any* objects on the heap (am I
missing something?), so making it now have 3 seems like a significant
regression.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode217
base/closure.h:217: Closure(Sig f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5)
In NewRunnableMethod we pass the arguments as const refs. It seems like that
will prevent a copy of objects passed around (say like a GURL).

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode288
base/closure.h:288: ClosureCanceller() : cancel_state_(new CancelState()) {}
Ditto for non-inlining these.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode300
base/closure.h:300: bool empty() const {
Since this calls an uppercase (non-inlinable-for-free) function, I'd expect it
to be called IsEmpty() and non-inline.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode319
base/closure.h:319: class CancelState : public RefCounted<CancelState> {
The definition of this can probably be moved to the .cc file and just forward
declare this inner class.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode334
base/closure.h:334: };
DISALLOW_COPY_AND_ASSIGN?

[brettw, 2011-01-04 19:14:57]

http://codereview.chromium.org/6052006/diff/7001/base/closure.h
File base/closure.h (right):

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode199
base/closure.h:199: : state_(new ClosureState(new tracked_objects::Tracked())) {
Actually NewRunnableMethod of course make a "new" runnable method, which I think
is equivalent to the new inside of bind? So it's 3 versus 1?

[awong, 2011-01-04 20:57:12]

Thanks for the review! Especially thanks for looking over the memory
allocation/copy semantics.

Address most comments, and replied to others.

I'm still being a bit lax on style & implementation because I don't even know if
this CL has a shot of going in yet. I'd like to nail the API first before
tweaking the implementation too much.

FYI, I scheduled a meeting for monday to talk over this.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h
File base/closure.h (right):

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode1
base/closure.h:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
On 2011/01/04 19:12:17, brettw wrote:
> Copyright is 2 years out of date.

Done.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode30
base/closure.h:30: explicit ClosureState(tracked_objects::Tracked* tracked)
On 2011/01/04 19:12:17, brettw wrote:
> I think we'd want the implementation of all of the non-template functions to
be
> in a .cc file to avoid bloat.

Right...I'd like to avoid breaking out a cc file for right now since this is
mainly a proof-of-concept CL for testing out the API and showing how the basic
components might look.

Added a TODO at the top of the file.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode44
base/closure.h:44: };
On 2011/01/04 19:12:17, brettw wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode68
base/closure.h:68: // object.  That is, this will not compile:
On 2011/01/04 19:12:17, brettw wrote:
> I find the existence of this "wrong" code confusing since I often just scan
for
> code samples and copy them. I'd just delete the "wrong" example and say
>   // object. You have to pass in a pointer to it:
>   //
>      <right code>
> 
> It would be nice if there was also an example of using a Closure with
> parameters.

Removed the incorrect code, and added a comment.

Once an API is settled on, I'll beef up the documentation a lot.  There's a
moderate amount of template magic going on as well that should be commented.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode84
base/closure.h:84: // --- Traits object defintions --
On 2011/01/04 19:12:17, brettw wrote:
> I'd make sure everything inside the class is indented 2 spaces. I've not seen
us
> do this non-indented comment style anywhere else.

fixed.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode98
base/closure.h:98: static ::std::tr1::function<void()> RefThunk(O* o) {
On 2011/01/04 19:12:17, brettw wrote:
> I've never seen us write "::std" (everywhere we just do "std::"). It seems
just
> as clear so I'd use that instead.

Done.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode143
base/closure.h:143: : state_(new ClosureState(new tracked_objects::Tracked())) {
On 2011/01/04 19:12:17, brettw wrote:
> All these wrapped initializers should be indented 2 more spaces (: indented 4
> spaces).

Done.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode199
base/closure.h:199: : state_(new ClosureState(new tracked_objects::Tracked())) {
On 2011/01/04 19:14:58, brettw wrote:
> Actually NewRunnableMethod of course make a "new" runnable method, which I
think
> is equivalent to the new inside of bind? So it's 3 versus 1?

Right...really, the tr1::function<> reference should be moved into the closure
state, and ClosureState should just contain a Tracked object.  That should move
this from 3 allocations down to 2.  Also, passing/copying a closure object would
become just one scoped_refptr copy.

I think 2 allocations is as low as can be gotten with any callback system that
wraps another unless you start playing with placement-new.  To get back to 1 one
allocation, we'd need to drop the tr1::bind dependency.  This might be a good
enough motivation to avoid tr1::bind.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode217
base/closure.h:217: Closure(Sig f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5)
On 2011/01/04 19:12:17, brettw wrote:
> In NewRunnableMethod we pass the arguments as const refs. It seems like that
> will prevent a copy of objects passed around (say like a GURL).

Eek...good point.  I was copying the tr1::bind API.  We can change this to take
const AN& as the argument type.

However, this points to another mark against wrapping tr1::bind's
implementation.

I just did a test run with gcc's tr1::bind implementation and found it copies
the argument 4 times on a bind call.  For NRM, it copies the argument 2 times.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode288
base/closure.h:288: ClosureCanceller() : cancel_state_(new CancelState()) {}
On 2011/01/04 19:12:17, brettw wrote:
> Ditto for non-inlining these.

deferring addressing this for now.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode300
base/closure.h:300: bool empty() const {
On 2011/01/04 19:12:17, brettw wrote:
> Since this calls an uppercase (non-inlinable-for-free) function, I'd expect it
> to be called IsEmpty() and non-inline.

Was replicating the ScopedRunnableMethodFactory API and implementation.  FYI,
SRMF violates this itself...

I added a TODO.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode319
base/closure.h:319: class CancelState : public RefCounted<CancelState> {
On 2011/01/04 19:12:17, brettw wrote:
> The definition of this can probably be moved to the .cc file and just forward
> declare this inner class.

deferred via TODO at the top of the file.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode334
base/closure.h:334: };
On 2011/01/04 19:12:17, brettw wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

[willchan no longer on Chromium, 2011-01-04 21:09:22]

http://codereview.chromium.org/6052006/diff/7001/base/closure.h
File base/closure.h (right):

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode217
base/closure.h:217: Closure(Sig f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5)
On 2011/01/04 20:57:13, awong wrote:
> On 2011/01/04 19:12:17, brettw wrote:
> > In NewRunnableMethod we pass the arguments as const refs. It seems like that
> > will prevent a copy of objects passed around (say like a GURL).
> 
> Eek...good point.  I was copying the tr1::bind API.  We can change this to
take
> const AN& as the argument type.
> 
> However, this points to another mark against wrapping tr1::bind's
> implementation.
> 
> I just did a test run with gcc's tr1::bind implementation and found it copies
> the argument 4 times on a bind call.  For NRM, it copies the argument 2 times.

Is this correct?  NewRunnableMethod() will create a Params, which is a struct
Tuple type, which has member variables which make a copy of the parameter.  We
definitely make a copy.  Otherwise, const std::string& as a parameter to
NewRunnableMethod() would cause a bunch of bugs.

[awong, 2011-01-04 21:44:45]

On 2011/01/04 21:09:22, willchan wrote:
> http://codereview.chromium.org/6052006/diff/7001/base/closure.h
> File base/closure.h (right):
> 
> http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode217
> base/closure.h:217: Closure(Sig f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5)
> On 2011/01/04 20:57:13, awong wrote:
> > On 2011/01/04 19:12:17, brettw wrote:
> > > In NewRunnableMethod we pass the arguments as const refs. It seems like
that
> > > will prevent a copy of objects passed around (say like a GURL).
> > 
> > Eek...good point.  I was copying the tr1::bind API.  We can change this to
> take
> > const AN& as the argument type.
> > 
> > However, this points to another mark against wrapping tr1::bind's
> > implementation.
> > 
> > I just did a test run with gcc's tr1::bind implementation and found it
copies
> > the argument 4 times on a bind call.  For NRM, it copies the argument 2
times.
> 
> Is this correct?  NewRunnableMethod() will create a Params, which is a struct
> Tuple type, which has member variables which make a copy of the parameter.  We
> definitely make a copy.  Otherwise, const std::string& as a parameter to
> NewRunnableMethod() would cause a bunch of bugs.

Right.  NRM does indeed store a copy of the bound arguments inside the Params
tuple, as does tr1::bind.

The difference is that tr1::bind introduced 2 more copies during the function
call chain used to create its params tuple.

I don't know why it adds 2 copies rather than 1...but it does.

Looking at this more, I'm also not certain why our current implementation of
base::Tuple causes an extra copy in its constructor.

[willchan no longer on Chromium, 2011-01-04 21:56:22]

On 2011/01/04 21:44:45, awong wrote:
> On 2011/01/04 21:09:22, willchan wrote:
> > http://codereview.chromium.org/6052006/diff/7001/base/closure.h
> > File base/closure.h (right):
> > 
> > http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode217
> > base/closure.h:217: Closure(Sig f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5)
> > On 2011/01/04 20:57:13, awong wrote:
> > > On 2011/01/04 19:12:17, brettw wrote:
> > > > In NewRunnableMethod we pass the arguments as const refs. It seems like
> that
> > > > will prevent a copy of objects passed around (say like a GURL).
> > > 
> > > Eek...good point.  I was copying the tr1::bind API.  We can change this to
> > take
> > > const AN& as the argument type.
> > > 
> > > However, this points to another mark against wrapping tr1::bind's
> > > implementation.
> > > 
> > > I just did a test run with gcc's tr1::bind implementation and found it
> copies
> > > the argument 4 times on a bind call.  For NRM, it copies the argument 2
> times.
> > 
> > Is this correct?  NewRunnableMethod() will create a Params, which is a
struct
> > Tuple type, which has member variables which make a copy of the parameter. 
We
> > definitely make a copy.  Otherwise, const std::string& as a parameter to
> > NewRunnableMethod() would cause a bunch of bugs.
> 
> Right.  NRM does indeed store a copy of the bound arguments inside the Params
> tuple, as does tr1::bind.
> 
> The difference is that tr1::bind introduced 2 more copies during the function
> call chain used to create its params tuple.
> 
> I don't know why it adds 2 copies rather than 1...but it does.

Well, there's a copy from the invocation of the Closure constructor, since the
parameters are copy by value.  They should use const references.  There will be
another copy by value for the bind() call, although I think I would expect an
optimizing compiler to optimize out that copy.  Are you testing this in debug
mode?

Just to be clear, are you saying that the proposed implementation has 2 *more*
copies than the existing implementation, which has 1 copy?  At a glance I'm only
seeing 1 *more* copy, which I point out can be eliminated via using const
references.


> 
> Looking at this more, I'm also not certain why our current implementation of
> base::Tuple causes an extra copy in its constructor.

It looks like it's because the Tuple's constructor does copy by value as well. 
Can't we fix that to use const references?

[awong, 2011-01-04 22:14:59]

On 2011/01/04 21:56:22, willchan wrote:
> On 2011/01/04 21:44:45, awong wrote:
> > On 2011/01/04 21:09:22, willchan wrote:
> > > http://codereview.chromium.org/6052006/diff/7001/base/closure.h
> > > File base/closure.h (right):
> > > 
> > > http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode217
> > > base/closure.h:217: Closure(Sig f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5)
> > > On 2011/01/04 20:57:13, awong wrote:
> > > > On 2011/01/04 19:12:17, brettw wrote:
> > > > > In NewRunnableMethod we pass the arguments as const refs. It seems
like
> > that
> > > > > will prevent a copy of objects passed around (say like a GURL).
> > > > 
> > > > Eek...good point.  I was copying the tr1::bind API.  We can change this
to
> > > take
> > > > const AN& as the argument type.
> > > > 
> > > > However, this points to another mark against wrapping tr1::bind's
> > > > implementation.
> > > > 
> > > > I just did a test run with gcc's tr1::bind implementation and found it
> > copies
> > > > the argument 4 times on a bind call.  For NRM, it copies the argument 2
> > times.
> > > 
> > > Is this correct?  NewRunnableMethod() will create a Params, which is a
> struct
> > > Tuple type, which has member variables which make a copy of the parameter.

> We
> > > definitely make a copy.  Otherwise, const std::string& as a parameter to
> > > NewRunnableMethod() would cause a bunch of bugs.
> > 
> > Right.  NRM does indeed store a copy of the bound arguments inside the
Params
> > tuple, as does tr1::bind.
> > 
> > The difference is that tr1::bind introduced 2 more copies during the
function
> > call chain used to create its params tuple.
> > 
> > I don't know why it adds 2 copies rather than 1...but it does.
> 
> Well, there's a copy from the invocation of the Closure constructor, since the
> parameters are copy by value.  They should use const references.  There will
be
> another copy by value for the bind() call, although I think I would expect an
> optimizing compiler to optimize out that copy.  Are you testing this in debug
> mode?

I didn't use this exact code, but wrote a reduction and testing using -O2. 
tr1::bind() yielded 4 copies when I invoked it directly.  It yielded 5 if I
wrote a wrapper (like I do in the Closure() constructor). 

It's possible that a trivial copy-constructor might have different optimization
behavior.

 
> Just to be clear, are you saying that the proposed implementation has 2 *more*
> copies than the existing implementation, which has 1 copy?  At a glance I'm
only
> seeing 1 *more* copy, which I point out can be eliminated via using const
> references.

Yes, it seems to have 2 more copies.  I don't know why 2 since I also only see
1.


> > 
> > Looking at this more, I'm also not certain why our current implementation of
> > base::Tuple causes an extra copy in its constructor.
> 
> It looks like it's because the Tuple's constructor does copy by value as well.

> Can't we fix that to use const references?

Yeah...that was my thought.  I want to ask Zhanyong briefly about the difference
before pushing that way.  I recall him making a note about using const T& for
arguments, but I can't remember the details.

[willchan no longer on Chromium, 2011-01-04 22:27:22]

http://codereview.chromium.org/6052006/diff/7001/base/closure.h
File base/closure.h (right):

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode35
base/closure.h:35: release_thunk_();
Why?  We don't do this for Tasks.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode43
base/closure.h:43: scoped_ptr<tracked_objects::Tracked> tracked_;
I'm not sure, but this might be a design flaw.  ClosureState is shared. 
tracked_objects::Tracked is designed to be used for a single task run on an
executor (MessageLoop or WorkerPool or what not).

So, if you intend a Closure to be allowed to be "permanent", then it needs to be
postable multiple times.  This means each time it's posted, it needs a new
tracked_objects::Tracked.

Really, that we make Task inherit from tracked_objects::Tracked seems like a
design flaw.  I think that tracked_objects::Tracked should be a sibling object
of the Task, contained within PendingTask.  Then this would work properly.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode199
base/closure.h:199: : state_(new ClosureState(new tracked_objects::Tracked())) {
On 2011/01/04 20:57:13, awong wrote:
> On 2011/01/04 19:14:58, brettw wrote:
> > Actually NewRunnableMethod of course make a "new" runnable method, which I
> think
> > is equivalent to the new inside of bind? So it's 3 versus 1?
> 
> Right...really, the tr1::function<> reference should be moved into the closure
> state, and ClosureState should just contain a Tracked object.  That should
move
> this from 3 allocations down to 2.  Also, passing/copying a closure object
would
> become just one scoped_refptr copy.
> 
> I think 2 allocations is as low as can be gotten with any callback system that
> wraps another unless you start playing with placement-new.  To get back to 1
one
> allocation, we'd need to drop the tr1::bind dependency.  This might be a good
> enough motivation to avoid tr1::bind.

I don't think tr1::bind exposes the allocator as a template parameter, so I
don't know of any way to use placement new to get around this limitation.

Stepping back a second, I want to revisit the whole wrapping thing.  As I
understand it, the reason to wrap it is to simplify the API to make it easier to
understand by hiding the details.  I think, that's possible without any
overhead, if eliminate ClosureState.  I've proposed removing
tracked_objects::Tracked from ClosureState, which I believe eliminates any
reason to have ClosureState.  Closure should basically be tr1::function, with a
more chromium-styled API to make it blend more easily into our codebase.

[awong, 2011-01-04 22:37:35]

On Tue, Jan 4, 2011 at 2:27 PM, <willchan@chromium.org> wrote:

>
> http://codereview.chromium.org/6052006/diff/7001/base/closure.h
> File base/closure.h (right):
>
> http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode35
> base/closure.h:35: release_thunk_();
> Why?  We don't do this for Tasks.
>
> http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode43
> base/closure.h:43: scoped_ptr<tracked_objects::Tracked> tracked_;
> I'm not sure, but this might be a design flaw.  ClosureState is shared.
> tracked_objects::Tracked is designed to be used for a single task run on
> an executor (MessageLoop or WorkerPool or what not).
>
> So, if you intend a Closure to be allowed to be "permanent", then it
> needs to be postable multiple times.  This means each time it's posted,
> it needs a new tracked_objects::Tracked.
>
> Really, that we make Task inherit from tracked_objects::Tracked seems
> like a design flaw.  I think that tracked_objects::Tracked should be a
> sibling object of the Task, contained within PendingTask.  Then this
> would work properly.


This is an interesting point...Tracked does 2 things currently.
  (1) Track the "post" location (though it calls it the BirthPlace).
  (2) Track the creation time.

If we disassociate these two parts, we can avoid this shared state.

I agree that Task inheriting from Tracked feels odd.  It does feel more of
something that belongs to the MesasgeLoop rather than the Task.

http://codereview.chromium.org/6052006/diff/7001/base/closure.h#newcode199
> base/closure.h:199: : state_(new ClosureState(new
> tracked_objects::Tracked())) {
> On 2011/01/04 20:57:13, awong wrote:
>
>> On 2011/01/04 19:14:58, brettw wrote:
>> > Actually NewRunnableMethod of course make a "new" runnable method,
>>
> which I
>
>> think
>> > is equivalent to the new inside of bind? So it's 3 versus 1?
>>
>
>  Right...really, the tr1::function<> reference should be moved into the
>>
> closure
>
>> state, and ClosureState should just contain a Tracked object.  That
>>
> should move
>
>> this from 3 allocations down to 2.  Also, passing/copying a closure
>>
> object would
>
>> become just one scoped_refptr copy.
>>
>
>  I think 2 allocations is as low as can be gotten with any callback
>>
> system that
>
>> wraps another unless you start playing with placement-new.  To get
>>
> back to 1 one
>
>> allocation, we'd need to drop the tr1::bind dependency.  This might be
>>
> a good
>
>> enough motivation to avoid tr1::bind.
>>
>
> I don't think tr1::bind exposes the allocator as a template parameter,
> so I don't know of any way to use placement new to get around this
> limitation.
>
> Stepping back a second, I want to revisit the whole wrapping thing.  As
> I understand it, the reason to wrap it is to simplify the API to make it
> easier to understand by hiding the details.  I think, that's possible
> without any overhead, if eliminate ClosureState.  I've proposed removing
> tracked_objects::Tracked from ClosureState, which I believe eliminates
> any reason to have ClosureState.  Closure should basically be
> tr1::function, with a more chromium-styled API to make it blend more
> easily into our codebase.


Take a look at this CL too then:

  http://codereview.chromium.org/6094005/diff/3001/base/prebind.h

Specifically, at prebind.h.  The "Thunk" class is closer to what Closure
probably should be I think...

-Albert