code review 119360043: runtime: convert hash functions to Go calling convention.

runtime: convert hash functions to Go calling convention.

Create proper closures so hash functions can be called
directly from Go.  Rearrange calling convention so return
value is directly accessible.

[khr, 2014-07-30 22:11:41]

Hello golang-codereviews@googlegroups.com,

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

[dvyukov, 2014-07-31 11:28:09]

LGTM

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

https://codereview.appspot.com/119360043/diff/120001/src/pkg/runtime/asm_amd6...
src/pkg/runtime/asm_amd64.s:963: MOVQ	8(SP), AX	// ptr to string struct
16(SP) is length of the string object itself (16), right?
please add a comment about what is 16(SP) and that it's unused
it's a bit unobvious

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

https://codereview.appspot.com/119360043/diff/120001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:140: func noescape(p unsafe.Pointer) unsafe.Pointer {
Does the following work?

//go:noescape
func noescape(p unsafe.Pointer) unsafe.Pointer {
  return p
}

Looks a bit cleaner to me. Also should eliminate xor and vet warning.

[dave_cheney.net, 2014-07-31 11:30:47]

On 31 Jul 2014 21:28, "dvyukov via golang-codereviews" <
golang-codereviews@googlegroups.com> wrote:
>
> LGTM
>
>
>
https://codereview.appspot.com/119360043/diff/120001/src/pkg/runtime/asm_amd64.s
> File src/pkg/runtime/asm_amd64.s (right):
>
>
https://codereview.appspot.com/119360043/diff/120001/src/pkg/runtime/asm_amd6...
> src/pkg/runtime/asm_amd64.s:963: MOVQ   8(SP), AX       // ptr to string
struct
> 16(SP) is length of the string object itself (16), right?
> please add a comment about what is 16(SP) and that it's unused
> it's a bit unobvious

Can you use the

ptr+8(SP)

style syntax ?
>
>
https://codereview.appspot.com/119360043/diff/120001/src/pkg/runtime/stubs.go
> File src/pkg/runtime/stubs.go (right):
>
>
https://codereview.appspot.com/119360043/diff/120001/src/pkg/runtime/stubs.go...
> src/pkg/runtime/stubs.go:140: func noescape(p unsafe.Pointer)
> unsafe.Pointer {
> Does the following work?
>
> //go:noescape
> func noescape(p unsafe.Pointer) unsafe.Pointer {
>   return p
> }
>
> Looks a bit cleaner to me. Also should eliminate xor and vet warning.
>
> https://codereview.appspot.com/119360043/
>
>
> --
> You received this message because you are subscribed to the Google Groups
"golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[dvyukov, 2014-07-31 11:41:53]

On Thu, Jul 31, 2014 at 3:30 PM, Dave Cheney <dave@cheney.net> wrote:
>
> On 31 Jul 2014 21:28, "dvyukov via golang-codereviews"
> <golang-codereviews@googlegroups.com> wrote:
>>
>> LGTM
>>
>>
>>
>>
https://codereview.appspot.com/119360043/diff/120001/src/pkg/runtime/asm_amd64.s
>> File src/pkg/runtime/asm_amd64.s (right):
>>
>>
>>
https://codereview.appspot.com/119360043/diff/120001/src/pkg/runtime/asm_amd6...
>> src/pkg/runtime/asm_amd64.s:963: MOVQ   8(SP), AX       // ptr to string
>> struct
>> 16(SP) is length of the string object itself (16), right?
>> please add a comment about what is 16(SP) and that it's unused
>> it's a bit unobvious
>
> Can you use the
>
> ptr+8(SP)
>
> style syntax ?

Then I guess it must be ptr+0(FP)

[khr1, 2014-07-31 21:29:50]

On Thu, Jul 31, 2014 at 4:28 AM, <dvyukov@google.com> wrote:

> LGTM
>
>
> https://codereview.appspot.com/119360043/diff/120001/src/
> pkg/runtime/asm_amd64.s
> File src/pkg/runtime/asm_amd64.s (right):
>
> https://codereview.appspot.com/119360043/diff/120001/src/
> pkg/runtime/asm_amd64.s#newcode963
> src/pkg/runtime/asm_amd64.s:963: MOVQ   8(SP), AX       // ptr to string
> struct
> 16(SP) is length of the string object itself (16), right?
> please add a comment about what is 16(SP) and that it's unused
> it's a bit unobvious
>
> https://codereview.appspot.com/119360043/diff/120001/src/
> pkg/runtime/stubs.go
> File src/pkg/runtime/stubs.go (right):
>
> https://codereview.appspot.com/119360043/diff/120001/src/
> pkg/runtime/stubs.go#newcode140
> src/pkg/runtime/stubs.go:140: func noescape(p unsafe.Pointer)
> unsafe.Pointer {
> Does the following work?
>
> //go:noescape
> func noescape(p unsafe.Pointer) unsafe.Pointer {
>   return p
> }
>
> Looks a bit cleaner to me. Also should eliminate xor and vet warning.
>
>
No, that doesn't work.  //go:noescape is only allowed on prototypes, not
implementations.
Of course we could change that rule.  I'll think about it, but not this CL.


> https://codereview.appspot.com/119360043/
>

[khr, 2014-07-31 22:07:08]

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

runtime: convert hash functions to Go calling convention.

Create proper closures so hash functions can be called
directly from Go.  Rearrange calling convention so return
value is directly accessible.

LGTM=dvyukov
R=golang-codereviews, dvyukov, dave, khr
CC=golang-codereviews
https://codereview.appspot.com/119360043

[gobot, 2014-07-31 22:07:49]

This CL appears to have broken the linux-amd64-race builder.
See http://build.golang.org/log/2839ba46f393bb31cb34c71f621afcb63913f0a2

[khr1, 2014-07-31 22:13:39]

Whoops, CL is missing a file.  Fixed.


On Thu, Jul 31, 2014 at 3:07 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the linux-amd64-race builder.
> See http://build.golang.org/log/2839ba46f393bb31cb34c71f621afcb63913f0a2
>
> https://codereview.appspot.com/119360043/
>

[adonovan, 2014-08-05 21:59:30]

On 2014/07/31 22:13:39, khr1 wrote:
> Whoops, CL is missing a file.  Fixed.
> 
> 
> On Thu, Jul 31, 2014 at 3:07 PM, <mailto:gobot@golang.org> wrote:
> 
> > This CL appears to have broken the linux-amd64-race builder.
> > See http://build.golang.org/log/2839ba46f393bb31cb34c71f621afcb63913f0a2
> >
> > https://codereview.appspot.com/119360043/
> >

Apologies for the false alarm yesterday; I think it was due to a stale
$GOROOT/pkg directory.
I think this CL is responsible for a catastrophic performance regression that
causes 68% of all CPU time in ssadump to be spent in map key comparisons.
Use this script to reproduce it:

#!/bin/bash

set -x

[ -z "$GOROOT" ] && { echo "Need GOROOT"; exit 1; }
[ -z "$GOPATH" ] && { echo "Need GOPATH"; exit 1; }

# These CLs are before/after "convert hash functions to Go calling convention".
# The reported times are 56.3283715ms, 5.496884316s.
for cl in c6b7f7454ffc d81fb87ee976; do
  # Build toolchain.
  cd $GOROOT/src
  hg up -r $cl
  ./make.bash

  # Build + run ssadump
  cd $GOPATH
  go get code.google.com/p/go.tools/cmd/ssadump
  rm -fr $GOPATH/pkg
  go build code.google.com/p/go.tools/cmd/ssadump
  time ./ssadump unicode
done

[dave_cheney.net, 2014-08-05 22:12:39]

I can confirm that the time to run the tests in the go.tools repo have
blown out from tens of seconds to tens of minutes.

On Wed, Aug 6, 2014 at 7:59 AM,  <adonovan@google.com> wrote:
> On 2014/07/31 22:13:39, khr1 wrote:
>>
>> Whoops, CL is missing a file.  Fixed.
>
>
>
>> On Thu, Jul 31, 2014 at 3:07 PM, <mailto:gobot@golang.org> wrote:
>
>
>> > This CL appears to have broken the linux-amd64-race builder.
>> > See
>
> http://build.golang.org/log/2839ba46f393bb31cb34c71f621afcb63913f0a2
>>
>> >
>> > https://codereview.appspot.com/119360043/
>> >
>
>
> Apologies for the false alarm yesterday; I think it was due to a stale
> $GOROOT/pkg directory.
> I think this CL is responsible for a catastrophic performance regression
> that causes 68% of all CPU time in ssadump to be spent in map key
> comparisons.
> Use this script to reproduce it:
>
> #!/bin/bash
>
> set -x
>
> [ -z "$GOROOT" ] && { echo "Need GOROOT"; exit 1; }
> [ -z "$GOPATH" ] && { echo "Need GOPATH"; exit 1; }
>
> # These CLs are before/after "convert hash functions to Go calling
> convention".
> # The reported times are 56.3283715ms, 5.496884316s.
> for cl in c6b7f7454ffc d81fb87ee976; do
>   # Build toolchain.
>   cd $GOROOT/src
>   hg up -r $cl
>   ./make.bash
>
>   # Build + run ssadump
>   cd $GOPATH
>   go get code.google.com/p/go.tools/cmd/ssadump
>   rm -fr $GOPATH/pkg
>   go build code.google.com/p/go.tools/cmd/ssadump
>   time ./ssadump unicode
> done
>
>
> https://codereview.appspot.com/119360043/

[rsc, 2014-08-06 18:15:36]

The problem is that ifacehash is returning 0 as the hash code for a non-empty
interface value. CL 125720044 should make this better.