code review 5503085: strconv: use a variable buffer size for decimal represe...

strconv: use a variable buffer size for decimal representations.

Fast cases usually only needed a small fraction of the previous
800-byte fixed size buffer.

                        old ns/op  new ns/op     diff
BenchmarkAtof64Decimal      327        192       -41%
BenchmarkAtof64Float        382        247       -35%
BenchmarkAtof64FloatExp     429        299       -30%
BenchmarkAtof64Big          710        582       -18%
BenchmarkAppendFloatDecimal 472        345       -27%
BenchmarkAppendFloat        483        357       -26%
BenchmarkAppendFloatExp     496        369       -26%
BenchmarkAppendFloatNegExp  485        361       -25%
BenchmarkAppendFloatBig     646        520       -19%

[remyoudompheng, 2011-12-31 11:51:01]

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

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

[remyoudompheng, 2011-12-31 11:51:56]

This changeset needs CL 5489128 to be applied first, otherwise it would
introduce unwanted allocations.

Rémy.

[sanjay.m, 2012-01-03 23:44:20]

Does this fix http://code.google.com/p/go/issues/detail?id=2642 ? I feel 
like changing to a variable length buffer would also solve this problem.

Sanjay

[remyoudompheng, 2012-01-04 05:50:27]

On 2012/1/4 sanjay.m <balasanjay@gmail.com> wrote:
> Does this fix http://code.google.com/p/go/issues/detail?id=2642 ? I feel
> like changing to a variable length buffer would also solve this problem.

It depends what you mean by variable length. As far as I understand,
the code was written like this in order to avoid dynamic allocations,
and the escape analysis code is not able to convert variable length
make()'s into stack allocations.

Rémy.

[sanjay.m, 2012-01-04 06:00:36]

Ahh, excuse me, from the CL title I thought you were changing it to be a 
dynamically growing slice for the buffer, but I see you're hardcoding to 32 
or 800.

Issue 2642 would only come up when trying to ParseFloat a string with a 
massive number of digits, and in this case, it causes a panic due to index 
out of range rather than returning an error, so this CL would not fix it.

A trivial fix would probably be to explicitly check for a write over the 
len of the array/slice (depending on whether this CL goes in or not). I can 
write it up once the fate of this CL is decided.

Cheers,

Sanjay

[remyoudompheng, 2012-01-14 11:21:41]

Hello golang-dev@googlegroups.com, balasanjay@gmail.com (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[rsc, 2012-01-16 23:06:21]

http://codereview.appspot.com/5503085/diff/8001/src/pkg/strconv/atof.go
File src/pkg/strconv/atof.go (right):

http://codereview.appspot.com/5503085/diff/8001/src/pkg/strconv/atof.go#newco...
src/pkg/strconv/atof.go:360: var d decimal
Before, decimal had an 800-byte buffer in it,
so there was one 800-byte buffer on the stack.

Now, with this code, assuming all the allocations end
up on the stack, we have a 32-byte buffer for one arm
of the if and an 800-byte buffer for the other arm.
And then another 800-byte buffer for decimalBuf below.
So the stack size has actually grown by 832 bytes, but
the 'fast path' is zeroing 768 fewer bytes, which must be
where the time savings comes from.

This is all rather subtle to me.  I wonder if there is a 
better way to make this faster.

[remyoudompheng, 2012-01-16 23:13:44]

It also looks very hacky to me. Actually none of the fast paths needs the buffer
at all, so for me the best solution is to leave the allocation to the slow path,
while for the fast paths, we only need a functions that parses a string into an
int64 mantissa and a decimal exponent.

[rsc, 2012-01-17 00:17:04]

Okay.  Let me know when I should take another look.

Thanks very much.
Russ

[remyoudompheng, 2012-04-07 09:30:23]

*** Abandoned ***