code review 4267050: strings: add IndexRune tests, ASCII fast path

strings: add IndexRune tests, ASCII fast path

$ gotest -test.v -test.run=IndexRune -test.bench=.*
=== RUN  strings_test.TestIndexRune
--- PASS: strings_test.TestIndexRune (0.0 seconds)
PASS
strings_test.BenchmarkIndexRune	20000000   105 ns/op
strings_test.BenchmarkIndexByte	50000000    48 ns/op

[bradfitz, 2011-03-08 17:24:13]

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

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

[dsymonds, 2011-03-08 17:27:13]

On Tue, Mar 8, 2011 at 9:24 AM,  <bradfitz@golang.org> wrote:

> $ gotest -test.v -test.run=IndexRune -test.bench=.*
> === RUN  strings_test.TestIndexRune
> --- PASS: strings_test.TestIndexRune (0.0 seconds)
> PASS
> strings_test.BenchmarkIndexRune 20000000   105 ns/op
> strings_test.BenchmarkIndexByte 50000000    48 ns/op

What was the before statistics on your machine?



Dave.

[rsc, 2011-03-08 17:29:23]

LGTM

[bradfitz, 2011-03-08 17:37:31]

On Tue, Mar 8, 2011 at 9:27 AM, David Symonds <dsymonds@golang.org> wrote:

> On Tue, Mar 8, 2011 at 9:24 AM,  <bradfitz@golang.org> wrote:
>
> > $ gotest -test.v -test.run=IndexRune -test.bench=.*
> > === RUN  strings_test.TestIndexRune
> > --- PASS: strings_test.TestIndexRune (0.0 seconds)
> > PASS
> > strings_test.BenchmarkIndexRune 20000000   105 ns/op
> > strings_test.BenchmarkIndexByte 50000000    48 ns/op
>
> What was the before statistics on your machine?
>
>
They were the same before.   Both 100-ish ns.

[dsymonds, 2011-03-08 17:38:56]

On Tue, Mar 8, 2011 at 9:37 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> On Tue, Mar 8, 2011 at 9:27 AM, David Symonds <dsymonds@golang.org> wrote:
>>
>> On Tue, Mar 8, 2011 at 9:24 AM,  <bradfitz@golang.org> wrote:
>>
>> > $ gotest -test.v -test.run=IndexRune -test.bench=.*
>> > === RUN  strings_test.TestIndexRune
>> > --- PASS: strings_test.TestIndexRune (0.0 seconds)
>> > PASS
>> > strings_test.BenchmarkIndexRune 20000000   105 ns/op
>> > strings_test.BenchmarkIndexByte 50000000    48 ns/op
>>
>> What was the before statistics on your machine?
>>
>
> They were the same before.   Both 100-ish ns.

So the fast path does not help?

[bradfitz, 2011-03-08 17:41:15]

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

strings: add IndexRune tests, ASCII fast path

$ gotest -test.v -test.run=IndexRune -test.bench=.*
=== RUN  strings_test.TestIndexRune
--- PASS: strings_test.TestIndexRune (0.0 seconds)
PASS
strings_test.BenchmarkIndexRune	20000000   105 ns/op
strings_test.BenchmarkIndexByte	50000000    48 ns/op

R=rsc, dsymonds
CC=golang-dev
http://codereview.appspot.com/4267050

[bradfitzgoog, 2011-03-08 17:42:45]

On Tue, Mar 8, 2011 at 9:38 AM, David Symonds <dsymonds@golang.org> wrote:

> On Tue, Mar 8, 2011 at 9:37 AM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
>
> > On Tue, Mar 8, 2011 at 9:27 AM, David Symonds <dsymonds@golang.org>
> wrote:
> >>
> >> On Tue, Mar 8, 2011 at 9:24 AM,  <bradfitz@golang.org> wrote:
> >>
> >> > $ gotest -test.v -test.run=IndexRune -test.bench=.*
> >> > === RUN  strings_test.TestIndexRune
> >> > --- PASS: strings_test.TestIndexRune (0.0 seconds)
> >> > PASS
> >> > strings_test.BenchmarkIndexRune 20000000   105 ns/op
> >> > strings_test.BenchmarkIndexByte 50000000    48 ns/op
> >>
> >> What was the before statistics on your machine?
> >>
> >
> > They were the same before.   Both 100-ish ns.
>
> So the fast path does not help?
>

It made it twice as fast.

My test names are probably confusing you (originating from me originally
going down the path of adding a new IndexByte func but scrapping that in
favor of just speeding up IndexRune).

[dsymonds, 2011-03-08 17:45:00]

LGTM

On Tue, Mar 8, 2011 at 9:42 AM, Brad Fitzpatrick <bradfitz@google.com> wrote:

> It made it twice as fast.
> My test names are probably confusing you (originating from me originally
> going down the path of adding a new IndexByte func but scrapping that in
> favor of just speeding up IndexRune).

Aah, got it now. I didn't study the tests. Yeah, better names would
have helped.  ;-)


Dave.

[r, 2011-03-08 17:58:27]

http://codereview.appspot.com/4267050/diff/1003/src/pkg/strings/strings.go
File src/pkg/strings/strings.go (right):

http://codereview.appspot.com/4267050/diff/1003/src/pkg/strings/strings.go#ne...
src/pkg/strings/strings.go:123: case rune < 0x80:
why not an if statement?  when i see 'switch' i think something more interesting
is coming than a single comparison.

[bradfitzgoog, 2011-03-08 18:01:00]

http://codereview.appspot.com/4267050/diff/1003/src/pkg/strings/strings.go
File src/pkg/strings/strings.go (right):

http://codereview.appspot.com/4267050/diff/1003/src/pkg/strings/strings.go#ne...
src/pkg/strings/strings.go:123: case rune < 0x80:
On 2011/03/08 17:58:27, r wrote:
> why not an if statement?  when i see 'switch' i think something more
interesting
> is coming than a single comparison.

I always got the impression that Go was switch-happy. I tend to use if
statements but I've been told a number of times to use a switch instead. From
now on I'll use if when there's fewer than 3 cases.

[r2, 2011-03-08 18:05:49]

On Mar 8, 2011, at 10:01 AM, bradfitz@google.com wrote:

> 
> http://codereview.appspot.com/4267050/diff/1003/src/pkg/strings/strings.go
> File src/pkg/strings/strings.go (right):
> 
>
http://codereview.appspot.com/4267050/diff/1003/src/pkg/strings/strings.go#ne...
> src/pkg/strings/strings.go:123: case rune < 0x80:
> On 2011/03/08 17:58:27, r wrote:
>> why not an if statement?  when i see 'switch' i think something more
> interesting
>> is coming than a single comparison.
> 
> I always got the impression that Go was switch-happy. I tend to use if
> statements but I've been told a number of times to use a switch instead.
> From now on I'll use if when there's fewer than 3 cases.

thanks.  switches beat long if-else chains for sure.  how long the chain must be
is debatable but it's certainly greater than 2.

-rob