code review 5372086: big/rat: add a few benchmarks for FloatString conversio...

big/rat: add a few benchmarks for FloatString conversion (it's slow)

[dga, 2011-11-13 15:10:50]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-11-13 16:46:25]

http://codereview.appspot.com/5372086/diff/3/src/pkg/math/big/rat_test.go
File src/pkg/math/big/rat_test.go (right):

http://codereview.appspot.com/5372086/diff/3/src/pkg/math/big/rat_test.go#new...
src/pkg/math/big/rat_test.go:501: func RatStringHelper(b *testing.B, num, denom
string, precision int) {
there's no reason for these helpers to be exported.
there is reason for them to be commented.

http://codereview.appspot.com/5372086/diff/3/src/pkg/math/big/rat_test.go#new...
src/pkg/math/big/rat_test.go:510: b.StartTimer()
i find it odd that the timer management is distributed like this. it's usually
better design to manage resources within a single function.

in any case, this loop is so trivial i'd just delete this function and move its
contents to its two locations. also change RatStringHelper to
benchmarkFloatString

[dga, 2011-11-16 14:37:31]

Hello golang-dev@googlegroups.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[gri, 2011-11-16 17:51:42]

This is fine but seems overkill to have 8 benchmarks for FloatString conversion,
each of which tests essentially one case. Ideally, there's one, perhaps two
benchmarks (one for "short" and one for very long results), each of which timing
the conversion of a good mix of numbers, so to get a somewhat representative
result for average output.

Can you combine them? I am thinking one benchmark for short and one for long
numbers, and each one converts a list of (say 10) numbers per iteration. Then,
the tests can be table-driven, and the number of functions can be reduced to
two.

- gri

[gri, 2011-11-17 21:52:08]

On Thu, Nov 17, 2011 at 9:37 AM, Rob 'Commander' Pike <r@google.com> wrote:
>
> On Nov 17, 2011, at 9:31 AM, dave.andersen@gmail.com wrote:
>
>> I agree completely for shorts - I'll update that now.

sounds good

>>
>> For longs, I had left them separate because each seemed likely to have a
>> different set of optimizations available, and the runtime was so long as
>> to make it unclear that running them all together would be a good
>> benchmark.  Would it be reasonable if I left them separate *for now*,
>> with the thought that I'll fold them together as a "medium" class once
>> I've also gotten the patches to speed things up submitted, and then
>> redefine a new set of "long" ones that still run for a while?
>
> You're arguing about benchmarks for a hypothetical condition. Should someone
decide to implement distinct optimizations, that would be a good time to
separate the conditions under which the optimizations occur and see their
effect.

agreed - the benchmarks make much more sense if they come with
optimizations the effect of which then can be measured

>
> Now, since you say you plan to make some optimizations, why not do it then?
The CL can include optimizations, new benchmarks, and in the description
document the improvement.

There's a CL of integer printing optimizations (long pending) that I
am reviewing. It's likely to have an impact on large number (and thus
float/rational) printing. I'd rather not do any tweaks at the moment.

>
> Although maybe everyone is overthinking here.

possibly. which is why I was concerned about adding 8 benchmarks w/o
much meaning in the first place.

- gri


>
> -rob
>
>

[dga, 2011-11-17 22:04:02]

On Thu, Nov 17, 2011 at 1:15 PM, Robert Griesemer <gri@golang.org> wrote:

>>
>> What's the CL for the integer printing optimizations?  Mine's ready to
>> submit - should have looked at the list of CLs first.  Oops.
>
> http://codereview.appspot.com/4573041/

>
>>
>> The relevant benchmark changes for mine are that Medium is 2.7x
>> faster, long is 5.5x faster:
>
> that's good, too. please send me that CL.

> Have you uploaded those changes?

Nope - I'll get them as soon as rietveld is back.  They need a little
cleaning to be up to snuff but I should upload them within three
hours.

From looking at the other CL, the changes look very compatible.  My
changes are algorithmic - it switches to divide-and-conquer for large
numbers, with better asymptotic perfornamce.

  -Dave

[gri, 2011-11-17 22:07:45]

On Thu, Nov 17, 2011 at 9:49 AM, David Andersen <dave.andersen@gmail.com> wrote:
> On Thu, Nov 17, 2011 at 12:44 PM, Robert Griesemer <gri@golang.org> wrote:
>>>
>>> Now, since you say you plan to make some optimizations, why not do it then?
The CL can include optimizations, new benchmarks, and in the description
document the improvement.
>>
>> There's a CL of integer printing optimizations (long pending) that I
>> am reviewing. It's likely to have an impact on large number (and thus
>> float/rational) printing. I'd rather not do any tweaks at the moment.
>>
>>>
>>> Although maybe everyone is overthinking here.
>>
>> possibly. which is why I was concerned about adding 8 benchmarks w/o
>> much meaning in the first place.
>
> Sounds good.  I'll stuff them all together.
>
> What's the CL for the integer printing optimizations?  Mine's ready to
> submit - should have looked at the list of CLs first.  Oops.

http://codereview.appspot.com/4573041/


>
> The relevant benchmark changes for mine are that Medium is 2.7x
> faster, long is 5.5x faster:

that's good, too. please send me that CL.

>
> Old:
> big.BenchmarkStringShort10        100000             27315 ns/op
> big.BenchmarkStringMedium10          500           3237750 ns/op
> big.BenchmarkStringLong10              5         516262600 ns/op
>
> With changes:
> big.BenchmarkStringShort10        100000             31003 ns/op
> big.BenchmarkStringMedium10         2000           1186873 ns/op
> big.BenchmarkStringLong10             20          94076850 ns/op
>
> (The change to short10 is, I hope, in the noise, or I can special case it.)

Have you uploaded those changes?
- gri

>
>  -Dave
>

[dga, 2011-11-17 22:20:44]

Hello golang-dev@googlegroups.com, r@golang.org, gri@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r2, 2011-11-17 22:25:07]

On Nov 17, 2011, at 9:31 AM, dave.andersen@gmail.com wrote:

> I agree completely for shorts - I'll update that now.
> 
> For longs, I had left them separate because each seemed likely to have a
> different set of optimizations available, and the runtime was so long as
> to make it unclear that running them all together would be a good
> benchmark.  Would it be reasonable if I left them separate *for now*,
> with the thought that I'll fold them together as a "medium" class once
> I've also gotten the patches to speed things up submitted, and then
> redefine a new set of "long" ones that still run for a while?

You're arguing about benchmarks for a hypothetical condition. Should someone
decide to implement distinct optimizations, that would be a good time to
separate the conditions under which the optimizations occur and see their
effect.

Now, since you say you plan to make some optimizations, why not do it then? The
CL can include optimizations, new benchmarks, and in the description document
the improvement.

Although maybe everyone is overthinking here.

-rob

[dga, 2011-11-17 22:35:36]

On Thu, Nov 17, 2011 at 12:44 PM, Robert Griesemer <gri@golang.org> wrote:
>>
>> Now, since you say you plan to make some optimizations, why not do it then?
The CL can include optimizations, new benchmarks, and in the description
document the improvement.
>
> There's a CL of integer printing optimizations (long pending) that I
> am reviewing. It's likely to have an impact on large number (and thus
> float/rational) printing. I'd rather not do any tweaks at the moment.
>
>>
>> Although maybe everyone is overthinking here.
>
> possibly. which is why I was concerned about adding 8 benchmarks w/o
> much meaning in the first place.

Sounds good.  I'll stuff them all together.

What's the CL for the integer printing optimizations?  Mine's ready to
submit - should have looked at the list of CLs first.  Oops.

The relevant benchmark changes for mine are that Medium is 2.7x
faster, long is 5.5x faster:

Old:
big.BenchmarkStringShort10	  100000	     27315 ns/op
big.BenchmarkStringMedium10	     500	   3237750 ns/op
big.BenchmarkStringLong10	       5	 516262600 ns/op

With changes:
big.BenchmarkStringShort10	  100000	     31003 ns/op
big.BenchmarkStringMedium10	    2000	   1186873 ns/op
big.BenchmarkStringLong10	      20	  94076850 ns/op

(The change to short10 is, I hope, in the noise, or I can special case it.)

  -Dave

[dga, 2011-11-17 22:40:32]

On Thu, Nov 17, 2011 at 1:44 PM, David Andersen <dave.andersen@gmail.com> wrote:

>>> The relevant benchmark changes for mine are that Medium is 2.7x
>>> faster, long is 5.5x faster:
>>
>> that's good, too. please send me that CL.
>
>> Have you uploaded those changes?
>
> Nope - I'll get them as soon as rietveld is back.  They need a little
> cleaning to be up to snuff but I should upload them within three
> hours.
>
> From looking at the other CL, the changes look very compatible.  My
> changes are algorithmic - it switches to divide-and-conquer for large
> numbers, with better asymptotic performance.

The changes to decimal are now uploaded: http://codereview.appspot.com/5413043

This is my first larger change to anything within the go libraries, so
I apologize in advance. :-)

  -Dave