Callback API Change: Reimplement Bind(); support IgnoreResult, full currying, and use less types

Callback API Change: Reimplement Bind(); support IgnoreResult, full currying, and use less types.

The main API change IgnoreResult() and fully currying.  See unittest for what the new API looks like.  The rest of the changes are done to support that.

Previously, IgnoreReturn could not be used with WeakPtr<> Bind()s as it was applied after the fact to the Callback object.  Now, IgnoreResult() wraps the function like Unretained().

As an incidental benefit, the new implementation gave us fully currying for free.

Also, the new implementation scales better when supporting higher arities of functions.  The new type growth is:

  (n^2 + 20n) / 2

as opposed to 

  (3n^2 + 17n) / 2

where n == arity.

For n = 6 and n=10, the new implementation has 81 and 155 templates respectively.

The old implementation had 105 and 235 templates respectively. 

BUG=35233, 98919, 98542
TEST=existing unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110975

[awong, 2011-11-08 02:58:09]

Ready for first pass review.  I still need to fix windows, and probably
proofread, but the guts are right.

Its huge. Sorry. :(

[awong, 2011-11-08 03:01:49]

FYI, I haven't run these through the no-compile tests yet.  Also,
there is one API oddity in that we can have a completely unbound
method. I'm not sure that's a good hting.

On Mon, Nov 7, 2011 at 6:58 PM,  <ajwong@chromium.org> wrote:
> Reviewers: willchan,
>
> Message:
> Ready for first pass review.  I still need to fix windows, and probably
> proofread, but the guts are right.
>
> Its huge. Sorry. :(
>
> Description:
> Callback API Change: Reimplement Bind(); support IgnoreResult, full
> currying,
> and use less types.
>
> The main API change IgnoreResult() and fully currying.  See unittest for
> what
> the new API looks like.  The rest of the changes are done to support that.
>
> Previously, IgnoreReturn could not be used with WeakPtr<> Bind()s as it was
> applied after the fact to the Callback object.  Now, IgnoreResult() wraps
> the
> function like Unretained().
>
> As an incidental benefit, the new implementation gave us fully currying for
> free.
>
> Also, the new implementation scales better when supporting higher arities of
> functions.  The new type growth is:
>
>  (n^2 + 19n) / 2
>
> as opposed to
>
>  (3n^2 + 17n) / 2
>
> where n == arity.
>
> For n = 6 and n=10, the new implementation has 75 and 145 templates
> respectively.
>
> The old implementation had 105 and 235 templates respectively.
>
> BUG=35233,98919,98542
> TEST=existing unittests
>
>
> Please review this at http://codereview.chromium.org/8483003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M base/bind.h
>  M base/bind.h.pump
>  M base/bind_helpers.h
>  M base/bind_internal.h
>  M base/bind_internal.h.pump
>  M base/callback.h
>  M base/callback.h.pump
>  M base/callback_internal.h
>
>
>

[awong, 2011-11-08 20:47:51]

Actually, if you haven't started reviewing, hold off for a bit.  Realized there
are 2 warts to work out.

-Albert


On 2011/11/08 03:01:49, awong wrote:
> FYI, I haven't run these through the no-compile tests yet.  Also,
> there is one API oddity in that we can have a completely unbound
> method. I'm not sure that's a good hting.
> 
> On Mon, Nov 7, 2011 at 6:58 PM,  <mailto:ajwong@chromium.org> wrote:
> > Reviewers: willchan,
> >
> > Message:
> > Ready for first pass review. &nbsp;I still need to fix windows, and probably
> > proofread, but the guts are right.
> >
> > Its huge. Sorry. :(
> >
> > Description:
> > Callback API Change: Reimplement Bind(); support IgnoreResult, full
> > currying,
> > and use less types.
> >
> > The main API change IgnoreResult() and fully currying. &nbsp;See unittest
for
> > what
> > the new API looks like. &nbsp;The rest of the changes are done to support
that.
> >
> > Previously, IgnoreReturn could not be used with WeakPtr<> Bind()s as it was
> > applied after the fact to the Callback object. &nbsp;Now, IgnoreResult()
wraps
> > the
> > function like Unretained().
> >
> > As an incidental benefit, the new implementation gave us fully currying for
> > free.
> >
> > Also, the new implementation scales better when supporting higher arities of
> > functions. &nbsp;The new type growth is:
> >
> > &nbsp;(n^2 + 19n) / 2
> >
> > as opposed to
> >
> > &nbsp;(3n^2 + 17n) / 2
> >
> > where n == arity.
> >
> > For n = 6 and n=10, the new implementation has 75 and 145 templates
> > respectively.
> >
> > The old implementation had 105 and 235 templates respectively.
> >
> > BUG=35233,98919,98542
> > TEST=existing unittests
> >
> >
> > Please review this at http://codereview.chromium.org/8483003/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files:
> > &nbsp;M base/bind.h
> > &nbsp;M base/bind.h.pump
> > &nbsp;M base/bind_helpers.h
> > &nbsp;M base/bind_internal.h
> > &nbsp;M base/bind_internal.h.pump
> > &nbsp;M base/callback.h
> > &nbsp;M base/callback.h.pump
> > &nbsp;M base/callback_internal.h
> >
> >
> >

[awong, 2011-11-09 21:59:27]

Okay, ready for review.

All the files that aren't directly Bind() related just have 1-line fixes.

[willchan no longer on Chromium, 2011-11-10 19:33:39]

Started to look, but Comcast just finished, so I'm headed into the office in a
bit for lunch. Will check back in the afternoon.

http://codereview.chromium.org/8483003/diff/13002/base/bind.h.pump
File base/bind.h.pump (right):

http://codereview.chromium.org/8483003/diff/13002/base/bind.h.pump#newcode41
base/bind.h.pump:41: // bit it feels a little nicer to have the asserts here so
people do not
s/bit/but/

http://codereview.chromium.org/8483003/diff/13002/base/bind.h.pump#newcode42
base/bind.h.pump:42: // need to crack open bind_internal.h.  On the otherhand,
it makes BindState
s/otherhand/other hand/

http://codereview.chromium.org/8483003/diff/13002/base/bind.h.pump#newcode64
base/bind.h.pump:64: // Use RunnableType::RunType instead of RunType above
because our
This is hard to grok. What's the difference between RunnableType, RunType, and
RunnableType::RunType?

[willchan no longer on Chromium, 2011-11-11 01:30:25]

http://codereview.chromium.org/8483003/diff/16002/base/bind.h.pump
File base/bind.h.pump (right):

http://codereview.chromium.org/8483003/diff/16002/base/bind.h.pump#newcode93
base/bind.h.pump:93: // scoped_refptr check because the binder itself takes care
of this. We also
I'm not sure what skipping the scoped_refptr check means.

http://codereview.chromium.org/8483003/diff/16002/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8483003/diff/16002/base/bind_helpers.h#newcode222
base/bind_helpers.h:222: class HasIsMethodTag {
It's not obvious to me what's better. Just using a typedef false_type IsMethod,
or having this HasIsMethodTag. Is there a reason you chose to do it this way?

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump
File base/bind_internal.h.pump (right):

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:10: // "oh well"?
Assuming you mean statically enforce, I think it's possible, you could make the
Run() method be templated :P

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:62: //                       object that adheres the
Runnable interface.
adheres *to*

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:75: //  MakeRunnable<> -- Takes a Functor an returns
an object in the Runnable
s/an/and/

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:271: // There are 3 logcaial InvokeHelper<>
specializations: normal, void-return,
logical

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:297: static ReturnType MakeItSo(Runnable runnable[[]]
MakeItSo, LOL!

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:332: #if !defined(_MSC_VER)
What's up with this? Does VS not support this somehow?

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:422: // This stores all the state passed into Bind()
and is also the where most
s/also the where/also where/

[awong, 2011-11-11 02:17:57]

PTAL

http://codereview.chromium.org/8483003/diff/13002/base/bind.h.pump
File base/bind.h.pump (right):

http://codereview.chromium.org/8483003/diff/13002/base/bind.h.pump#newcode41
base/bind.h.pump:41: // bit it feels a little nicer to have the asserts here so
people do not
On 2011/11/10 19:33:39, willchan wrote:
> s/bit/but/

Done.

http://codereview.chromium.org/8483003/diff/13002/base/bind.h.pump#newcode42
base/bind.h.pump:42: // need to crack open bind_internal.h.  On the otherhand,
it makes BindState
On 2011/11/10 19:33:39, willchan wrote:
> s/otherhand/other hand/

Done.

http://codereview.chromium.org/8483003/diff/13002/base/bind.h.pump#newcode64
base/bind.h.pump:64: // Use RunnableType::RunType instead of RunType above
because our
On 2011/11/10 19:33:39, willchan wrote:
> This is hard to grok. What's the difference between RunnableType, RunType, and
> RunnableType::RunType?

Naming this is tricky.

RunnableType.  Read the top comment in bind_internal.h first which defines all
the concepts.  That might clear it up. I'll put a reference to bind_internal.h's
taxonomy here.

Possible alternate names:

RunnableType -> {RunnableStorageType,Runnable}
RunType -> {EffectiveRunType}
RunnableType::RunType ... leave as is I think.

http://codereview.chromium.org/8483003/diff/16002/base/bind.h.pump
File base/bind.h.pump (right):

http://codereview.chromium.org/8483003/diff/16002/base/bind.h.pump#newcode93
base/bind.h.pump:93: // scoped_refptr check because the binder itself takes care
of this. We also
On 2011/11/11 01:30:25, willchan wrote:
> I'm not sure what skipping the scoped_refptr check means.

Reworded.

http://codereview.chromium.org/8483003/diff/16002/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8483003/diff/16002/base/bind_helpers.h#newcode222
base/bind_helpers.h:222: class HasIsMethodTag {
On 2011/11/11 01:30:25, willchan wrote:
> It's not obvious to me what's better. Just using a typedef false_type
IsMethod,
> or having this HasIsMethodTag. Is there a reason you chose to do it this way?

I didn't want to introduce an IsMethod typedef into Callback<>.  I'm open to
doing that.  Could go either way.

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump
File base/bind_internal.h.pump (right):

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:10: // "oh well"?
On 2011/11/11 01:30:25, willchan wrote:
> Assuming you mean statically enforce, I think it's possible, you could make
the
> Run() method be templated :P

Hah. I'd rather not do that.

The problem is I would need to inform the Callback<> type whether or not it is a
method.  It's doable with an extra template parameter in Callback<> that
defaults to false_type.  But seems like a lot of complexity.  COMPILE_ASSERT in
the 0-argument Bind() against HasIsMethod() tag seems better...

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:62: //                       object that adheres the
Runnable interface.
On 2011/11/11 01:30:25, willchan wrote:
> adheres *to*

Done.

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:75: //  MakeRunnable<> -- Takes a Functor an returns
an object in the Runnable
On 2011/11/11 01:30:25, willchan wrote:
> s/an/and/

Done.

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:271: // There are 3 logcaial InvokeHelper<>
specializations: normal, void-return,
On 2011/11/11 01:30:25, willchan wrote:
> logical

Done.

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:297: static ReturnType MakeItSo(Runnable runnable[[]]
On 2011/11/11 01:30:25, willchan wrote:
> MakeItSo, LOL!

;)  Wanted to get Engage() in there somehow too, but failed...

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:332: #if !defined(_MSC_VER)
On 2011/11/11 01:30:25, willchan wrote:
> What's up with this? Does VS not support this somehow?

This pissed me off.  I don't know what's wrong with MSVC.

This partial specialization should *not* be evaluated -- especially since
is_void<ReturnType> is a dependent type. But it does, which means this dumps an
error for each include.

I do not know how to fix.  Gave up.

http://codereview.chromium.org/8483003/diff/16002/base/bind_internal.h.pump#n...
base/bind_internal.h.pump:422: // This stores all the state passed into Bind()
and is also the where most
On 2011/11/11 01:30:25, willchan wrote:
> s/also the where/also where/

Done.

[willchan no longer on Chromium, 2011-11-11 21:32:48]

lgtm

[James Hawkins, 2011-11-19 01:33:12]

http://codereview.chromium.org/8483003/diff/23001/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8483003/diff/23001/base/bind_helpers.h#newcode393
base/bind_helpers.h:393: // IsWeakMethod determines is a helper to determine if
we are binding a
"determines is a helper to determine" -> need to fix the grammar.

[awong, 2011-11-21 20:11:18]

http://codereview.chromium.org/8483003/diff/23001/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8483003/diff/23001/base/bind_helpers.h#newcode393
base/bind_helpers.h:393: // IsWeakMethod determines is a helper to determine if
we are binding a
On 2011/11/19 01:33:12, James Hawkins wrote:
> "determines is a helper to determine" -> need to fix the grammar.

Done.

[joth, 2011-11-22 10:27:24]

I just noticed this appeared to reduce the size of windows and mac builds (22
and 48 kb resp) but increased the linux binary by 140kb (although, chrome-text
dipped by 70kb)
http://build.chromium.org/f/chromium/perf/linux-release/sizes/report.html?his...

Mostly just mentioning it here in passing / for the record, but there could be
something worth exploring further?

[awong, 2011-11-22 19:34:30]

Whoa, that's nuts.

Where's that extra 70kb coming from then?  I'm looking at chrome-bss and
chrome-data which don't seem to show 70kb of growth. Do you know if these builds
include debug symbols?  More symbols is expected...

-Albert

On 2011/11/22 10:27:24, joth wrote:
> I just noticed this appeared to reduce the size of windows and mac builds (22
> and 48 kb resp) but increased the linux binary by 140kb (although, chrome-text
> dipped by 70kb)
>
http://build.chromium.org/f/chromium/perf/linux-release/sizes/report.html?his...
> 
> Mostly just mentioning it here in passing / for the record, but there could be
> something worth exploring further?