code review 12289043: bytes: move IndexByte assembly to pkg runtime

bytes: move IndexByte assembly to pkg runtime

Per suggestion from Russ in February. Then strings.IndexByte
can be implemented in terms of the shared code in pkg runtime.

Update Issue 3751

[bradfitz, 2013-08-01 22:31:19]

Hello golang-dev@googlegroups.com,

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

[r, 2013-08-01 22:35:08]

LGTM

[bradfitz, 2013-08-01 23:11:22]

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

bytes: move IndexByte assembly to pkg runtime

Per suggestion from Russ in February. Then strings.IndexByte
can be implemented in terms of the shared code in pkg runtime.

Update Issue 3751

R=golang-dev, r
CC=golang-dev
https://codereview.appspot.com/12289043

[dave_cheney.net, 2013-08-01 23:41:48]

On 2013/08/01 23:11:22, bradfitz wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=c8da77d7e912 ***
> 
> bytes: move IndexByte assembly to pkg runtime
> 
> Per suggestion from Russ in February. Then strings.IndexByte
> can be implemented in terms of the shared code in pkg runtime.
> 
> Update Issue 3751
> 
> R=golang-dev, r
> CC=golang-dev
> https://codereview.appspot.com/12289043

Hi Brad,

Did you do any performance benchmarks ? When we tried this before Go 1.1 shipped
this caused a marked regression for arm. I'll try some benchmarks of this change
this afternoon.

Cheers

Dave

[bradfitz, 2013-08-01 23:43:30]

This just moves code. I didn't benchmark anything.

Are you talking about something else or did this actually slow things down
in the past?
 On Aug 1, 2013 4:41 PM, <dave@cheney.net> wrote:

> On 2013/08/01 23:11:22, bradfitz wrote:
>
>> *** Submitted as
>>
>
https://code.google.com/p/go/**source/detail?r=c8da77d7e912<https://code.goog...
>
>  bytes: move IndexByte assembly to pkg runtime
>>
>
>  Per suggestion from Russ in February. Then strings.IndexByte
>> can be implemented in terms of the shared code in pkg runtime.
>>
>
>  Update Issue 3751
>>
>
>  R=golang-dev, r
>> CC=golang-dev
>>
https://codereview.appspot.**com/12289043<https://codereview.appspot.com/1228...
>>
>
> Hi Brad,
>
> Did you do any performance benchmarks ? When we tried this before Go 1.1
> shipped this caused a marked regression for arm. I'll try some
> benchmarks of this change this afternoon.
>
> Cheers
>
> Dave
>
>
https://codereview.appspot.**com/12289043/<https://codereview.appspot.com/122...
>

[dave_cheney.net, 2013-08-01 23:45:32]

I may have been thinking about the bytes.Equals unification that we
tried in in Go 1.1 which caused some weird regression for arm; it was
only one more unconditional branch.

Thanks for responding, I'll run so bunchmarks of this change, I agree
it shouldn't cause an issue, but arm always complicates assumptions
like this.

On Fri, Aug 2, 2013 at 9:43 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> This just moves code. I didn't benchmark anything.
>
> Are you talking about something else or did this actually slow things down
> in the past?
>
> On Aug 1, 2013 4:41 PM, <dave@cheney.net> wrote:
>>
>> On 2013/08/01 23:11:22, bradfitz wrote:
>>>
>>> *** Submitted as
>>
>> https://code.google.com/p/go/source/detail?r=c8da77d7e912 ***
>>
>>> bytes: move IndexByte assembly to pkg runtime
>>
>>
>>> Per suggestion from Russ in February. Then strings.IndexByte
>>> can be implemented in terms of the shared code in pkg runtime.
>>
>>
>>> Update Issue 3751
>>
>>
>>> R=golang-dev, r
>>> CC=golang-dev
>>> https://codereview.appspot.com/12289043
>>
>>
>> Hi Brad,
>>
>> Did you do any performance benchmarks ? When we tried this before Go 1.1
>> shipped this caused a marked regression for arm. I'll try some
>> benchmarks of this change this afternoon.
>>
>> Cheers
>>
>> Dave
>>
>> https://codereview.appspot.com/12289043/
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[dave_cheney.net, 2013-08-02 01:09:41]

LGTM. Benchmarks are fine, sorry for the false alarm.