gmane.comp.lang.go.devel

Re: code review 6114046: runtime, reflect, ld, gc: garbage collection precision ... (issue 6114046)

2012-05-24T17:00:33

Let's make a separate CL that does nothing but add the (unused for now)
gc field to the reflect type information.  That touches a fair number of
files so it's good to split out.

Then let's also make a separate CL that is just the linker changes.

I'm pretty comfortable with both of those.  Then we'll have left the
compiler changes to generate the information and the garbage collector
changes to use it.

Thanks again.


http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode860
src/cmd/ld/data.c:860: gc_addsym(Sym *gc, Sym *s, int32 ofs)
Please s/ofs/off/ to match other code here.

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode881
src/cmd/ld/data.c:881: for(a=0; a<s->size; a++) {
Avoid the conditional and all the unnecessary loop iterations:

for(a = -ofs&(PtrSize-1); a+PtrSize<=s->size; a+=PtrSize) {
     adduintxx(gc, GC_APTR);
     adduintxx(gc, off+a);
}

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode883
src/cmd/ld/data.c:883: if(PtrSize == 4) {
Use adduintxx.

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode956
src/cmd/ld/data.c:956: /*
I'm worried about needing to maintain two copies of this code, which
changes more frequently than you might expect.

Instead of this, can we do

/*
  * Two passes: first pass creates gc information.
  * The addresses assigned during the first pass are incorrect
  * but harmless.
  */
for(pass=0; pass<2; pass++) {
    ... the original copy of the code,
    looking at pass only in the data and bss loops,
    to determine whether to update the gc info ...
}

Or, another way to do this would be to put the gc-computing code in the
original copy and then add a loop at the end that determines how far
each address moves down to make room for the gc data and does

for(s=aftergc; s != nil; s = s->next)
     s->value += delta;
for(sect=aftergcsect; sect != nil; sect = sect->next)
     sect->addr += delta;

or whatever the appropriate adjustments would be.

I'd be happy with whichever one you think is easiest.

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode964
src/cmd/ld/data.c:964: if(PtrSize == 4) {
can avoid the if.
adduintxx(data_gc, 0, PtrSize);
adduintxx(bss_gc, 0, PtrSize);

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode1012
src/cmd/ld/data.c:1012: if(PtrSize == 4) {
Same.

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode1135
src/cmd/ld/data.c:1135: if(PtrSize == 4)
Use setuintxx.

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode1152
src/cmd/ld/data.c:1152: if(PtrSize == 4)
Use setuintxx.

http://codereview.appspot.com/6114046/

Re: code review 6114046: runtime, reflect, ld, gc: garbage collection precision ... (issue 6114046)

2012-05-24T16:36:45

I made a first pass through for style-related things.  I know it's
annoying and avoids the point of the CL, but if we get these out of the
way early, it should make the iterations faster.  I certainly didn't
flag every variable declared mid-function, but please look through the
code for them.

I would also like to start cutting this up into smaller CLs that can be
reviewed and submitted, like we did for Dmitriy's garbage collector
changes.

One good first CL to separate out would be the change from byte* to
uintptr in the various M and G fields, along with the casts required to
keep the code compiling.  Please make that it's own CL, and then that
will reduce the number of files here significantly.

Another good CL to separate out would be the hugestring change, since
the idea of allocating one big string does not depend on having the new
garbage collector code.

I'll send more substantial comments in the next review.

Thanks.


http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode631
src/cmd/ld/data.c:631: setuintxx(Sym *s, vlong r, uint64 v, int wid)
Please change r to off (as in offset).

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode831
src/cmd/ld/data.c:831: align_symsize(int32 s)
s/_// to match the rest of the functions here

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode841
src/cmd/ld/data.c:841: align_datsize(int32 datsize, Sym *s)
s/_//

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode860
src/cmd/ld/data.c:860: gc_addsym(Sym *gc, Sym *s, int32 ofs)
s/_//

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode901
src/cmd/ld/data.c:901: Sym *data_gc, *bss_gc;
s/_//

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/lib.h
File src/cmd/ld/lib.h (right):

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/lib.h#newcode42
src/cmd/ld/lib.h:42: SDATA_GC,
Please call these SGCDATA and SGCBSS, which match the naming of things
like SRODATA and SNOPTRBSS.

This code tree tends to avoid _ in names, for what it's worth.

http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/lib.h#newcode325
src/cmd/ld/lib.h:325: // The type.* symbols. This has to be in sync
with
runtime/type.h already has a copy of these.  Let's not add another.
Maybe move the enum into runtime/typekind.h and include it from the ld
files that need this information.

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newcode90
src/pkg/runtime/mgc0.c:90: {
Please sync with dmitriy's latest code.

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newcode434
src/pkg/runtime/mgc0.c:434: if( ((uintptr)obj & ((uintptr)PtrSize-1)) !=
0 ) {
Avoid spaces inside if( ) parens.

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newcode517
src/pkg/runtime/mgc0.c:517: struct Y *ybufpos = ybuf;
Please move all variable declarations to top of function, and don't
initialize and declare in the same statement.

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newcode829
src/pkg/runtime/mgc0.c:829: Hmap *h = (Hmap*)b;
Please move variables to top, avoiding { } around case blocks.

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newcode1837
src/pkg/runtime/mgc0.c:1837: case 2:
unindent

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newcode2089
src/pkg/runtime/mgc0.c:2089: int32 i;
Move variables to top.

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/runtime.c
File src/pkg/runtime/runtime.c (right):

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/runtime.c#newcode114
src/pkg/runtime/runtime.c:114: //runtime·dopanic(0);
Remove this file from the CL?

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/sema.goc
File src/pkg/runtime/sema.goc (right):

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/sema.goc#newcode46
src/pkg/runtime/sema.goc:46: union semtable_t
s/_t//

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/slice.c
File src/pkg/runtime/slice.c (right):

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/slice.c#newcode45
src/pkg/runtime/slice.c:45: if(cap != 0) {
Instead of writing

if(xxx) {
     long long block
} else {
     short block
}

It is usually easier to follow to reverse the order, so that when as a
reader you see the else you still remember what the if condition was.
In this case we can get rid of the else entirely too:

if(cap == 0) {
     ret->len = 0;
     ret->cap = 0;
     ret->array = (byte*)&zerobase;
     return;
}

This is the same as what we suggest for Go programs in
http://golang.org/doc/effective_go.html#if

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (left):

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/symtab.c#oldcode208
src/pkg/runtime/symtab.c:208: files[nfile].srcstring =
runtime·gostring(srcbuf);
I'm a little worried about losing gostring, because I believe gostring
happened to NUL-terminate the strings so that you could use the pointer
as a C string too, but maybe that's not depended on anymore.

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/symtab.c#newcode223
src/pkg/runtime/symtab.c:223: if(!writestr) {
This block (the new code) should be its own function, since it appears
here and then 10 lines later again.

http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/symtab.c#newcode226
src/pkg/runtime/symtab.c:226: if(l > 0) {
Why make l > 0 a special case?  The code here would work fine for l == 0
too.  Same below.

http://codereview.appspot.com/6114046/

Re: code review 6203091: cmd/6g: peephole fixes/additions (issue 6203091)

2012-05-24T16:11:38

*** Submitted as
http://code.google.com/p/go/source/detail?r=3b320ce16e39 ***

cmd/6g: peephole fixes/additions

* Shift/rotate by constant doesn't have to stop subprop. (also in 8g)
* Remove redundant MOVLQZX instructions.
* An attempt at issuing loads early.
   Good for 0.5% on a good day, might not be worth keeping.
   Need to understand more about whether the x86
   looks ahead to what loads might be coming up.

R=ken2, ken
CC=golang-dev
http://codereview.appspot.com/6203091


http://codereview.appspot.com/6203091/

Re: code review 6249048: cmd/cc: fix uint right shift in constant propagation (issue 6249048)

2012-05-24T16:09:07

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

cmd/cc: fix uint right shift in constant evaluation
         Fixes issue 3664.

R=golang-dev, bradfitz, rsc
CC=golang-dev
http://codereview.appspot.com/6249048


http://codereview.appspot.com/6249048/

Re: code review 6249048: cmd/cc: fix uint right shift in constant propagation (issue 6249048)

minux — 2012-05-24T16:08:17


I wish we have tests for cc....

Re: code review 6249048: cmd/cc: fix uint right shift in constant propagation (issue 6249048)

Russ Cox — 2012-05-24T16:01:30

LGTM

Please change 'propagation' to 'evaluation' in the CL description.

Re: code review 6249048: cmd/cc: fix uint right shift in constant propagation (issue 6249048)

Brad Fitzpatrick — 2012-05-24T16:01:02

Test?  (question, not request.  how/where are tests for cc?)

On Thu, May 24, 2012 at 8:58 AM, <minux.ma-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:

code review 6249048: cmd/cc: fix uint right shift in constant propagation (issue 6249048)

2012-05-24T15:58:31

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/cc: fix uint right shift in constant propagation
         Fixes issue 3664.

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

Affected files:
   src/cmd/cc/scon.c


Index: src/cmd/cc/scon.c
===================================================================
--- a/src/cmd/cc/scon.c
+++ b/src/cmd/cc/scon.c
< at >< at > -175,7 +175,10 < at >< at >
  break;

  case OLSHR:
-v = (uvlong)l->vconst >> r->vconst;
+if(l->type->width != sizeof(uvlong))
+v = ((uvlong)l->vconst & 0xffffffffULL) >> r->vconst;
+else
+v = (uvlong)l->vconst >> r->vconst;
  break;

  case OASHR:

Re: code review 6218072: syscall: simplify text returned by Errno.Error() when F... (issue 6218072)

Russ Cox — 2012-05-24T15:43:22

I think 3376 should not be too hard.  The code that initializes an
os.File should call GetConsoleMode to determine whether the underlying
handle is in fact the console.  If so, the Write method should use
WriteConsoleW instead of the usual write.  It will need to save a
trailing partial UTF-8 sequence in a small buffer for adding to the
next write.

Russ

Re: Re: code review 6218072: syscall: simplify text returned by Errno.Error() when F... (issue 6218072)

Russ Cox — 2012-05-24T15:39:24

LGTM

Re: code review 6237045: math/big: make Rat.Denom() always return a reference (issue 6237045)

Russ Cox — 2012-05-24T15:34:16

LGTM

code review 6256045: exp/html: adjust parseForeignContent to match spec (issue 6256045)

2012-05-24T15:31:51

Reviewers: nigeltao,

Message:
Hello nigeltao-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:
exp/html: adjust parseForeignContent to match spec

Remove redundant checks for integration points.

Ignore null bytes in text.

Don't break out of foreign content for a <font> tag unless it
has a color, face, or size attribute.

Check for MathML text integration points when breaking out of
foreign content.

Pass two new tests.

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

Affected files:
   M src/pkg/exp/html/foreign.go
   M src/pkg/exp/html/parse.go
   M src/pkg/exp/html/testlogs/plain-text-unsafe.dat.log


Index: src/pkg/exp/html/foreign.go
===================================================================
--- a/src/pkg/exp/html/foreign.go
+++ b/src/pkg/exp/html/foreign.go
< at >< at > -82,7 +82,6 < at >< at >
  "dt":         true,
  "em":         true,
  "embed":      true,
-"font":       true,
  "h1":         true,
  "h2":         true,
  "h3":         true,
Index: src/pkg/exp/html/parse.go
===================================================================
--- a/src/pkg/exp/html/parse.go
+++ b/src/pkg/exp/html/parse.go
< at >< at > -1785,12 +1785,7 < at >< at >
  func parseForeignContent(p *parser) bool {
  switch p.tok.Type {
  case TextToken:
-// TODO: HTML integration points.
-if p.top().Namespace == "" {
-inBodyIM(p)
-p.resetInsertionMode()
-return true
-}
+p.tok.Data = strings.Replace(p.tok.Data, "\x00", "", -1)
  if p.framesetOK {
  p.framesetOK = strings.TrimLeft(p.tok.Data, whitespace) == ""
  }
< at >< at > -1801,15 +1796,21 < at >< at >
  Data: p.tok.Data,
  })
  case StartTagToken:
-if htmlIntegrationPoint(p.top()) {
-inBodyIM(p)
-p.resetInsertionMode()
-return true
+b := breakout[p.tok.Data]
+if p.tok.Data == "font" {
+loop:
+for _, attr := range p.tok.Attr {
+switch attr.Key {
+case "color", "face", "size":
+b = true
+break loop
+}
+}
  }
-if breakout[p.tok.Data] {
+if b {
  for i := len(p.oe) - 1; i >= 0; i-- {
-// TODO: MathML integration points.
-if p.oe[i].Namespace == "" || htmlIntegrationPoint(p.oe[i]) {
+n := p.oe[i]
+if n.Namespace == "" || htmlIntegrationPoint(n) ||  
mathMLTextIntegrationPoint(n) {
  p.oe = p.oe[:i+1]
  break
  }
< at >< at > -1833,6 +1834,10 < at >< at >
  namespace := p.top().Namespace
  p.addElement(p.tok.Data, p.tok.Attr)
  p.top().Namespace = namespace
+if p.hasSelfClosingToken {
+p.oe.pop()
+p.acknowledgeSelfClosingTag()
+}
  case EndTagToken:
  for i := len(p.oe) - 1; i >= 0; i-- {
  if p.oe[i].Namespace == "" {
< at >< at > -1856,6 +1861,7 < at >< at >
  return false
  }
  n := p.oe[len(p.oe)-1]
+
  if n.Namespace == "" {
  return false
  }
Index: src/pkg/exp/html/testlogs/plain-text-unsafe.dat.log
===================================================================
--- a/src/pkg/exp/html/testlogs/plain-text-unsafe.dat.log
+++ b/src/pkg/exp/html/testlogs/plain-text-unsafe.dat.log
< at >< at > -16,8 +16,8 < at >< at >
  FAIL "<svg>\x00<frameset>"
  FAIL "<svg>\x00 <frameset>"
  FAIL "<svg>\x00a<frameset>"
-FAIL "<svg>\x00</svg><frameset>"
-FAIL "<svg>\x00 </svg><frameset>"
+PASS "<svg>\x00</svg><frameset>"
+PASS "<svg>\x00 </svg><frameset>"
  FAIL "<svg>\x00a</svg><frameset>"
  PASS "<svg><path></path></svg><frameset>"
  PASS "<svg><p><frameset>"

Re: code review 6228047: godoc: correctly categorize interface methods, performa... (issue 6228047)

2012-05-24T15:28:51

LGTM

http://codereview.appspot.com/6228047/

Re: code review 6251044: hash/adler32: optimize. (issue 6251044)

2012-05-24T14:55:00

LGTM

Very nice.


http://codereview.appspot.com/6251044/

Re: code review 6223050: runtime: faster GC mark phase (issue 6223050)

Russ Cox — 2012-05-24T14:00:37

Thank you so much for seeing this through. I am more than happy to put
off concurrent sweep for now.

Russ

Re: code review 6245046: gotsan: add package (issue 6245046)

Maxim Pimenov — 2012-05-24T13:23:20

please ignore this, I'm trying to learn codereview.py the hard way

On Thu, May 24, 2012 at 5:22 PM, <mpimenov-hpIqsD4AKlfQT0dZR+AlfA< at >public.gmane.org> wrote:

code review 6245046: gotsan: add package (issue 6245046)

2012-05-24T13:22:42

Reviewers: golang-dev_googlegroups.com,

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

I'd like you to review this change to
https://mpimenov%40google.com-LGSXBDjVo1BibQn6LdNjmg< at >public.gmane.org/r/mpimenov-go1-tsan/


Description:
gotsan: add package
it should be added to rev. 12771,
version managing is not a priority now

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

Affected files:
   A src/pkg/gotsan/atomic_test.go
   A src/pkg/gotsan/chan_test.go
   A src/pkg/gotsan/gotsan.c
   A src/pkg/gotsan/gotsan.go
   A src/pkg/gotsan/gotsan.h
   A src/pkg/gotsan/mop_test.go
   A src/pkg/gotsan/race.go
   A src/pkg/gotsan/runtimemop_test.go
   A src/pkg/gotsan/select_test.go
   A src/pkg/gotsan/stress_test.go
   A src/pkg/gotsan/sync_test.go
   A src/pkg/gotsan/testtsan.go

Re: code review 6207044: syscall: add Termios support on freebsd (issue 6207044)

Francisco Souza — 2012-05-24T12:06:54

Makes sense. The O_CLOEXEC, Stat_t and other stuff have nothing with
Termios.

I'll split this CL in two new CLs and send them later.

Re: code review 6207044: syscall: add Termios support on freebsd (issue 6207044)

Mikio Hara — 2012-05-24T10:09:24

On Wed, May 23, 2012 at 1:26 PM, Francisco Souza
<franciscossouza-Re5JQEeQqe8AvxtiuMwx3w< at >public.gmane.org> wrote:


Looks like you are trying to do two things in one CL: update z-files
and add termios stuff into syscall package. So what about following
steps:
- re-generate z-files for FreeBSD 9-STABLE
- add termios to syscall package then

code review 6251044: hash/adler32: optimize. (issue 6251044)

2012-05-24T07:07:56

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:
hash/adler32: optimize.

The bulk of the gains come from hoisting the modulo ops outside of
the inner loop. The magic 5552 number was suggested in a previous
code review: http://codereview.appspot.com/4441073/

Reducing the digest type from 8 bytes to 4 bytes gains another 1% on
the hash/adler32 micro-benchmark.

Benchmarks for $GOOS,$GOARCH = linux,amd64 below.

hash/adler32 benchmark:
benchmark             old ns/op    new ns/op    delta
BenchmarkAdler32KB         1660         1364  -17.83%

image/png benchmark:
benchmark                       old ns/op    new ns/op    delta
BenchmarkDecodeGray               2466909      2425539   -1.68%
BenchmarkDecodeNRGBAGradient      9884500      9751705   -1.34%
BenchmarkDecodeNRGBAOpaque        8511615      8379800   -1.55%
BenchmarkDecodePaletted           1366683      1330677   -2.63%
BenchmarkDecodeRGB                6987496      6884974   -1.47%
BenchmarkEncodePaletted           6292408      6040052   -4.01%
BenchmarkEncodeRGBOpaque         19780680     19178440   -3.04%
BenchmarkEncodeRGBA              80738600     79076800   -2.06%

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

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

Affected files:
   M src/pkg/hash/adler32/adler32.go
   M src/pkg/hash/adler32/adler32_test.go


Index: src/pkg/hash/adler32/adler32.go
===================================================================
--- a/src/pkg/hash/adler32/adler32.go
+++ b/src/pkg/hash/adler32/adler32.go
< at >< at > -3,7 +3,8 < at >< at >
  // license that can be found in the LICENSE file.

  // Package adler32 implements the Adler-32 checksum.
-// Defined in RFC 1950:
+//
+// It is defined in RFC 1950:
  //Adler-32 is composed of two sums accumulated per byte: s1 is
  //the sum of all bytes, s2 is the sum of all s1 values. Both sums
  //are done modulo 65521. s1 is initialized to 1, s2 to zero.  The
< at >< at > -14,20 +15,21 < at >< at >
  import "hash"

  const (
+// mod is the largest prime that is less than 65536.
  mod = 65521
+// nmax is the largest n such that
+// 255 * n * (n+1) / 2 + (n+1) * (mod-1) <= 2^32-1.
+nmax = 5552
  )

  // The size of an Adler-32 checksum in bytes.
  const Size = 4

  // digest represents the partial evaluation of a checksum.
-type digest struct {
-// invariant: (a < mod && b < mod) || a <= b
-// invariant: a + b + 255 <= 0xffffffff
-a, b uint32
-}
+// The low 16 bits are s1, the high 16 bits are s2.
+type digest uint32

-func (d *digest) Reset() { d.a, d.b = 1, 0 }
+func (d *digest) Reset() { *d = 1 }

  // New returns a new hash.Hash32 computing the Adler-32 checksum.
  func New() hash.Hash32 {
< at >< at > -40,43 +42,36 < at >< at >

  func (d *digest) BlockSize() int { return 1 }

-// Add p to the running checksum a, b.
-func update(a, b uint32, p []byte) (aa, bb uint32) {
-for _, pi := range p {
-a += uint32(pi)
-b += a
-// invariant: a <= b
-if b > (0xffffffff-255)/2 {
-a %= mod
-b %= mod
-// invariant: a < mod && b < mod
-} else {
-// invariant: a + b + 255 <= 2 * b + 255 <= 0xffffffff
+// Add p to the running checksum d.
+func update(d digest, p []byte) digest {
+s1, s2 := uint32(d&0xffff), uint32(d>>16)
+for len(p) > 0 {
+var q []byte
+if len(p) > nmax {
+p, q = p[:nmax], p[nmax:]
  }
+for _, x := range p {
+s1 += uint32(x)
+s2 += s1
+}
+s1 %= mod
+s2 %= mod
+p = q
  }
-return a, b
-}
-
-// Return the 32-bit checksum corresponding to a, b.
-func finish(a, b uint32) uint32 {
-if b >= mod {
-a %= mod
-b %= mod
-}
-return b<<16 | a
+return digest(s2<<16 | s1)
  }

  func (d *digest) Write(p []byte) (nn int, err error) {
-d.a, d.b = update(d.a, d.b, p)
+*d = update(*d, p)
  return len(p), nil
  }

-func (d *digest) Sum32() uint32 { return finish(d.a, d.b) }
+func (d *digest) Sum32() uint32 { return uint32(*d) }

  func (d *digest) Sum(in []byte) []byte {
-s := d.Sum32()
+s := uint32(*d)
  return append(in, byte(s>>24), byte(s>>16), byte(s>>8), byte(s))
  }

  // Checksum returns the Adler-32 checksum of data.
-func Checksum(data []byte) uint32 { return finish(update(1, 0, data)) }
+func Checksum(data []byte) uint32 { return uint32(update(1, data)) }
Index: src/pkg/hash/adler32/adler32_test.go
===================================================================
--- a/src/pkg/hash/adler32/adler32_test.go
+++ b/src/pkg/hash/adler32/adler32_test.go
< at >< at > -6,6 +6,7 < at >< at >

  import (
  "io"
+"strings"
  "testing"
  )

< at >< at > -15,15 +16,15 < at >< at >
  }

  var golden = []_Adler32Test{
-{0x1, ""},
-{0x620062, "a"},
-{0x12600c4, "ab"},
-{0x24d0127, "abc"},
-{0x3d8018b, "abcd"},
-{0x5c801f0, "abcde"},
-{0x81e0256, "abcdef"},
-{0xadb02bd, "abcdefg"},
-{0xe000325, "abcdefgh"},
+{0x00000001, ""},
+{0x00620062, "a"},
+{0x012600c4, "ab"},
+{0x024d0127, "abc"},
+{0x03d8018b, "abcd"},
+{0x05c801f0, "abcde"},
+{0x081e0256, "abcdef"},
+{0x0adb02bd, "abcdefg"},
+{0x0e000325, "abcdefgh"},
  {0x118e038e, "abcdefghi"},
  {0x158603f8, "abcdefghij"},
  {0x3f090f02, "Discard medicine more than two years old."},
< at >< at > -47,6 +48,9 < at >< at >
  {0x91dd304f, "The fugacity of a constituent in a mixture of gases at a  
given temperature is proportional to its mole fraction.  Lewis-Randall  
Rule"},
  {0x2e5d1316, "How can you write a big system without C++?  -Paul Glick"},
  {0xd0201df6, "'Invariant assertions' is the most elegant programming  
technique!  -Tom Szymanski"},
+{0x86af0001, strings.Repeat("\x00", 1e5)},
+{0x79660b4d, strings.Repeat("a", 1e5)},
+{0x110588ee, strings.Repeat("ABCDEFGHIJKLMNOPQRSTUVWXYZ", 1e4)},
  }

  func TestGolden(t *testing.T) {

Re: code review 6223050: runtime: faster GC mark phase (issue 6223050)

2012-05-24T06:59:01

*** Submitted as
http://code.google.com/p/go/source/detail?r=0ed15bfbe0f4 ***

runtime: faster GC mark phase
Also bump MaxGcproc to 8.

benchmark             old ns/op    new ns/op    delta
Parser               3796323000   3763880000   -0.85%
Parser-2             3591752500   3518560250   -2.04%
Parser-4             3423825250   3334955250   -2.60%
Parser-8             3304585500   3267014750   -1.14%
Parser-16            3313615750   3286160500   -0.83%

Tree                  984128500    942501166   -4.23%
Tree-2                932564444    883266222   -5.29%
Tree-4                835831000    799912777   -4.30%
Tree-8                819238500    789717333   -3.73%
Tree-16               880837833    837840055   -5.13%

Tree2                 604698100    579716900   -4.13%
Tree2-2               372414500    356765200   -4.20%
Tree2-4               187488100    177455900   -5.56%
Tree2-8               136315300    102086700  -25.11%
Tree2-16               93725900     76705800  -22.18%

ParserPause           157441210    166202783   +5.56%
ParserPause-2          93842650     85199900   -9.21%
ParserPause-4          56844404     53535684   -5.82%
ParserPause-8          35739446     30767613  -16.15%
ParserPause-16         32718255     27212441  -16.83%

TreePause              29610557     29787725   +0.60%
TreePause-2            24001659     20674421  -13.86%
TreePause-4            15114887     12842781  -15.03%
TreePause-8            13128725     10741747  -22.22%
TreePause-16           16131360     12506901  -22.47%

Tree2Pause           2673350920   2651045280   -0.83%
Tree2Pause-2         1796999200   1709350040   -4.88%
Tree2Pause-4         1163553320   1090706480   -6.67%
Tree2Pause-8          987032520    858916360  -25.11%
Tree2Pause-16         864758560    809567480   -6.81%

ParserLastPause       280537000    289047000   +3.03%
ParserLastPause-2     183030000    166748000   -8.90%
ParserLastPause-4     105817000     91552000  -13.48%
ParserLastPause-8      65127000     53288000  -18.18%
ParserLastPause-16     45258000     38334000  -15.30%

TreeLastPause          45072000     51449000  +12.39%
TreeLastPause-2        39269000     37866000   -3.57%
TreeLastPause-4        23564000     20649000  -12.37%
TreeLastPause-8        20881000     15807000  -24.30%
TreeLastPause-16       23297000     17309000  -25.70%

Tree2LastPause       6046912000   5797120000   -4.13%
Tree2LastPause-2     3724034000   3567592000   -4.20%
Tree2LastPause-4     1874831000   1774524000   -5.65%
Tree2LastPause-8     1363108000   1020809000  -12.79%
Tree2LastPause-16     937208000    767019000  -22.18%

R=rsc, 0xe2.0x9a.0x9b
CC=golang-dev
http://codereview.appspot.com/6223050


http://codereview.appspot.com/6223050/