code review 4253049: fmt: make recursive scan more efficient.

fmt: make recursive scan more efficient.
Detect when scan is being called recursively and
re-use the same scan state.

On my machine, for a recursion-heavy benchmark, this
results in 44x speed up. This does impose a 4% penalty
on the non-recursive case, which can be removed by
heap-allocating the saved state, at 40% performance penalty
on the recursive case. Either way is fine with me.

[rog, 2011-03-03 11:07:52]

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

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

[r, 2011-03-03 17:44:43]

nice

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan.go#newcode321
src/pkg/fmt/scan.go:321: if s != nil {
i'd prefer comma, ok. but is that right? doing it this way, you imply that the
conversion could work but still give me nil, and that is all right. you're
confusing me

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan_test.go
File src/pkg/fmt/scan_test.go (right):

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan_test.go#newc...
src/pkg/fmt/scan_test.go:777: // but without recursing through scanner, so we
can compare
s/recursing/recurring/ recurse means to swear again

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan_test.go#newc...
src/pkg/fmt/scan_test.go:841: const intCount = 1000
move this above the test that uses it. threw me off

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan_test.go#newc...
src/pkg/fmt/scan_test.go:846: runtime.GC()
why?

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan_test.go#newc...
src/pkg/fmt/scan_test.go:859: runtime.GC()
why?

[rog, 2011-03-03 18:00:08]

PTAL

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan.go#newcode321
src/pkg/fmt/scan.go:321: if s != nil {
On 2011/03/03 17:44:43, r wrote:
> i'd prefer comma, ok. but is that right? doing it this way, you imply that the
> conversion could work but still give me nil, and that is all right. you're
> confusing me

this package is the only place that can create a *ss, so we know that if the
conversion succeeds then it is always going to be non nil.
but comma, ok is clearer. done.

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan_test.go
File src/pkg/fmt/scan_test.go (right):

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan_test.go#newc...
src/pkg/fmt/scan_test.go:777: // but without recursing through scanner, so we
can compare
On 2011/03/03 17:44:43, r wrote:
> s/recursing/recurring/ recurse means to swear again

Done.

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan_test.go#newc...
src/pkg/fmt/scan_test.go:841: const intCount = 1000
On 2011/03/03 17:44:43, r wrote:
> move this above the test that uses it. threw me off

Done.

http://codereview.appspot.com/4253049/diff/8001/src/pkg/fmt/scan_test.go#newc...
src/pkg/fmt/scan_test.go:846: runtime.GC()
On 2011/03/03 17:44:43, r wrote:
> why?

because NewBuffer creates garbage every time through the loop and i don't want
the garbage collection overhead to mess up the timing. to be honest, i think
testing should do a GC before calling a benchmark function anyway.
also it would be nice if Buffer.Reset took a []byte argument so that it was
possible to reset the contents without allocating a new buffer. i'll delete the
GC line if you prefer.

[r, 2011-03-03 18:14:48]

http://codereview.appspot.com/4253049/diff/10002/src/pkg/fmt/scan_test.go
File src/pkg/fmt/scan_test.go (right):

http://codereview.appspot.com/4253049/diff/10002/src/pkg/fmt/scan_test.go#new...
src/pkg/fmt/scan_test.go:859: runtime.GC()
i guess what's throwing me off is that you're doing the GC inside the benchmark.
why not before b.ResetTimer?

you have a good point about maybe doing gc before calling the benchmarks.
i suggest you make a CL - and hold off submitting this until then. it's such a
small change the decision to do it should be quick.

[rog, 2011-03-03 18:26:27]

On 3 March 2011 18:14,  <r@golang.org> wrote:
>
> http://codereview.appspot.com/4253049/diff/10002/src/pkg/fmt/scan_test.go
> File src/pkg/fmt/scan_test.go (right):
>
>
http://codereview.appspot.com/4253049/diff/10002/src/pkg/fmt/scan_test.go#new...
> src/pkg/fmt/scan_test.go:859: runtime.GC()
> i guess what's throwing me off is that you're doing the GC inside the
> benchmark. why not before b.ResetTimer?

ResetTimer stops the timer. (it would probably be better
if it didn't)

> you have a good point about maybe doing gc before calling the
> benchmarks.
> i suggest you make a CL - and hold off submitting this until then. it's
> such a small change the decision to do it should be quick.

ok will do.

[rog, 2011-03-03 18:37:02]

PTAL. hackery removed.

On 3 March 2011 18:14,  <r@golang.org> wrote:
>
> http://codereview.appspot.com/4253049/diff/10002/src/pkg/fmt/scan_test.go
> File src/pkg/fmt/scan_test.go (right):
>
>
http://codereview.appspot.com/4253049/diff/10002/src/pkg/fmt/scan_test.go#new...
> src/pkg/fmt/scan_test.go:859: runtime.GC()
> i guess what's throwing me off is that you're doing the GC inside the
> benchmark. why not before b.ResetTimer?
>
> you have a good point about maybe doing gc before calling the
> benchmarks.
> i suggest you make a CL - and hold off submitting this until then. it's
> such a small change the decision to do it should be quick.
>
> http://codereview.appspot.com/4253049/
>

[r, 2011-03-03 18:43:34]

*** Submitted as http://code.google.com/p/go/source/detail?r=d46104f37547 ***

fmt: make recursive scan more efficient.
Detect when scan is being called recursively and
re-use the same scan state.

On my machine, for a recursion-heavy benchmark, this
results in 44x speed up. This does impose a 4% penalty
on the non-recursive case, which can be removed by
heap-allocating the saved state, at 40% performance penalty
on the recursive case. Either way is fine with me.

R=r
CC=golang-dev
http://codereview.appspot.com/4253049

Committer: Rob Pike <r@golang.org>