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.
On Thu, Jul 31, 2014 at 4:28 AM, <dvyukov@google.com> wrote: > LGTM > > > ...
10 years, 7 months ago
(2014-07-31 21:29:50 UTC)
#5
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/
>
*** Submitted as https://code.google.com/p/go/source/detail?r=2aa6648d7146 *** runtime: convert hash functions to Go calling convention. Create proper ...
10 years, 7 months ago
(2014-07-31 22:07:08 UTC)
#6
*** 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
On 2014/07/31 22:13:39, khr1 wrote: > Whoops, CL is missing a file. Fixed. > > ...
10 years, 7 months ago
(2014-08-05 21:59:30 UTC)
#9
Message was sent while issue was closed.
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
I can confirm that the time to run the tests in the go.tools repo have ...
10 years, 7 months ago
(2014-08-05 22:12:39 UTC)
#10
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/
Issue 119360043: code review 119360043: runtime: convert hash functions to Go calling convention.
(Closed)
Created 10 years, 7 months ago by khr
Modified 10 years, 7 months ago
Reviewers: gobot, adonovan, rsc
Base URL:
Comments: 2