gmane.comp.lang.go.devel

code review 6243059: runtime: optimize the symbol table (issue 6243059)

2012-05-26T11:35:33

Reviewers: rsc,

Message:
Hello rsc< at >golang.org (cc: golang-dev< at >googlegroups.com),

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


Description:
runtime: optimize the symbol table

Please review this at http://codereview.appspot.com/6243059/

Affected files:
   M src/pkg/runtime/symtab.c


Index: src/pkg/runtime/symtab.c
===================================================================
--- a/src/pkg/runtime/symtab.c
+++ b/src/pkg/runtime/symtab.c
< at >< at > -16,6 +16,7 < at >< at >
  #include "defs_GOOS_GOARCH.h"
  #include "os_GOOS.h"
  #include "arch_GOARCH.h"
+#include "malloc.h"

  extern byte pclntab[], epclntab[], symtab[], esymtab[];

< at >< at > -28,6 +29,11 < at >< at >
  //byte *gotype;
  };

+// A dynamically allocated string containing multiple substrings.
+// Individual strings are slices of hugestring.
+static String hugestring;
+static int32 hugestring_len;
+
  // Walk over symtab, calling fn(&s) for each symbol.
  static void
  walksymtab(void (*fn)(Sym*))
< at >< at > -135,14 +141,15 < at >< at >

  // put together the path name for a z entry.
  // the f entries have been accumulated into fname already.
-static void
+// returns the length of the path name.
+static int32
  makepath(byte *buf, int32 nbuf, byte *path)
  {
  int32 n, len;
  byte *p, *ep, *q;

  if(nbuf <= 0)
-return;
+return 0;

  p = buf;
  ep = buf + nbuf;
< at >< at > -163,6 +170,7 < at >< at >
  runtime·memmove(p, q, len+1);
  p += len;
  }
+return p - buf;
  }

  // walk symtab accumulating path names for use by pc/ln table.
< at >< at > -181,47 +189,75 < at >< at >
  static int32 incstart;
  static int32 nfunc, nfile, nhist;
  Func *f;
-int32 i;
+int32 i, l;
+bool writestr;
+
+writestr = (hugestring.str != nil);

  switch(sym->symtype) {
  case 't':
  case 'T':
-if(runtime·strcmp(sym->name, (byte*)"etext") == 0)
-break;
-f = &func[nfunc++];
-// find source file
-for(i = 0; i < nfile - 1; i++) {
-if (files[i+1].aline > f->ln0)
+if(writestr) {
+if(runtime·strcmp(sym->name, (byte*)"etext") == 0)
  break;
+f = &func[nfunc++];
+// find source file
+for(i = 0; i < nfile - 1; i++) {
+if (files[i+1].aline > f->ln0)
+break;
+}
+f->src = files[i].srcstring;
+f->ln0 -= files[i].delta;
  }
-f->src = files[i].srcstring;
-f->ln0 -= files[i].delta;
  break;
  case 'z':
  if(sym->value == 1) {
  // entry for main source file for a new object.
-makepath(srcbuf, sizeof srcbuf, sym->name+1);
+l = makepath(srcbuf, sizeof srcbuf, sym->name+1);
  nhist = 0;
  nfile = 0;
  if(nfile == nelem(files))
  return;
-files[nfile].srcstring = runtime·gostring(srcbuf);
+
+if(!writestr) {
+hugestring_len += l;
+} else {
+if(l > 0) {
+files[nfile].srcstring.str = hugestring.str + hugestring.len;
+files[nfile].srcstring.len = l;
+} else {
+files[nfile].srcstring = runtime·emptystring;
+}
+runtime·memmove(hugestring.str+hugestring.len, srcbuf, l);
+hugestring.len += l;
+}
  files[nfile].aline = 0;
  files[nfile++].delta = 0;
  } else {
  // push or pop of included file.
-makepath(srcbuf, sizeof srcbuf, sym->name+1);
+l = makepath(srcbuf, sizeof srcbuf, sym->name+1);
  if(srcbuf[0] != '\0') {
  if(nhist++ == 0)
  incstart = sym->value;
  if(nhist == 0 && nfile < nelem(files)) {
  // new top-level file
-files[nfile].srcstring = runtime·gostring(srcbuf);
+if(!writestr) {
+hugestring_len += runtime·findnull(srcbuf);
+} else {
+if(l > 0) {
+files[nfile].srcstring.str = hugestring.str + hugestring.len;
+files[nfile].srcstring.len = l;
+} else {
+files[nfile].srcstring = runtime·emptystring;
+}
+runtime·memmove(hugestring.str+hugestring.len, srcbuf, l);
+hugestring.len += l;
+}
  files[nfile].aline = sym->value;
  // this is "line 0"
  files[nfile++].delta = sym->value - 1;
  }
-}else{
+} else {
  if(--nhist == 0)
  files[nfile-1].delta += sym->value - incstart;
  }
< at >< at > -408,10 +444,12 < at >< at >
  nfname = 0;
  walksymtab(dofunc);

-// initialize tables
-func = runtime·mal((nfunc+1)*sizeof func[0]);
+// Initialize tables.
+// Can use FlagNoPointers - all pointers either point into sections of  
the executable
+// or point to hugestring.
+func = runtime·mallocgc((nfunc+1)*sizeof func[0], FlagNoPointers, 0, 1);
  func[nfunc].entry = (uint64)etext;
-fname = runtime·mal(nfname*sizeof fname[0]);
+fname = runtime·mallocgc(nfname*sizeof fname[0], FlagNoPointers, 0, 1);
  nfunc = 0;
  walksymtab(dofunc);

< at >< at > -419,7 +457,13 < at >< at >
  splitpcln();

  // record src file and line info for each func
-walksymtab(dosrcline);
+walksymtab(dosrcline);  // pass 1: determine hugestring_len
+hugestring.str = runtime·mallocgc(hugestring_len, FlagNoPointers, 0, 0);
+hugestring.len = 0;
+walksymtab(dosrcline);  // pass 2: fill and use hugestring
+
+if(hugestring.len != hugestring_len)
+runtime·throw("buildfunc: problem in initialization procedure");

  m->nomemprof--;
  }

code review 6257059: path/filepath: Implement documented SkipDir behavior (issue 6257059)

2012-05-26T11:25:20

Reviewers: rsc,

Message:
Hello rsc-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org (cc: golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

I'd like you to review this change to
https://code.google.com/p/go


Description:
path/filepath: Implement documented SkipDir behavior

Currently walk() doesn't check for err == SkipDir when iterating
a directory list, but such promise is made in the docs for WalkFunc.

Fixes issue 3486.

Please review this at http://codereview.appspot.com/6257059/

Affected files:
   M src/pkg/path/filepath/path.go


Index: src/pkg/path/filepath/path.go
===================================================================
--- a/src/pkg/path/filepath/path.go
+++ b/src/pkg/path/filepath/path.go
< at >< at > -320,7 +320,7 < at >< at >
  }

  for _, fileInfo := range list {
-if err = walk(Join(path, fileInfo.Name()), fileInfo, walkFn); err != nil  
{
+if err = walk(Join(path, fileInfo.Name()), fileInfo, walkFn); err != nil  
&& err != SkipDir {
  return err
  }
  }

code review 6242061: runtime: update field types in preparation for GC changes (issue 6242061)

2012-05-26T08:30:30

Reviewers: rsc,

Message:
Hello rsc-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org (cc: golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

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


Description:
runtime: update field types in preparation for GC changes

Please review this at http://codereview.appspot.com/6242061/

Affected files:
   M src/pkg/runtime/cgocall.c
   M src/pkg/runtime/mgc0.c
   M src/pkg/runtime/mprof.goc
   M src/pkg/runtime/proc.c
   M src/pkg/runtime/runtime.h
   M src/pkg/runtime/thread_linux.c
   M src/pkg/runtime/traceback_x86.c

code review 6258050: cmd/cgo: give error if imported "C" but not used (issue 6258050)

2012-05-25T17:55:04

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org (cc: golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

I'd like you to review this change to
https://code.google.com/p/go/


Description:
cmd/cgo: give error if imported "C" but not used
     runtime/cgo is a special case, so it uses a workaround.

Please review this at http://codereview.appspot.com/6258050/

Affected files:
   M src/cmd/cgo/ast.go
   M src/pkg/runtime/cgo/cgo.go


Index: src/cmd/cgo/ast.go
===================================================================
--- a/src/cmd/cgo/ast.go
+++ b/src/cmd/cgo/ast.go
< at >< at > -58,6 +58,7 < at >< at >

  // In ast1, find the import "C" line and get any extra C preamble.
  sawC := false
+var importCPos token.Pos
  for _, decl := range ast1.Decls {
  d, ok := decl.(*ast.GenDecl)
  if !ok {
< at >< at > -69,6 +70,7 < at >< at >
  continue
  }
  sawC = true
+importCPos = s.Path.Pos()
  if s.Name != nil {
  error_(s.Path.Pos(), `cannot rename import "C"`)
  }
< at >< at > -127,6 +129,10 < at >< at >
  f.walk(ast1, "prog", (*File).saveExport)
  f.walk(ast2, "prog", (*File).saveExport2)

+if len(f.Ref) == 0 && len(f.ExpFunc) == 0 {
+error_(importCPos, `imported and not used: "C"`)
+}
+
  f.Comments = ast1.Comments
  f.AST = ast2
  }
Index: src/pkg/runtime/cgo/cgo.go
===================================================================
--- a/src/pkg/runtime/cgo/cgo.go
+++ b/src/pkg/runtime/cgo/cgo.go
< at >< at > -21,9 +21,10 < at >< at >
  */
  import "C"

+type _ C.uint // use C to avoid cgo "imported but not used" error
+
  // Supports _cgo_panic by converting a string constant to an empty
  // interface.
-
  func cgoStringToEface(s string, ret *interface{}) {
  *ret = s
  }

wanna atomic func..

まにらふ — 2012-05-25T12:42:55

4 exmple

a:=int{1,2,3,4,5}
b:=int{3,2,4}

for _,i:=range a|b {
  fmt.printf(%d)            // stdout is 1 2 3 4 5
}

for _,i:=range a&b{
  fmt.printf(%d)            // stdout is 2 3 4
}

if its logical array function, very useful..

wanna atomic func..

まにらふ — 2012-05-25T12:45:47

4 exmple

a:=[]int{1,2,3,4,5}
b:=[]int{3,2,4}

for _,i:=range a|b {
  fmt.printf(%d)            // stdout is 1 2 3 4 5
}

for _,i:=range a&b{
  fmt.printf(%d)            // stdout is 2 3 4
}

if its logical array function, very useful..

Showstoppers with cross-compilation to TI C6x architecture

Timon ter Braak — 2012-05-25T09:53:38

I have a cross-compilation toolchain (derived from codesourcery) towards 
the C6x architecture from TI ( a HOST to C6x-uclibc chain).
The toolchain is able to produce C,C++ compilers.

I started this effort to attempt to run Go on this multi-core DSP.
I know that C6x is a specialized architecture and freshly supported. But 
I would like to use Go on it for it's robustness and the convenience of 
Go operating in a heterogeneous platform.

Starting with GCC 4.7, both Go and C6x are supported. However, the 
combination of these two is not. I will outline my findings, and would 
like some feedback whether this combo is a nogo ;) or that I should 
invest some porting effort to do so.

Currently, Go builds upon two pillars that are currently prohibiting the 
build:

* libffi:
   the C6x architecture is not available within libffi
   the runtime can be build without libffi support, at the cost of 
broken reflection support; we could port libffi to the C6x architecture 
to fix this. So, for an initial attempt this is not a real issue.

* segmented stacks / context functions
   the gold linker in binutils only supports a limited set of target 
architectures. Linking with LD instead of gold disables the segmented 
stack support. Alternatively, Go then uses the 
getcontext/setcontext/makecontext functions, but these are not available 
in uClibc, and not implemented for C6x in Glibc.

What path would be advised to get the first bits of Go running? I assume 
that implementing the context functions for C6x would be the best option?

Timon

code review 6248049: cmd/6g, cmd/8g: move panicindex calls out of line (issue 6248049)

2012-05-25T06:44:59

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org,

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


Description:
cmd/6g, cmd/8g: move panicindex calls out of line

The old code generated for a bounds check was
                 CMP
                 JLT ok
                 CALL panicindex
         ok:
                 ...

The new code is (once the linker finishes with it):
                 CMP
                 JGE panic
                 ...
         panic:
                 CALL panicindex

which moves the calls out of line, putting more useful
code in each cache line.  This matters especially in tight
loops, such as in Fannkuch.  The benefit is more modest
elsewhere, but real.

 From test/bench/go1, amd64:

benchmark                old ns/op    new ns/op    delta
BenchmarkBinaryTree17   6096092000   6088808000   -0.12%
BenchmarkFannkuch11     6151404000   4020463000  -34.64%
BenchmarkGobDecode        28990050     28894630   -0.33%
BenchmarkGobEncode        12406310     12136730   -2.17%
BenchmarkGzip               179923       179903   -0.01%
BenchmarkGunzip              11219        11130   -0.79%
BenchmarkJSONEncode       86429350     86515900   +0.10%
BenchmarkJSONDecode      334593800    315728400   -5.64%
BenchmarkRevcomp25M     1219763000   1180767000   -3.20%
BenchmarkTemplate        492947600    483646800   -1.89%

And 386:

benchmark                old ns/op    new ns/op    delta
BenchmarkBinaryTree17   6354902000   6243000000   -1.76%
BenchmarkFannkuch11     8043769000   7326965000   -8.91%
BenchmarkGobDecode        19010800     18941230   -0.37%
BenchmarkGobEncode        14077500     13792460   -2.02%
BenchmarkGzip               194087       193619   -0.24%
BenchmarkGunzip              12495        12457   -0.30%
BenchmarkJSONEncode      125636400    125451400   -0.15%
BenchmarkJSONDecode      696648600    685032800   -1.67%
BenchmarkRevcomp25M     2058088000   2052545000   -0.27%
BenchmarkTemplate        602140000    589876800   -2.04%

To implement this, two new instruction forms:

         JLT target      // same as always
         JLT $0, target  // branch expected not taken
         JLT $1, target  // branch expected taken

The linker could also emit the prediction prefixes, but it
does not: expected taken branches are reversed so that the
expected case is not taken (as in example above), and
the default expectaton for such a jump is not taken
already.

Please review this at http://codereview.appspot.com/6248049/

Affected files:
   M src/cmd/6a/a.y
   M src/cmd/6a/y.tab.c
   M src/cmd/6a/y.tab.h
   M src/cmd/6g/cgen.c
   M src/cmd/6g/gg.h
   M src/cmd/6g/ggen.c
   M src/cmd/6g/gsubr.c
   M src/cmd/6l/optab.c
   M src/cmd/6l/pass.c
   M src/cmd/8a/a.y
   M src/cmd/8a/y.tab.c
   M src/cmd/8g/cgen.c
   M src/cmd/8g/gg.h
   M src/cmd/8g/ggen.c
   M src/cmd/8g/gsubr.c
   M src/cmd/8l/optab.c
   M src/cmd/8l/pass.c

code review 6248048: net/rpc: replace else if with an else (issue 6248048)

2012-05-25T05:03:30

Reviewers: r,

Message:
Hello r< at >golang.org (cc: golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

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


Description:
net/rpc: replace else if with an else

CL 5956051 used an else if condition, which is unnecessary.
If the top condition didn't hit, then we already know that
response.Error == "".

Please review this at http://codereview.appspot.com/6248048/

Affected files:
   M src/pkg/net/rpc/client.go


Index: src/pkg/net/rpc/client.go
===================================================================
--- a/src/pkg/net/rpc/client.go
+++ b/src/pkg/net/rpc/client.go
< at >< at > -127,7 +127,7 < at >< at >
  if err != nil {
  err = errors.New("reading error body: " + err.Error())
  }
-} else if response.Error == "" {
+} else {
  err = client.codec.ReadResponseBody(call.Reply)
  if err != nil {
  call.Error = errors.New("reading body " + err.Error())

ab is uh, weird

Brad Fitzpatrick — 2012-05-25T04:26:09

Playing with ab against Go's http server, I wasted some time debugging some
failures like this (on OS X 10.7):

$ ab -c 5 -n 100 -v 0 http://localhost:8080/hello
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)...apr_socket_recv: Connection reset by
peer (54)


Look what ab is sending, though!

In one TCP connection (in one packet, even) it's sending multiple pipelined
(!!) HTTP/1.0 (!!) requests, without Connection: keep-alive, even. (true, I
didn't specify -k to ab)

So Go's doing the right thing and closing the connection after the first
one, but still.... this is stupid.  People use this piece of crap software
to benchmark webservers and compare webservers against each other?

Or maybe the ab version that comes with OS X just sucks?

*sigh*

code review 6259043: crypto: amd64 assembly for hashes (issue 6259043)

2012-05-25T04:15:58

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org,

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


Description:
crypto: amd64 assembly for hashes

Tuning the Go versions was a good source of things to fix
in the compiler, but ultimately the assembly is going to win.
Can't beat 'em, so join 'em.

The good news is that the compiled code size goes down.

benchmark                  old MB/s     new MB/s  speedup
md5.BenchmarkHash8K          359.22       599.04    1.67x
sha1.BenchmarkHash8K          87.31       405.65    4.65x
sha256.BenchmarkHash8K        51.87       185.23    3.57x
sha512.BenchmarkHash8K        79.28       288.15    3.63x

Please review this at http://codereview.appspot.com/6259043/

Affected files:
   A src/pkg/crypto/gen.sh
   A src/pkg/crypto/md5/asm.go
   A src/pkg/crypto/md5/block_amd64.s
   M src/pkg/crypto/md5/gen.go
   M src/pkg/crypto/md5/md5.go
   M src/pkg/crypto/md5/md5block.go
   A src/pkg/crypto/sha1/asm.go
   A src/pkg/crypto/sha1/block_amd64.s
   M src/pkg/crypto/sha1/sha1.go
   M src/pkg/crypto/sha1/sha1_test.go
   M src/pkg/crypto/sha1/sha1block.go
   A src/pkg/crypto/sha256/asm.go
   A src/pkg/crypto/sha256/block_amd64.s
   M src/pkg/crypto/sha256/sha256.go
   M src/pkg/crypto/sha256/sha256_test.go
   M src/pkg/crypto/sha256/sha256block.go
   A src/pkg/crypto/sha512/asm.go
   A src/pkg/crypto/sha512/block_amd64.s
   M src/pkg/crypto/sha512/sha512.go
   M src/pkg/crypto/sha512/sha512_test.go
   M src/pkg/crypto/sha512/sha512block.go

code review 6258049: cmd/8c: better fix for 64-bit register smash (issue 6258049)

2012-05-25T03:36:24

Reviewers: ken2,

Message:
Hello ken2 (cc: golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

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


Description:
cmd/8c: better fix for 64-bit register smash

Ken pointed out that CL 5998043 was ugly code.
This should be better.

Fixes issue 3501.

Please review this at http://codereview.appspot.com/6258049/

Affected files:
   M src/cmd/8c/cgen.c
   M src/cmd/8c/cgen64.c


Index: src/cmd/8c/cgen.c
===================================================================
--- a/src/cmd/8c/cgen.c
+++ b/src/cmd/8c/cgen.c
< at >< at > -1703,6 +1703,7 < at >< at >
  }
  }

+v = w == 8;
  if(n->complex >= FNX && nn != nil && nn->complex >= FNX) {
  t = nn->type;
  nn->type = types[TLONG];
< at >< at > -1728,8 +1729,28 < at >< at >
  }

  x = 0;
-v = w == 8;
  if(v) {
+if(nn != nil && nn->complex >= FNX) {
+t = nn->type;
+nn->type = types[TLONG];
+regialloc(&nod2, nn, Z);
+lcgen(nn, &nod2);
+nn->type = t;
+
+nod2.type = typ(TIND, t);
+
+nod1 = nod2;
+nod1.op = OIND;
+nod1.left = &nod2;
+nod1.right = Z;
+nod1.complex = 1;
+nod1.type = t;
+
+sugen(n, &nod1, w);
+regfree(&nod2);
+return;
+}
+
  c = cursafe;
  if(n->left != Z && n->left->complex >= FNX
  && n->right != Z && n->right->complex >= FNX) {
Index: src/cmd/8c/cgen64.c
===================================================================
--- a/src/cmd/8c/cgen64.c
+++ b/src/cmd/8c/cgen64.c
< at >< at > -1601,33 +1601,6 < at >< at >
  prtree(n, "cgen64");
  print("AX = %d\n", reg[D_AX]);
  }
-
-if(nn != Z && nn->complex >= FNX) {
-// Evaluate nn address to register
-// before we use registers for n.
-// Otherwise the call during computation of nn
-// will smash the registers.  See
-// http://golang.org/issue/3501.
-
-// If both n and nn want calls, refuse to compile.
-if(n != Z && n->complex >= FNX)
-diag(n, "cgen64 miscompile");
-
-reglcgen(&nod1, nn, Z);
-m = cgen64(n, &nod1);
-regfree(&nod1);
-
-if(m == 0) {
-// Now what?  We computed &nn, which involved a
-// function call, and didn't use it.  The caller will recompute nn,
-// calling the function a second time.
-// We can figure out what to do later, if this actually happens.
-diag(n, "cgen64 miscompile");
-}
-
-return m;
-}
-
  cmp = 0;
  sh = 0;

code review 6242056: image/png: optimize the paeth filter implementation. (issue 6242056)

2012-05-25T03:09:25

Reviewers: rsc,

Message:
Hello rsc-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org (cc: golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

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


Description:
image/png: optimize the paeth filter implementation.

image/png benchmarks:
benchmark                       old ns/op    new ns/op    delta
BenchmarkPaeth                         10            7  -29.21%
BenchmarkDecodeGray               2381745      2241620   -5.88%
BenchmarkDecodeNRGBAGradient      9535555      8835100   -7.35%
BenchmarkDecodeNRGBAOpaque        8189590      7611865   -7.05%
BenchmarkDecodePaletted           1300688      1301940   +0.10%
BenchmarkDecodeRGB                6760146      6317082   -6.55%
BenchmarkEncodePaletted           6048596      6122666   +1.22%
BenchmarkEncodeRGBOpaque         18891140     19474230   +3.09%
BenchmarkEncodeRGBA              78945350     78552600   -0.50%

Wall time for Denis Cheremisov's PNG-decoding program given in
https://groups.google.com/group/golang-nuts/browse_thread/thread/22aa8a05040fdd49
Before: 2.25s
After:  2.27s
Delta:  +1%

The same program, but with a different PNG input file
(http://upload.wikimedia.org/wikipedia/commons/4/47/PNG_transparency_demonstration_1.png)
and only 100 iterations instead of 1000
Before: 4.78s
After:  4.42s
Delta:  -8%

Please review this at http://codereview.appspot.com/6242056/

Affected files:
   A src/pkg/image/png/paeth_test.go
   M src/pkg/image/png/reader.go


Index: src/pkg/image/png/paeth_test.go
===================================================================
new file mode 100644
--- /dev/null
+++ b/src/pkg/image/png/paeth_test.go
< at >< at > -0,0 +1,51 < at >< at >
+// Copyright 2012 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package png
+
+import (
+"testing"
+)
+
+func abs(x int) int {
+if x < 0 {
+return -x
+}
+return x
+}
+
+// slowPaeth is a slow but simple implementation of the Paeth function.
+// It is a straight port of the sample code in the PNG spec, section 9.4.
+func slowPaeth(a, b, c uint8) uint8 {
+p := int(a) + int(b) - int(c)
+pa := abs(p - int(a))
+pb := abs(p - int(b))
+pc := abs(p - int(c))
+if pa <= pb && pa <= pc {
+return a
+} else if pb <= pc {
+return b
+}
+return c
+}
+
+func TestPaeth(t *testing.T) {
+for a := 0; a < 256; a += 15 {
+for b := 0; b < 256; b += 15 {
+for c := 0; c < 256; c += 15 {
+got := paeth(uint8(a), uint8(b), uint8(c))
+want := slowPaeth(uint8(a), uint8(b), uint8(c))
+if got != want {
+t.Errorf("a, b, c = %d, %d, %d: got %d, want %d", a, b, c, got, want)
+}
+}
+}
+}
+}
+
+func BenchmarkPaeth(b *testing.B) {
+for i := 0; i < b.N; i++ {
+paeth(uint8(i>>16), uint8(i>>8), uint8(i))
+}
+}
Index: src/pkg/image/png/reader.go
===================================================================
--- a/src/pkg/image/png/reader.go
+++ b/src/pkg/image/png/reader.go
< at >< at > -98,13 +98,6 < at >< at >

  func (e UnsupportedError) Error() string { return "png: unsupported  
feature: " + string(e) }

-func abs(x int) int {
-if x < 0 {
-return -x
-}
-return x
-}
-
  func min(a, b int) int {
  if a < b {
  return a
< at >< at > -241,12 +234,28 < at >< at >
  return d.verifyChecksum()
  }

-// The Paeth filter function, as per the PNG specification.
+// paeth implements the Paeth filter function, as per the PNG  
specification.
  func paeth(a, b, c uint8) uint8 {
-p := int(a) + int(b) - int(c)
-pa := abs(p - int(a))
-pb := abs(p - int(b))
-pc := abs(p - int(c))
+// This is an optimized version of the sample code in the PNG spec.
+// For example, the sample code starts with:
+//p := int(a) + int(b) - int(c)
+//pa := abs(p - int(a))
+// but the optimized form uses fewer arithmetic operations:
+//pa := int(b) - int(c)
+//pa = abs(pa)
+pc := int(c)
+pa := int(b) - pc
+pb := int(a) - pc
+pc = pa + pb
+if pa < 0 {
+pa = -pa
+}
+if pb < 0 {
+pb = -pb
+}
+if pc < 0 {
+pc = -pc
+}
  if pa <= pb && pa <= pc {
  return a
  } else if pb <= pc {

code review 6252048: cmd/gc: fix parallel assignment in range (issue 6252048)

2012-05-25T03:05:34

Reviewers: ken2,

Message:
Hello ken2 (cc: golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

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


Description:
cmd/gc: fix parallel assignment in range

for expr1, expr2 = range slice
was assigning to expr1 and expr2 in sequence
instead of in parallel.  Now it assigns in parallel,
as it should.  This matters for things like
for i, x[i] = range slice.

Fixes issue 3464.

Please review this at http://codereview.appspot.com/6252048/

Affected files:
   M src/cmd/gc/range.c
   M src/cmd/gc/subr.c
   M test/range.go


Index: src/cmd/gc/range.c
===================================================================
--- a/src/cmd/gc/range.c
+++ b/src/cmd/gc/range.c
< at >< at > -152,9 +152,14 < at >< at >
  n->ntest = nod(OLT, hv1, hn);
  n->nincr = nod(OASOP, hv1, nodintconst(1));
  n->nincr->etype = OADD;
-body = list1(nod(OAS, v1, hv1));
-if(v2) {
-body = list(body, nod(OAS, v2, nod(OIND, hp, N)));
+if(v2 == N)
+body = list1(nod(OAS, v1, hv1));
+else {
+a = nod(OAS2, N, N);
+a->list = list(list1(v1), v2);
+a->rlist = list(list1(hv1), nod(OIND, hp, N));
+body = list1(a);
+
  tmp = nod(OADD, hp, nodintconst(t->type->width));
  tmp->type = hp->type;
  tmp->typecheck = 1;
Index: src/cmd/gc/subr.c
===================================================================
--- a/src/cmd/gc/subr.c
+++ b/src/cmd/gc/subr.c
< at >< at > -1950,6 +1950,12 < at >< at >
  if(n == N)
  return N;

+if(n->ninit) {
+walkstmtlist(n->ninit);
+*init = concat(*init, n->ninit);
+n->ninit = nil;
+}
+
  switch(n->op) {
  case ONAME:
  case OLITERAL:
Index: test/range.go
===================================================================
--- a/test/range.go
+++ b/test/range.go
< at >< at > -58,6 +58,17 < at >< at >
  println("wrong sum ranging over makeslice")
  panic("fail")
  }
+
+x := []int{10, 20}
+y := []int{99}
+i := 1
+for i, x[i] = range y {
+break
+}
+if i != 0 || x[0] != 10 || x[1] != 99 {
+println("wrong parallel assignment", i, x[0], x[1])
+panic("fail")
+}
  }

  func testslice1() {

code review 6250056: misc/dist: use archive/tar.FileInfoHeader (issue 6250056)

2012-05-25T00:17:30

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org,

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


Description:
misc/dist: use archive/tar.FileInfoHeader

Fixes issue 3299

Please review this at http://codereview.appspot.com/6250056/

Affected files:
   M misc/dist/bindist.go
   R misc/dist/stat_darwin.go
   R misc/dist/stat_linux.go

code review 6257050: C: Bill Thiede (Google CLA) (issue 6257050)

2012-05-24T23:15:41

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org,

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


Description:
C: Bill Thiede (Google CLA)

Please review this at http://codereview.appspot.com/6257050/

Affected files:
   M CONTRIBUTORS


Index: CONTRIBUTORS
===================================================================
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
< at >< at > -68,6 +68,7 < at >< at >
  Benny Siegert <bsiegert-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org>
  Berengar Lehr <Berengar.Lehr-Mmb7MZpHnFY< at >public.gmane.org>
  Bill Neubauer <wcn-iFWiy5xATs8dnm+yROfE0A< at >public.gmane.org> <wcn-hpIqsD4AKlfQT0dZR+AlfA< at >public.gmane.org>
+Bill Thiede <couchmoney-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org>
  Bjorn Tillenius <bjorn-dSjf3tBqmUzwXD0AkxfrAg< at >public.gmane.org>
  Bjorn Tipling <bjorn.tipling-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org>
  Blake Mizerany <blake.mizerany-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org>

code review 6244051: syscall: regenerate z-files for FreeBSD 9.0-STABLE (issue 6244051)

2012-05-24T22:54:48

Reviewers: mikioh, dho,

Message:
Hello mikioh.mikioh-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org, devon.odell-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org (cc:
golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org),

I'd like you to review this change to
https://code.google.com/p/go


Description:
syscall: regenerate z-files for FreeBSD 9.0-STABLE

There are changes in archive/tar and os due to the changes in the
struct syscall.Stat_t.

Please review this at http://codereview.appspot.com/6244051/

Affected files:
   M src/pkg/archive/tar/stat_atim.go
   M src/pkg/archive/tar/stat_atimespec.go
   M src/pkg/os/stat_freebsd.go
   M src/pkg/syscall/mkerrors.sh
   M src/pkg/syscall/zerrors_freebsd_386.go
   M src/pkg/syscall/zerrors_freebsd_amd64.go
   M src/pkg/syscall/ztypes_freebsd_386.go
   M src/pkg/syscall/ztypes_freebsd_amd64.go

code review 6252047: strconv: better documentation for FormatInt, FormatUint. (issue 6252047)

2012-05-24T22:54:03

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org,

I'd like you to review this change to
https://code.google.com/p/go


Description:
strconv: better documentation for FormatInt, FormatUint.

Fixes issue 3580.

Please review this at http://codereview.appspot.com/6252047/

Affected files:
   M src/pkg/strconv/itoa.go


Index: src/pkg/strconv/itoa.go
===================================================================
--- a/src/pkg/strconv/itoa.go
+++ b/src/pkg/strconv/itoa.go
< at >< at > -4,13 +4,17 < at >< at >

  package strconv

-// FormatUint returns the string representation of i in the given base.
+// FormatUint returns the string representation of i in the given base,
+// for 2 <= base <= 36. The result uses the lowercase letters 'a' to 'z'
+// for digit values >= 10.
  func FormatUint(i uint64, base int) string {
  _, s := formatBits(nil, i, base, false, false)
  return s
  }

-// FormatInt returns the string representation of i in the given base.
+// FormatInt returns the string representation of i in the given base,
+// for 2 <= base <= 36. The result uses the lowercase letters 'a' to 'z'
+// for digit values >= 10.
  func FormatInt(i int64, base int) string {
  _, s := formatBits(nil, uint64(i), base, i < 0, false)
  return s

code review 6215078: runtime: handle and test large map values (issue 6215078)

2012-05-24T21:54:42

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev< at >googlegroups.com,

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


Description:
runtime: handle and test large map values

This is from CL 5451105 but was dropped from that CL.
See also CL 6137051.

The only change compared to 5451105 is to check for
h != nil in reflect·mapiterinit; allowing use of nil maps
must have happened after that original CL.

Fixes issue 3573.

Please review this at http://codereview.appspot.com/6215078/

Affected files:
   M src/pkg/runtime/hashmap.c
   M test/bigmap.go

code review 6249052: archive/tar: fix windows test failure (issue 6249052)

2012-05-24T21:31:19

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev-/JYPxA39Uh5TLH3MbocFFw< at >public.gmane.org,

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


Description:
archive/tar: fix windows test failure

Please review this at http://codereview.appspot.com/6249052/

Affected files:
   M src/pkg/archive/tar/tar_test.go


Index: src/pkg/archive/tar/tar_test.go
===================================================================
--- a/src/pkg/archive/tar/tar_test.go
+++ b/src/pkg/archive/tar/tar_test.go
< at >< at > -22,7 +22,7 < at >< at >
  if g, e := h.Name, "small.txt"; g != e {
  t.Errorf("Name = %q; want %q", g, e)
  }
-if g, e := h.Mode, int64(0644|c_ISREG); g != e {
+if g, e := h.Mode, int64(fi.Mode().Perm())|c_ISREG; g != e {
  t.Errorf("Mode = %#o; want %#o", g, e)
  }
  if g, e := h.Size, int64(5); g != e {

windows-amd64 broken by cmd/gc: faster code, mainly for rotate

2012-05-24T21:30:40

Change 67c0b8c8fb29 broke the windows-amd64 build:
http://build.golang.org/log/eb172565120eb83a2a4bd9ff73b056608657cf62

cmd/gc: faster code, mainly for rotate

* Eliminate bounds check on known small shifts.
* Rewrite x<<s | x>>(32-s) as a rotate (constant s).
* More aggressive (but still minimal) range analysis.

R=ken, dave, iant
CC=golang-dev
http://codereview.appspot.com/6209077

http://code.google.com/p/go/source/detail?r=67c0b8c8fb29

$ tail -200 < log
container/ring
crypto/sha256
crypto/sha512
database/sql/driver
database/sql
debug/gosym
encoding/ascii85
encoding/base32
encoding/csv
encoding/hex
exp/ebnf
exp/ebnflint
exp/types
exp/gotype
exp/html
exp/norm
exp/locale/collate
hash/fnv
exp/locale/collate/build
exp/proxy
exp/utf8string
exp/winfsnotify
hash/crc64
image/color
image
image/draw
image/gif
image/jpeg
image/png
log/syslog
math/cmplx
net/http/cgi
net/http/fcgi
net/http/httptest
net/http/httputil
net/mail
net/rpc
net/rpc/jsonrpc
net/smtp
old/netchan
os/signal
os/user
runtime/cgo
testing
testing/iotest
testing/quick

# Testing packages.
ok  cmd/api0.050s
?   cmd/cgo[no test files]
ok  cmd/fix1.498s
ok  cmd/go0.056s
?   cmd/godoc[no test files]
ok  cmd/gofmt0.078s
?   cmd/vet[no test files]
?   cmd/yacc[no test files]
--- FAIL: TestFileInfoHeader (0.00 seconds)
tar_test.go:26: Mode = 0100666; want 0100644
FAIL
FAILarchive/tar0.027s
ok  archive/zip0.069s
ok  bufio0.092s
ok  bytes0.085s
ok  compress/bzip20.083s
ok  compress/flate0.666s
ok  compress/gzip0.027s
ok  compress/lzw0.150s
ok  compress/zlib1.312s
ok  container/heap0.025s
ok  container/list0.027s
ok  container/ring0.030s
?   crypto[no test files]
ok  crypto/aes0.053s
ok  crypto/cipher0.074s
ok  crypto/des0.048s
ok  crypto/dsa0.038s
ok  crypto/ecdsa0.071s
ok  crypto/elliptic0.043s
ok  crypto/hmac0.025s
ok  crypto/md50.027s
ok  crypto/rand0.046s
ok  crypto/rc40.020s
ok  crypto/rsa0.150s
ok  crypto/sha10.027s
ok  crypto/sha2560.021s
ok  crypto/sha5120.026s
ok  crypto/subtle0.031s
ok  crypto/tls0.163s
ok  crypto/x5090.899s
?   crypto/x509/pkix[no test files]
ok  database/sql0.033s
ok  database/sql/driver0.025s
ok  debug/dwarf0.031s
ok  debug/elf0.033s
ok  debug/gosym0.027s
ok  debug/macho0.028s
ok  debug/pe0.027s
ok  encoding/ascii850.024s
ok  encoding/asn10.031s
ok  encoding/base320.026s
ok  encoding/base640.026s
ok  encoding/binary0.021s
ok  encoding/csv0.027s
ok  encoding/gob0.078s
ok  encoding/hex0.028s
ok  encoding/json0.082s
ok  encoding/pem0.030s
ok  encoding/xml0.043s
ok  errors0.022s
ok  exp/ebnf0.027s
ok  exp/ebnflint0.030s
ok  exp/gotype0.232s
ok  exp/html0.172s
ok  exp/locale/collate0.050s
ok  exp/locale/collate/build0.031s
ok  exp/norm1.424s
ok  exp/proxy0.029s
ok  exp/types0.036s
ok  exp/utf8string0.027s
ok  exp/winfsnotify0.077s
ok  expvar0.038s
ok  flag0.027s
ok  fmt0.078s
ok  go/ast0.031s
ok  go/build0.304s
ok  go/doc0.106s
ok  go/parser0.079s
ok  go/printer0.374s
ok  go/scanner0.029s
ok  go/token0.072s
?   hash[no test files]
ok  hash/adler320.024s
ok  hash/crc320.024s
ok  hash/crc640.024s
ok  hash/fnv0.026s
ok  html0.026s
ok  html/template0.081s
ok  image0.142s
ok  image/color0.036s
ok  image/draw0.062s
?   image/gif[no test files]
ok  image/jpeg0.116s
ok  image/png0.081s
ok  index/suffixarray0.034s
ok  io0.038s
ok  io/ioutil0.029s
ok  log0.027s
?   log/syslog[no test files]
ok  math0.030s
ok  math/big0.165s
ok  math/cmplx0.025s
ok  math/rand0.201s
ok  mime0.054s
ok  mime/multipart0.199s
ok  net0.939s
ok  net/http8.395s
ok  net/http/cgi0.083s
ok  net/http/fcgi0.039s
ok  net/http/httptest0.039s
ok  net/http/httputil0.050s
?   net/http/pprof[no test files]
ok  net/mail0.029s
ok  net/rpc0.104s
ok  net/rpc/jsonrpc0.047s
ok  net/smtp0.032s
ok  net/textproto0.023s
ok  net/url0.026s
ok  old/netchan0.041s
ok  os0.087s
ok  os/exec0.141s
ok  os/signal1.470s
ok  os/user22.564s
ok  path0.025s
ok  path/filepath0.040s
ok  reflect0.030s
ok  regexp0.162s
ok  regexp/syntax0.971s
ok  runtime1.486s
?   runtime/cgo[no test files]
ok  runtime/debug0.026s
ok  runtime/pprof0.396s
ok  sort0.119s
ok  strconv0.633s
ok  strings0.030s
ok  sync0.079s
ok  sync/atomic0.039s
?   syscall[no test files]
?   testing[no test files]
?   testing/iotest[no test files]
ok  testing/quick0.062s
ok  text/scanner0.028s
ok  text/tabwriter0.028s
ok  text/template0.052s
ok  text/template/parse0.030s
ok  time41.031s
ok  unicode0.026s
ok  unicode/utf160.026s
ok  unicode/utf80.027s
?   unsafe[no test files]