Create "Prebind" a wrapper to tr1::bind.

Create "Prebind" a wrapper to tr1::bind.

BUG=
TEST=

[awong, 2010-12-30 03:07:12]

Start of an attempt to wrap bind.

[awong, 2010-12-30 09:46:07]

I finished out wrapping tr1::function/bind with base::Thunk/Prebind.  Then I
threw it on top of the Closure example from this CL:

  http://codereview.chromium.org/6052006/

The interesting files are prebind.h, download_manager_unittest.cc,
host_resolver_impl.cc, and client_socket_pool_base.{h,cc}.

The other files are just holdovers from the Closure CL which I'm reusing so I
can run the unittests.  Since Thunk is a proxy for tr1::function, it can be used
by Closure just fine.  yay :D

[willchan no longer on Chromium, 2011-01-05 00:30:16]

I haven't reviewed things in detail, but I think we can be considerably more
efficient by giving up some flexibility.

http://codereview.chromium.org/6094005/diff/3001/base/closure.h
File base/closure.h (right):

http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode99
base/closure.h:99: static ::std::tr1::function<void()> RefThunk(O* o) {
Do we really need to use Thunks here?  Is the flexibility worth the extra heap
allocation here?  Can't we have a bool parameter in the Closure object that
indicates whether or not to AddRef()/Release()?  Note that to do this, we'd have
to define a helper function with a boolean template parameter.  Specialize one
boolean value to call AddRef()/Release() and the other to be a no-op.  This is
necessary since otherwise the code may not compile since O may not define
AddRef()/Release().

http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode268
base/closure.h:268: func_ = ::std::tr1::bind(&DoNothing);
can't this just be std::tr1::function()?

[willchan no longer on Chromium, 2011-01-05 00:59:01]

I need to review this some more, but I have to head out.  Here are some
preliminary comments.

http://codereview.chromium.org/6094005/diff/3001/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6094005/diff/3001/base/prebind.h#newcode18
base/prebind.h:18: tracked_objects::Tracked tracked_;
Similar comment as I made on the ClosureState thing.  I think this is not
needed.  It should moved to Thunk and ThunkState should be removed.

http://codereview.chromium.org/6094005/diff/3001/base/prebind.h#newcode50
base/prebind.h:50: class Thunk;
I'm not sure if thunk is as commonly used a term as callback.  If possible, I'd
like to see us use callback instead of thunk.

[awong, 2011-01-05 03:17:41]

I've removed ThunkState in favor of using a higher-order function + a
scoped_refptr to handle the refcounting.

http://codereview.chromium.org/6094005/diff/3001/base/closure.h
File base/closure.h (right):

http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode99
base/closure.h:99: static ::std::tr1::function<void()> RefThunk(O* o) {
On 2011/01/05 00:30:17, willchan wrote:
> Do we really need to use Thunks here?  Is the flexibility worth the extra heap
> allocation here?  Can't we have a bool parameter in the Closure object that
> indicates whether or not to AddRef()/Release()?  Note that to do this, we'd
have
> to define a helper function with a boolean template parameter.  Specialize one
> boolean value to call AddRef()/Release() and the other to be a no-op.  This is
> necessary since otherwise the code may not compile since O may not define
> AddRef()/Release().

Looking at this more, I don't think it's quite that simple.  We probably don't
need a fully generalized thunk.  However, if we specialize the Thunk object
based on a boolean refcount, then the caller will have to always declare that in
the type.  So, the API would look like:

  Thunk<void(void), true> t = base::Prebind(&O::blah, &o);

Also, if t held the reference, to o, then all copies of t would ref/deref o. 
We'd be trading a heap allocation at creation for an extra ref/deref per copy.

I think a nicer solution would be to bind a scoped_refptr into the argument list
when we're refcounting.  See the "Invoker" think I just added.

http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode268
base/closure.h:268: func_ = ::std::tr1::bind(&DoNothing);
On 2011/01/05 00:30:17, willchan wrote:
> can't this just be std::tr1::function()?

That actually asserted.

However, I can remove the bind call and just do func_ = &DoNothing.

http://codereview.chromium.org/6094005/diff/3001/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6094005/diff/3001/base/prebind.h#newcode18
base/prebind.h:18: tracked_objects::Tracked tracked_;
On 2011/01/05 00:59:01, willchan wrote:
> Similar comment as I made on the ClosureState thing.  I think this is not
> needed.  It should moved to Thunk and ThunkState should be removed.

Okay, I whacked it for now.

http://codereview.chromium.org/6094005/diff/3001/base/prebind.h#newcode50
base/prebind.h:50: class Thunk;
On 2011/01/05 00:59:01, willchan wrote:
> I'm not sure if thunk is as commonly used a term as callback.  If possible,
I'd
> like to see us use callback instead of thunk.

I don't like Thunk either.  After we settle on a particular API type, we can fix
the names to something sane.

I just started using different names so people had something to refer to.

[willchan no longer on Chromium, 2011-01-05 19:44:09]

I'm primarily concerned with the extra AddRef()/Release() pairs.

http://codereview.chromium.org/6094005/diff/3001/base/closure.h
File base/closure.h (right):

http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode99
base/closure.h:99: static ::std::tr1::function<void()> RefThunk(O* o) {
On 2011/01/05 03:17:42, awong wrote:
> On 2011/01/05 00:30:17, willchan wrote:
> > Do we really need to use Thunks here?  Is the flexibility worth the extra
heap
> > allocation here?  Can't we have a bool parameter in the Closure object that
> > indicates whether or not to AddRef()/Release()?  Note that to do this, we'd
> have
> > to define a helper function with a boolean template parameter.  Specialize
one
> > boolean value to call AddRef()/Release() and the other to be a no-op.  This
is
> > necessary since otherwise the code may not compile since O may not define
> > AddRef()/Release().
> 
> Looking at this more, I don't think it's quite that simple.  We probably don't
> need a fully generalized thunk.  However, if we specialize the Thunk object
> based on a boolean refcount, then the caller will have to always declare that
in
> the type.  So, the API would look like:

I never said template parameter.  I said parameter.  A template parameter would
cause the bad API as you pointed out.

> 
>   Thunk<void(void), true> t = base::Prebind(&O::blah, &o);
> 
> Also, if t held the reference, to o, then all copies of t would ref/deref o. 
> We'd be trading a heap allocation at creation for an extra ref/deref per copy.

Yes, that'd be bad, and that's not what I'm suggesting :)

> 
> I think a nicer solution would be to bind a scoped_refptr into the argument
list
> when we're refcounting.  See the "Invoker" think I just added.

This approach adds unnecessary AddRef()/Release() pairs.  I'll point them out in
the new code.

http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode268
base/closure.h:268: func_ = ::std::tr1::bind(&DoNothing);
On 2011/01/05 03:17:42, awong wrote:
> On 2011/01/05 00:30:17, willchan wrote:
> > can't this just be std::tr1::function()?
> 
> That actually asserted.

Oh really?  Ok, whatever.

> 
> However, I can remove the bind call and just do func_ = &DoNothing. 

Sounds good.

http://codereview.chromium.org/6094005/diff/11001/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6094005/diff/11001/base/prebind.h#newcode233
base/prebind.h:233: typename RetainTraits<T>::type*
object_(RetainTraits<T>::unwrap(t));
Here you take T, which may be scoped_refptr<O>, and withdraw the raw pointer. 
In the next line, you use MaybeScopedRefptr to wrap it within a scoped_refptr<O>
again.  This then gets copied by value as a parameter to bind().  bind() then
will copy construct it into its member variable to store the parameter.  Then
when the Thunk is returned, it will be copy constructed to the local variable. 
Every time the Thunk is copied, the scoped_refptr will be copied.  So each time
we pass around the Thunk, we get extra AddRef()/Release() pairs.  This leads to
a lot of unnecessary refcounting.

Note that this is not a problem for C++0x, because of move semantics with rvalue
references.  But this implementation as is will multiply the number of
AddRef()/Release() pairs in Chromium for all Callbacks/Tasks (which would now be
Thunks).

Note that the current implementation avoids this problem because Task objects
are heap allocated and generally not copied, whereas Thunk is intended to be
copied on the stack.

http://codereview.chromium.org/6094005/diff/11001/base/prebind.h#newcode239
base/prebind.h:239: RetainTraits<T>::MaybeScopedRefptr(object_)));
MaybeScopedRefptr() basically always forces using a scoped_refptr for a raw
pointer, unless Unretained() is used.  This causes the problem for
DISABLE_RUNNABLE_METHOD_REFCOUNT() which basically says an object shouldn't be
refcounted.  As I proposed in my fix to DISABLE_RUNNABLE_METHOD_REFCOUNT, a
base::integral_type value should be added to the UnretainedWrapper class, so
that MaybeScopedRefptr adds the scoped_refptr if T::is_refcounted::value.

[awong, 2011-01-06 03:37:03]

On 2011/01/05 19:44:09, willchan wrote:
> I'm primarily concerned with the extra AddRef()/Release() pairs.
> 
> http://codereview.chromium.org/6094005/diff/3001/base/closure.h
> File base/closure.h (right):
> 
> http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode99
> base/closure.h:99: static ::std::tr1::function<void()> RefThunk(O* o) {
> On 2011/01/05 03:17:42, awong wrote:
> > On 2011/01/05 00:30:17, willchan wrote:
> > > Do we really need to use Thunks here?  Is the flexibility worth the extra
> heap
> > > allocation here?  Can't we have a bool parameter in the Closure object
that
> > > indicates whether or not to AddRef()/Release()?  Note that to do this,
we'd
> > have
> > > to define a helper function with a boolean template parameter.  Specialize
> one
> > > boolean value to call AddRef()/Release() and the other to be a no-op. 
This
> is
> > > necessary since otherwise the code may not compile since O may not define
> > > AddRef()/Release().
> > 
> > Looking at this more, I don't think it's quite that simple.  We probably
don't
> > need a fully generalized thunk.  However, if we specialize the Thunk object
> > based on a boolean refcount, then the caller will have to always declare
that
> in
> > the type.  So, the API would look like:
> 
> I never said template parameter.  I said parameter.  A template parameter
would
> cause the bad API as you pointed out.

I don't think I understand then.  I think you're suggesting that we do something
like:

template <typename Sig>
class Thunk {
  Thunk(bool should_ref, O* o) : should_ref_(should_ref) {
    RefHelper<should_ref_>::MaybeRef(o);
  }  
}

But that doesn't compile since we're trying to instantiate a template based off
a runtime value...do you think you could write up some pseudocode to describe it
better?


> >   Thunk<void(void), true> t = base::Prebind(&O::blah, &o);
> > 
> > Also, if t held the reference, to o, then all copies of t would ref/deref o.

> > We'd be trading a heap allocation at creation for an extra ref/deref per
copy.
> 
> Yes, that'd be bad, and that's not what I'm suggesting :)
> 
> > 
> > I think a nicer solution would be to bind a scoped_refptr into the argument
> list
> > when we're refcounting.  See the "Invoker" think I just added.
> 
> This approach adds unnecessary AddRef()/Release() pairs.  I'll point them out
in
> the new code.
> 
> http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode268
> base/closure.h:268: func_ = ::std::tr1::bind(&DoNothing);
> On 2011/01/05 03:17:42, awong wrote:
> > On 2011/01/05 00:30:17, willchan wrote:
> > > can't this just be std::tr1::function()?
> > 
> > That actually asserted.
> 
> Oh really?  Ok, whatever.
> 
> > 
> > However, I can remove the bind call and just do func_ = &DoNothing. 
> 
> Sounds good.
> 
> http://codereview.chromium.org/6094005/diff/11001/base/prebind.h
> File base/prebind.h (right):
> 
> http://codereview.chromium.org/6094005/diff/11001/base/prebind.h#newcode233
> base/prebind.h:233: typename RetainTraits<T>::type*
> object_(RetainTraits<T>::unwrap(t));
> Here you take T, which may be scoped_refptr<O>, and withdraw the raw pointer. 
> In the next line, you use MaybeScopedRefptr to wrap it within a
scoped_refptr<O>
> again.  This then gets copied by value as a parameter to bind().  bind() then
> will copy construct it into its member variable to store the parameter.  Then
> when the Thunk is returned, it will be copy constructed to the local variable.

> Every time the Thunk is copied, the scoped_refptr will be copied.  So each
time
> we pass around the Thunk, we get extra AddRef()/Release() pairs.  This leads
to
> a lot of unnecessary refcounting.
> 
> Note that this is not a problem for C++0x, because of move semantics with
rvalue
> references.  But this implementation as is will multiply the number of
> AddRef()/Release() pairs in Chromium for all Callbacks/Tasks (which would now
be
> Thunks).
> 
> Note that the current implementation avoids this problem because Task objects
> are heap allocated and generally not copied, whereas Thunk is intended to be
> copied on the stack.

Doh, you're right.  Somehow I had thought that tr1::function would share one
params object.  But it doesn't. Just did some profiling with valgrind to
confirm.

So, what this really comes down to is that passing the Thunk object, or the
equivalent, will require:

  1) on a copy, extra AddRef()/Release() pairs for each 
     argument bound argument for the Invoker.
  2) one extra memory allocation at creation for ThunkState,
     and one extra AddRef()/Relaese() pair.

Does that sound right?

> http://codereview.chromium.org/6094005/diff/11001/base/prebind.h#newcode239
> base/prebind.h:239: RetainTraits<T>::MaybeScopedRefptr(object_)));
> MaybeScopedRefptr() basically always forces using a scoped_refptr for a raw
> pointer, unless Unretained() is used.  This causes the problem for
> DISABLE_RUNNABLE_METHOD_REFCOUNT() which basically says an object shouldn't be
> refcounted.  As I proposed in my fix to DISABLE_RUNNABLE_METHOD_REFCOUNT, a
> base::integral_type value should be added to the UnretainedWrapper class, so
> that MaybeScopedRefptr adds the scoped_refptr if T::is_refcounted::value.

[awong, 2011-01-06 03:38:26]

On 2011/01/05 19:44:09, willchan wrote:
> I'm primarily concerned with the extra AddRef()/Release() pairs.
> 
> http://codereview.chromium.org/6094005/diff/3001/base/closure.h
> File base/closure.h (right):
> 
> http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode99
> base/closure.h:99: static ::std::tr1::function<void()> RefThunk(O* o) {
> On 2011/01/05 03:17:42, awong wrote:
> > On 2011/01/05 00:30:17, willchan wrote:
> > > Do we really need to use Thunks here?  Is the flexibility worth the extra
> heap
> > > allocation here?  Can't we have a bool parameter in the Closure object
that
> > > indicates whether or not to AddRef()/Release()?  Note that to do this,
we'd
> > have
> > > to define a helper function with a boolean template parameter.  Specialize
> one
> > > boolean value to call AddRef()/Release() and the other to be a no-op. 
This
> is
> > > necessary since otherwise the code may not compile since O may not define
> > > AddRef()/Release().
> > 
> > Looking at this more, I don't think it's quite that simple.  We probably
don't
> > need a fully generalized thunk.  However, if we specialize the Thunk object
> > based on a boolean refcount, then the caller will have to always declare
that
> in
> > the type.  So, the API would look like:
> 
> I never said template parameter.  I said parameter.  A template parameter
would
> cause the bad API as you pointed out.
> 
> > 
> >   Thunk<void(void), true> t = base::Prebind(&O::blah, &o);
> > 
> > Also, if t held the reference, to o, then all copies of t would ref/deref o.

> > We'd be trading a heap allocation at creation for an extra ref/deref per
copy.
> 
> Yes, that'd be bad, and that's not what I'm suggesting :)
> 
> > 
> > I think a nicer solution would be to bind a scoped_refptr into the argument
> list
> > when we're refcounting.  See the "Invoker" think I just added.
> 
> This approach adds unnecessary AddRef()/Release() pairs.  I'll point them out
in
> the new code.
> 
> http://codereview.chromium.org/6094005/diff/3001/base/closure.h#newcode268
> base/closure.h:268: func_ = ::std::tr1::bind(&DoNothing);
> On 2011/01/05 03:17:42, awong wrote:
> > On 2011/01/05 00:30:17, willchan wrote:
> > > can't this just be std::tr1::function()?
> > 
> > That actually asserted.
> 
> Oh really?  Ok, whatever.
> 
> > 
> > However, I can remove the bind call and just do func_ = &DoNothing. 
> 
> Sounds good.
> 
> http://codereview.chromium.org/6094005/diff/11001/base/prebind.h
> File base/prebind.h (right):
> 
> http://codereview.chromium.org/6094005/diff/11001/base/prebind.h#newcode233
> base/prebind.h:233: typename RetainTraits<T>::type*
> object_(RetainTraits<T>::unwrap(t));
> Here you take T, which may be scoped_refptr<O>, and withdraw the raw pointer. 
> In the next line, you use MaybeScopedRefptr to wrap it within a
scoped_refptr<O>
> again.  This then gets copied by value as a parameter to bind().  bind() then
> will copy construct it into its member variable to store the parameter.  Then
> when the Thunk is returned, it will be copy constructed to the local variable.

> Every time the Thunk is copied, the scoped_refptr will be copied.  So each
time
> we pass around the Thunk, we get extra AddRef()/Release() pairs.  This leads
to
> a lot of unnecessary refcounting.
> 
> Note that this is not a problem for C++0x, because of move semantics with
rvalue
> references.  But this implementation as is will multiply the number of
> AddRef()/Release() pairs in Chromium for all Callbacks/Tasks (which would now
be
> Thunks).
> 
> Note that the current implementation avoids this problem because Task objects
> are heap allocated and generally not copied, whereas Thunk is intended to be
> copied on the stack.
> 
> http://codereview.chromium.org/6094005/diff/11001/base/prebind.h#newcode239
> base/prebind.h:239: RetainTraits<T>::MaybeScopedRefptr(object_)));
> MaybeScopedRefptr() basically always forces using a scoped_refptr for a raw
> pointer, unless Unretained() is used.  This causes the problem for
> DISABLE_RUNNABLE_METHOD_REFCOUNT() which basically says an object shouldn't be
> refcounted.  As I proposed in my fix to DISABLE_RUNNABLE_METHOD_REFCOUNT, a
> base::integral_type value should be added to the UnretainedWrapper class, so
> that MaybeScopedRefptr adds the scoped_refptr if T::is_refcounted::value.

For this last point, I'm hoping we just completely kill
DISABLE_RUNNABLE_METHOD_REFCOUNT() and force everyone to explicitly wrap their
objects with Unreation() when creating a callback.  That side-steps this issue,
no?