Extend base::Callback to 8 arguments.

Extend base::Callback to 8 arguments.

This is required by the Android port in order to support using base::Bind with
the FaviconService::GetRawFaviconForURL method. This is used in code yet in process of being upstreamed (bug 138755).

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

[Leandro Graciá Gil, 2012-09-03 02:04:03]

jar: Generic review and owner approval.

Thanks!

[jar (doing other things), 2012-09-04 19:15:14]

New code at least looks just like existing code... even with funky readability
issues (violation of readability... that compacts the code to fewer lines).

I tried to review a lot of it... but my eyes glazed a bit.. and I think it is
fine.

LGTM

[Leandro Graciá Gil, 2012-09-04 20:06:54]

On 2012/09/04 19:15:14, jar wrote:
> New code at least looks just like existing code... even with funky readability
> issues (violation of readability... that compacts the code to fewer lines).
> 
> I tried to review a lot of it... but my eyes glazed a bit.. and I think it is
> fine.
> 
> LGTM

Yes, as you say it's basically an extension of the existing code, and it's quite
easy to get lost on it. I've gone through it quite a few times to make sure I'm
not missing anything.

Thanks for the review.

[commit-bot: I haz the power, 2012-09-04 20:13:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/10919055/1

[commit-bot: I haz the power, 2012-09-04 22:04:29]

Try job failure for 10919055-1 (retry) on linux_rel for step
"interactive_ui_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-09-04 22:10:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/10919055/1

[commit-bot: I haz the power, 2012-09-05 03:00:00]

Change committed as 154885

[Ryan Sleevi, 2012-09-05 03:06:17]

On 2012/09/05 03:00:00, I haz the power (commit-bot) wrote:
> Change committed as 154885

Drive-by: Please monitor compilation times of the waterfall for this.

In the past, extensions to the arity base::Bind() caused a 1-3% slowdown of
compile times across the board (+cc awong, who IIRC, was doing the
measurements).

If you're at a point where you need 8-arity, perhaps consider using a struct
rather than a set of parameters. This lets you make a trade-off between a
localized perf hit (perhaps due to invoking copy ctors) and a global perf hit
(due to template resolution that all callers have to provide)

[Peter Kasting, 2012-09-05 03:09:55]

On 2012/09/05 03:06:17, Ryan Sleevi wrote:
> If you're at a point where you need 8-arity, perhaps consider using a struct
> rather than a set of parameters. This lets you make a trade-off between a
> localized perf hit (perhaps due to invoking copy ctors) and a global perf hit
> (due to template resolution that all callers have to provide)

Indeed, I would drive-by suggest that any function that needs 8 args should be a
candidate for structification even if Bind() is not involved.

[awong, 2012-09-05 03:15:21]

On 2012/09/05 03:09:55, Peter Kasting wrote:
> On 2012/09/05 03:06:17, Ryan Sleevi wrote:
> > If you're at a point where you need 8-arity, perhaps consider using a struct
> > rather than a set of parameters. This lets you make a trade-off between a
> > localized perf hit (perhaps due to invoking copy ctors) and a global perf
hit
> > (due to template resolution that all callers have to provide)
> 
> Indeed, I would drive-by suggest that any function that needs 8 args should be
a
> candidate for structification even if Bind() is not involved.

Ack! Please revert this change.

First off, you should not be modifying any of these files by hand.  See the
first 3 lines of each file.  You should only ever modify the the .pump files and
then regenerate these.  Otherwise it is impossible to review (as jar@ noted) and
future maintainers will happily wipe out your changes by mistake.

Second, Ryan and Peter are right that we are the limit of acceptable compile
time (specifically on windows) that at 8-arity, using a struct is probably a
better solution.

Compile cost grows N^2 with this implementation due to the lack of templated
vaargs in c++98 so we try very very hard not to increase this number.

[Leandro Gracia Gil, 2012-09-05 04:00:41]

Oops, sorry for the inconveniences.

This problem started with this patch, which added additional parameters to
the FaviconService methods: https://chromiumcodereview.appspot.com/10870022/

In the downstream Android code (in process of being upstreamed) we have
some code that called GetFaviconForURL and that needs to make use of
base::Bind. As a consequence of that change the new method has too many
arguments for base::Bind, which led to the current patch.

I'm open to any other solutions, but we do need to use base::Bind and to
call these methods. What do you suggest? I'll be happy to revert this
patch, but I'd like to have a backup plan if possible since once we merge
the revert our downstream build will break again.

Thanks and again, sorry for the inconveniences.
Leandro


On 4 September 2012 20:15, <ajwong@chromium.org> wrote:

> On 2012/09/05 03:09:55, Peter Kasting wrote:
>
>> On 2012/09/05 03:06:17, Ryan Sleevi wrote:
>> > If you're at a point where you need 8-arity, perhaps consider using a
>> struct
>> > rather than a set of parameters. This lets you make a trade-off between
>> a
>> > localized perf hit (perhaps due to invoking copy ctors) and a global
>> perf
>>
> hit
>
>> > (due to template resolution that all callers have to provide)
>>
>
>  Indeed, I would drive-by suggest that any function that needs 8 args
>> should be
>>
> a
>
>> candidate for structification even if Bind() is not involved.
>>
>
> Ack! Please revert this change.
>
> First off, you should not be modifying any of these files by hand.  See the
> first 3 lines of each file.  You should only ever modify the the .pump
> files and
> then regenerate these.  Otherwise it is impossible to review (as jar@noted)
and
> future maintainers will happily wipe out your changes by mistake.
>
> Second, Ryan and Peter are right that we are the limit of acceptable
> compile
> time (specifically on windows) that at 8-arity, using a struct is probably
> a
> better solution.
>
> Compile cost grows N^2 with this implementation due to the lack of
> templated
> vaargs in c++98 so we try very very hard not to increase this number.
>
>
https://chromiumcodereview.**appspot.com/10919055/<https://chromiumcodereview...
>

[Leandro Graciá Gil, 2012-09-05 04:11:57]

I'll be preparing a patch to use structures for these FaviconService methods
that have too many arguments. If you don't mind, I'd be grateful if we could
revert the base::Bind patch once the structification goes in. I hope all of this
should be done by tomorrow. Otherwise, if you think we should revert it now
please let me know and I'll figure something out for the downstream Android
build.

On 2012/09/05 04:00:41, Leandro Gracia Gil wrote:
> Oops, sorry for the inconveniences.
> 
> This problem started with this patch, which added additional parameters to
> the FaviconService methods: https://chromiumcodereview.appspot.com/10870022/
> 
> In the downstream Android code (in process of being upstreamed) we have
> some code that called GetFaviconForURL and that needs to make use of
> base::Bind. As a consequence of that change the new method has too many
> arguments for base::Bind, which led to the current patch.
> 
> I'm open to any other solutions, but we do need to use base::Bind and to
> call these methods. What do you suggest? I'll be happy to revert this
> patch, but I'd like to have a backup plan if possible since once we merge
> the revert our downstream build will break again.
> 
> Thanks and again, sorry for the inconveniences.
> Leandro
> 
> 
> On 4 September 2012 20:15, <mailto:ajwong@chromium.org> wrote:
> 
> > On 2012/09/05 03:09:55, Peter Kasting wrote:
> >
> >> On 2012/09/05 03:06:17, Ryan Sleevi wrote:
> >> > If you're at a point where you need 8-arity, perhaps consider using a
> >> struct
> >> > rather than a set of parameters. This lets you make a trade-off between
> >> a
> >> > localized perf hit (perhaps due to invoking copy ctors) and a global
> >> perf
> >>
> > hit
> >
> >> > (due to template resolution that all callers have to provide)
> >>
> >
> >  Indeed, I would drive-by suggest that any function that needs 8 args
> >> should be
> >>
> > a
> >
> >> candidate for structification even if Bind() is not involved.
> >>
> >
> > Ack! Please revert this change.
> >
> > First off, you should not be modifying any of these files by hand.  See the
> > first 3 lines of each file.  You should only ever modify the the .pump
> > files and
> > then regenerate these.  Otherwise it is impossible to review (as jar@noted)
> and
> > future maintainers will happily wipe out your changes by mistake.
> >
> > Second, Ryan and Peter are right that we are the limit of acceptable
> > compile
> > time (specifically on windows) that at 8-arity, using a struct is probably
> > a
> > better solution.
> >
> > Compile cost grows N^2 with this implementation due to the lack of
> > templated
> > vaargs in c++98 so we try very very hard not to increase this number.
> >
> >
>
https://chromiumcodereview.**appspot.com/10919055/%3Chttps://chromiumcoderevi...>
> >