code review 93380044: runtime: implement string ops in Go

runtime: implement string ops in Go

Also implement go:nosplit annotation.  Not really needed
for now, but we'll definitely need it for other conversions.

benchmark                 old ns/op     new ns/op     delta
BenchmarkRuneIterate      534           474           -11.24%
BenchmarkRuneIterate2     535           470           -12.15%

[khr, 2014-06-16 23:52:04]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://khr%40golang.org@code.google.com/p/go/

[dave_cheney.net, 2014-06-17 00:00:13]

On 2014/06/16 23:52:04, khr wrote:
> Hello mailto:golang-codereviews@googlegroups.com,
> 
> I'd like you to review this change to
> https://khr%40golang.org@code.google.com/p/go/

Thanks Keith. How would you feel about splitting out the eqstring -> asm change
into its own CL ?

[bradfitz, 2014-06-17 00:14:52]

Pull nosplit stuff out into its own CL that this one could then depend on? Maybe
not worth it.

https://codereview.appspot.com/93380044/diff/260001/src/pkg/runtime/string.c
File src/pkg/runtime/string.c (right):

https://codereview.appspot.com/93380044/diff/260001/src/pkg/runtime/string.c#...
src/pkg/runtime/string.c:1: // Copyright 2009 The Go Authors. All rights
reserved.
What's with this file? Just not converted yet, or will be removed later as C
dependencies are removed?

https://codereview.appspot.com/93380044/diff/260001/src/pkg/runtime/string.go
File src/pkg/runtime/string.go (right):

https://codereview.appspot.com/93380044/diff/260001/src/pkg/runtime/string.go...
src/pkg/runtime/string.go:12: var out string
or just keep the index into a instead, to save some stack? if you care (I note
it's called from nosplit functions)

https://codereview.appspot.com/93380044/diff/260001/src/pkg/runtime/string.go...
src/pkg/runtime/string.go:34: i := 0
or just re-use l?

[minux, 2014-06-17 00:17:31]

just looked at the assembly files.

you also need to make changes to asm_amd64p32.s.

https://codereview.appspot.com/93380044/diff/260001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/93380044/diff/260001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:564: MOVW	0(SP), R0
$-4 functions will not save return address onto the stack.

use this instead:
TEXT runtime·gogetcallerpc(SB),NOSPLIT,$-4-4
     MOVW R14, 0(FP)
     RET

[bradfitz, 2014-06-17 05:23:07]

LGTM

I didn't verify rune.c -> rune.go super closely, but assume it's right
(precedence, etc) if the tests all pass.

https://codereview.appspot.com/93380044/diff/280001/src/pkg/runtime/string.go
File src/pkg/runtime/string.go (right):

https://codereview.appspot.com/93380044/diff/280001/src/pkg/runtime/string.go...
src/pkg/runtime/string.go:11: func concatstrings(a []string) string {
I don't know the nosplit rules. Does this need to be nosplit if concatstringN
call it? Or is that implicit?

https://codereview.appspot.com/93380044/diff/280001/src/pkg/runtime/string.go...
src/pkg/runtime/string.go:166: func stringiter(s string, k int) int {
Might be nice to start documenting some of these. This one wasn't entirely
obvious, but mostly because I didn't remember the name/semantics of this entry
point from the compiler.

https://codereview.appspot.com/93380044/diff/280001/src/pkg/runtime/stubs.go
File src/pkg/runtime/stubs.go (right):

https://codereview.appspot.com/93380044/diff/280001/src/pkg/runtime/stubs.go#...
src/pkg/runtime/stubs.go:16: // The storage is not zeroed.  Callers should use
I think we've slowly converged on one space after periods. (here and two lines
later)

[khr, 2014-06-17 06:03:11]

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

runtime: implement string ops in Go

Also implement go:nosplit annotation.  Not really needed
for now, but we'll definitely need it for other conversions.

benchmark                 old ns/op     new ns/op     delta
BenchmarkRuneIterate      534           474           -11.24%
BenchmarkRuneIterate2     535           470           -12.15%

LGTM=bradfitz
R=golang-codereviews, dave, bradfitz, minux
CC=golang-codereviews
https://codereview.appspot.com/93380044

[gobot, 2014-06-17 06:05:10]

This CL appears to have broken the windows-amd64-race builder.
See http://build.golang.org/log/6d2cf7d788936c0ff8782ef5cafb5589644e3dab

[rsc, 2014-06-17 16:18:42]

LGTM but follow up with fixes

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/asm_386.s
File src/pkg/runtime/asm_386.s (right):

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/asm_386.s...
src/pkg/runtime/asm_386.s:785: MOVL	x+0(FP),AX		// addr of first arg
Please use p+0(FP) to match the prototype in stubs.go, or change the prototype
to say x.

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:864: MOVQ	AX,r+4(FP)
If you run 'go vet runtime' it will tell you that this should be r+8(FP) (well,
it tells you that +4(FP) is still 'p').

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/asm_amd64...
File src/pkg/runtime/asm_amd64p32.s (right):

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64p32.s:669: MOVL	AX, r+4(FP)
Same. This should be r+8.

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:564: MOVW	R14, 4(FP)
Please use r+4(FP) so that go vet can check it.

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/string.c
File src/pkg/runtime/string.c (right):

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/string.c#...
src/pkg/runtime/string.c:1: // Copyright 2009 The Go Authors. All rights
reserved.
For future CLs please use hg cp/mv (or, after the fact, hg cp/mv -A), so that hg
shows the diff against the old file (string.goc).
I assume this file is string.goc with a bunch of deletions.

That would help with the other conversions too (rune.c -> rune.go, etc).

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/string.go
File src/pkg/runtime/string.go (right):

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/string.go...
src/pkg/runtime/string.go:147: // Note: we need i to be the same type as
_string.len and to start at 0.
I'd like to find a way to use int here. (The old code used intgo.)
Perhaps instead of using _string we should define our own structure in this
file.

type stringStruct struct {
    str *byte
    len int
}

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/stubs.goc
File src/pkg/runtime/stubs.goc (right):

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/stubs.goc...
src/pkg/runtime/stubs.goc:22: // During conversion, we can still call out to
splittable
Can you be more explicit? During what conversion?
Do you mean:

// These invariants do not hold yet but will be established once we have
// finished converting runtime support code from C to Go.

?

[rsc, 2014-06-17 16:24:10]

I should add that you can run go vet to check assembly for other architectures
by using (for example)

GOARCH=amd64p32 go vet runtime
GOARCH=arm go vet runtime

[rsc, 2014-08-14 18:21:48]

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/string.go
File src/pkg/runtime/string.go (right):

https://codereview.appspot.com/93380044/diff/340001/src/pkg/runtime/string.go...
src/pkg/runtime/string.go:42: //go:nosplit
Are these go:nosplits necessary? I am running into nosplit chains being too
large here on power64, and I think I can just remove the go:nosplits.

The original C version was nosplit because it took an int and a list of strings,
and the split wouldn't know how big a list to copy. That's not an issue here.

[khr1, 2014-08-17 04:07:58]

These nosplits are not necessary.  They just emulate what we had in C and
lower the function overhead by a tiny bit.


On Thu, Aug 14, 2014 at 11:21 AM, <rsc@golang.org> wrote:

>
> https://codereview.appspot.com/93380044/diff/340001/src/
> pkg/runtime/string.go
> File src/pkg/runtime/string.go (right):
>
> https://codereview.appspot.com/93380044/diff/340001/src/
> pkg/runtime/string.go#newcode42
> src/pkg/runtime/string.go:42: //go:nosplit
> Are these go:nosplits necessary? I am running into nosplit chains being
> too large here on power64, and I think I can just remove the
> go:nosplits.
>
> The original C version was nosplit because it took an int and a list of
> strings, and the split wouldn't know how big a list to copy. That's not
> an issue here.
>
> https://codereview.appspot.com/93380044/
>

[minux, 2014-08-17 06:13:15]

On Aug 17, 2014 12:07 AM, "Keith Randall" <khr@google.com> wrote:
> These nosplits are not necessary.  They just emulate what we had in C and
lower the function overhead by a tiny bit.
thanks for confirmation, due to lack of optimization, on the power64
branch, their NOSPLIT flags are removed.