From 7aa3031ebaa5dc641808f55cb1cb27ddc409fbca Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 6 Aug 2014 15:46:33 -0400 Subject: [PATCH 1/3] cmd/gc: make liveness ~10x faster MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1) The arrayindexof lookup function is O(n). Replace with O(1) lookups. 2) The checkptxt function is O(n²) and is purely for debugging. Only run when the debugging flags are turned on. 3) Iterating over sparse bitmaps can be done faster word by word. Introduce and use bvnext for that. Run times before and after, on my 2.5 GHz Core i5 MacBook Pro. x.go 9.48 0.84 issue 8259 x100.go 0.01 0.01 issue 8354 x1000.go 0.10 0.10 x2000.go 0.62 0.19 x3000.go 1.33 0.34 x4000.go 2.29 0.49 x5000.go 3.89 0.67 x6000.go 5.00 0.90 x7000.go 6.70 1.13 x8000.go 9.44 1.38 x9000.go 11.23 1.87 x10000.go 13.78 2.09 Fixes #8259. Fixes #8354. LGTM=iant, r R=golang-codereviews, iant, r CC=golang-codereviews https://golang.org/cl/125720043 --- src/cmd/gc/array.c | 14 ------------- src/cmd/gc/bv.c | 36 ++++++++++++++++++++++++++------ src/cmd/gc/go.h | 2 +- src/cmd/gc/plive.c | 52 +++++++++++++++++++++++++++++++++------------- 4 files changed, 69 insertions(+), 35 deletions(-) diff --git a/src/cmd/gc/array.c b/src/cmd/gc/array.c index 5e53c1ff0e..611fc9fbd4 100644 --- a/src/cmd/gc/array.c +++ b/src/cmd/gc/array.c @@ -108,20 +108,6 @@ arrayadd(Array *array, void *element) arrayset(array, array->length - 1, element); } -int32 -arrayindexof(Array *array, void *element) -{ - void *p; - int32 i; - - for(i = 0; i < array->length; i++) { - p = arrayget(array, i); - if(memcmp(p, &element, array->size) == 0) - return i; - } - return -1; -} - void arraysort(Array *array, int (*cmp)(const void*, const void*)) { diff --git a/src/cmd/gc/bv.c b/src/cmd/gc/bv.c index 2efbbc565e..0e8f8d4739 100644 --- a/src/cmd/gc/bv.c +++ b/src/cmd/gc/bv.c @@ -9,6 +9,8 @@ enum { WORDSIZE = sizeof(uint32), WORDBITS = 32, + WORDMASK = WORDBITS - 1, + WORDSHIFT = 5, }; static uintptr @@ -94,13 +96,35 @@ bvconcat(Bvec *src1, Bvec *src2) int bvget(Bvec *bv, int32 i) { - uint32 mask, word; - if(i < 0 || i >= bv->n) fatal("bvget: index %d is out of bounds with length %d\n", i, bv->n); - mask = 1U << (i % WORDBITS); - word = bv->b[i / WORDBITS] & mask; - return word ? 1 : 0; + return (bv->b[i>>WORDSHIFT] >> (i&WORDMASK)) & 1; +} + +// bvnext returns the smallest index >= i for which bvget(bv, i) == 1. +// If there is no such index, bvnext returns -1. +int +bvnext(Bvec *bv, int32 i) +{ + uint32 w; + + // Jump i ahead to next word with bits. + if((bv->b[i>>WORDSHIFT]>>(i&WORDMASK)) == 0) { + i &= ~WORDMASK; + i += WORDBITS; + while(i < bv->n && bv->b[i>>WORDSHIFT] == 0) + i += WORDBITS; + } + if(i >= bv->n) + return -1; + + // Find 1 bit. + w = bv->b[i>>WORDSHIFT]>>(i&WORDMASK); + while((w&1) == 0) { + w>>=1; + i++; + } + return i; } int @@ -109,7 +133,7 @@ bvisempty(Bvec *bv) int32 i; for(i = 0; i < bv->n; i += WORDBITS) - if(bv->b[i / WORDBITS] != 0) + if(bv->b[i>>WORDSHIFT] != 0) return 0; return 1; } diff --git a/src/cmd/gc/go.h b/src/cmd/gc/go.h index 30b210f92e..c3da5f636a 100644 --- a/src/cmd/gc/go.h +++ b/src/cmd/gc/go.h @@ -1017,7 +1017,6 @@ int32 arraylength(Array *array); void* arrayget(Array *array, int32 index); void arrayset(Array *array, int32 index, void *element); void arrayadd(Array *array, void *element); -int32 arrayindexof(Array* array, void *element); void arraysort(Array* array, int (*cmp)(const void*, const void*)); /* @@ -1043,6 +1042,7 @@ int bvcmp(Bvec *bv1, Bvec *bv2); void bvcopy(Bvec *dst, Bvec *src); Bvec* bvconcat(Bvec *src1, Bvec *src2); int bvget(Bvec *bv, int32 i); +int32 bvnext(Bvec *bv, int32 i); int bvisempty(Bvec *bv); void bvnot(Bvec *bv); void bvor(Bvec *dst, Bvec *src1, Bvec *src2); diff --git a/src/cmd/gc/plive.c b/src/cmd/gc/plive.c index 9026f6003e..e67b0af757 100644 --- a/src/cmd/gc/plive.c +++ b/src/cmd/gc/plive.c @@ -283,13 +283,30 @@ getvariables(Node *fn) // For arguments and results, the bitmap covers all variables, // so we must include all the variables, even the ones without // pointers. + // + // The Node.opt field is available for use by optimization passes. + // We use it to hold the index of the node in the variables array, plus 1 + // (so that 0 means the Node is not in the variables array). + // Each pass should clear opt when done, but you never know, + // so clear them all ourselves too. + // The Node.curfn field is supposed to be set to the current function + // already, but for some compiler-introduced names it seems not to be, + // so fix that here. + // Later, when we want to find the index of a node in the variables list, + // we will check that n->curfn == curfn and n->opt > 0. Then n->opt - 1 + // is the index in the variables list. + ll->n->opt = nil; + ll->n->curfn = curfn; switch(ll->n->class) { case PAUTO: - if(haspointers(ll->n->type)) + if(haspointers(ll->n->type)) { + ll->n->opt = (void*)(uintptr)(arraylength(result)+1); arrayadd(result, &ll->n); + } break; case PPARAM: case PPARAMOUT: + ll->n->opt = (void*)(uintptr)(arraylength(result)+1); arrayadd(result, &ll->n); break; } @@ -718,14 +735,16 @@ progeffects(Prog *prog, Array *vars, Bvec *uevar, Bvec *varkill, Bvec *avarinit) } if(info.flags & (LeftRead | LeftWrite | LeftAddr)) { from = &prog->from; - if (from->node != nil && from->sym != nil) { + if (from->node != nil && from->sym != nil && from->node->curfn == curfn) { switch(from->node->class & ~PHEAP) { case PAUTO: case PPARAM: case PPARAMOUT: - pos = arrayindexof(vars, from->node); + pos = (int)(uintptr)from->node->opt - 1; // index in vars if(pos == -1) goto Next; + if(pos >= arraylength(vars) || *(Node**)arrayget(vars, pos) != from->node) + fatal("bad bookkeeping in liveness %N %d", from->node, pos); if(from->node->addrtaken) { bvset(avarinit, pos); } else { @@ -741,14 +760,16 @@ progeffects(Prog *prog, Array *vars, Bvec *uevar, Bvec *varkill, Bvec *avarinit) Next: if(info.flags & (RightRead | RightWrite | RightAddr)) { to = &prog->to; - if (to->node != nil && to->sym != nil) { + if (to->node != nil && to->sym != nil && to->node->curfn == curfn) { switch(to->node->class & ~PHEAP) { case PAUTO: case PPARAM: case PPARAMOUT: - pos = arrayindexof(vars, to->node); + pos = (int)(uintptr)to->node->opt - 1; // index in vars if(pos == -1) goto Next1; + if(pos >= arraylength(vars) || *(Node**)arrayget(vars, pos) != to->node) + fatal("bad bookkeeping in liveness %N %d", to->node, pos); if(to->node->addrtaken) { if(prog->as != AVARKILL) bvset(avarinit, pos); @@ -1020,6 +1041,9 @@ checkptxt(Node *fn, Prog *firstp) { Prog *p; + if(debuglive == 0) + return; + for(p = firstp; p != P; p = p->link) { if(0) print("analyzing '%P'\n", p); @@ -1172,21 +1196,17 @@ twobitlivepointermap(Liveness *lv, Bvec *liveout, Array *vars, Bvec *args, Bvec vlong xoffset; int32 i; - for(i = 0; i < arraylength(vars); i++) { + for(i = 0; (i = bvnext(liveout, i)) >= 0; i++) { node = *(Node**)arrayget(vars, i); switch(node->class) { case PAUTO: - if(bvget(liveout, i)) { - xoffset = node->xoffset + stkptrsize; - twobitwalktype1(node->type, &xoffset, locals); - } + xoffset = node->xoffset + stkptrsize; + twobitwalktype1(node->type, &xoffset, locals); break; case PPARAM: case PPARAMOUT: - if(bvget(liveout, i)) { - xoffset = node->xoffset; - twobitwalktype1(node->type, &xoffset, args); - } + xoffset = node->xoffset; + twobitwalktype1(node->type, &xoffset, args); break; } } @@ -1944,6 +1964,7 @@ liveness(Node *fn, Prog *firstp, Sym *argssym, Sym *livesym) Array *cfg, *vars; Liveness *lv; int debugdelta; + NodeList *l; // Change name to dump debugging information only for a specific function. debugdelta = 0; @@ -1984,6 +2005,9 @@ liveness(Node *fn, Prog *firstp, Sym *argssym, Sym *livesym) twobitwritesymbol(lv->argslivepointers, argssym); // Free everything. + for(l=fn->dcl; l != nil; l = l->next) + if(l->n != N) + l->n->opt = nil; freeliveness(lv); arrayfree(vars); freecfg(cfg); From 4cc8be60e99286bcef6c4dc247d01210e89ba73f Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 6 Aug 2014 16:10:08 -0400 Subject: [PATCH 2/3] codereview: fix submit of merge + include branch prefix in submitted message hg insists that we not list explicit files for a merge. for benefit of other tools reading logs, include branch prefix at start of every commit message. LGTM=minux R=minux CC=golang-codereviews https://golang.org/cl/124780044 --- lib/codereview/codereview.py | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/lib/codereview/codereview.py b/lib/codereview/codereview.py index a64401533c..b5fdf1338e 100644 --- a/lib/codereview/codereview.py +++ b/lib/codereview/codereview.py @@ -308,18 +308,13 @@ class CL(object): os.unlink(dir + "/cl." + self.name) def Subject(self, ui, repo): - branchPrefix = "" - branch = repo[None].branch() - if branch.startswith("dev."): - branchPrefix = "[" + branch + "] " - s = line1(self.desc) if len(s) > 60: s = s[0:55] + "..." if self.name != "new": s = "code review %s: %s" % (self.name, s) typecheck(s, str) - return branchPrefix + s + return branch_prefix(ui, repo) + s def Upload(self, ui, repo, send_mail=False, gofmt=True, gofmt_just_warn=False, creating=False, quiet=False): if not self.files and not creating: @@ -394,6 +389,7 @@ class CL(object): if vcs: set_status("uploading base files") vcs.UploadBaseFiles(issue, rpc, patches, patchset, upload_options, files) + MySend("/" + issue + "/upload_complete/" + patchset, payload="") if send_mail: set_status("sending mail") MySend("/" + issue + "/mail", payload="") @@ -1921,6 +1917,13 @@ def pending(ui, repo, *pats, **opts): def need_sync(): raise hg_util.Abort("local repository out of date; must sync before submit") +def branch_prefix(ui, repo): + prefix = "" + branch = repo[None].branch() + if branch.startswith("dev."): + prefix = "[" + branch + "] " + return prefix + @hgcommand def submit(ui, repo, *pats, **opts): """submit change to remote repository @@ -1990,7 +1993,7 @@ def submit(ui, repo, *pats, **opts): cl.Mail(ui, repo) # submit changes locally - message = cl.desc.rstrip() + "\n\n" + about + message = branch_prefix(ui, repo) + cl.desc.rstrip() + "\n\n" + about typecheck(message, str) set_status("pushing " + cl.name + " to remote server") @@ -2000,12 +2003,22 @@ def submit(ui, repo, *pats, **opts): old_heads = len(hg_heads(ui, repo).split()) + # Normally we commit listing the specific files in the CL. + # If there are no changed files other than those in the CL, however, + # let hg build the list, because then committing a merge works. + # (You cannot name files for a merge commit, even if you name + # all the files that would be committed by not naming any.) + files = ['path:'+f for f in cl.files] + if ChangedFiles(ui, repo, []) == cl.files: + files = [] + global commit_okay commit_okay = True - ret = hg_commit(ui, repo, *['path:'+f for f in cl.files], message=message, user=userline) + ret = hg_commit(ui, repo, *files, message=message, user=userline) commit_okay = False if ret: raise hg_util.Abort("nothing changed") + node = repo["-1"].node() # push to remote; if it fails for any reason, roll back try: From 2185dbcd8554ef728df06c30ef83e4c61e8c17d8 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 6 Aug 2014 16:12:24 -0400 Subject: [PATCH 3/3] codereview: do not hit upload_complete on first patch Causes server 500 error, so don't do it. TBR=minux CC=golang-codereviews https://golang.org/cl/125770043 --- lib/codereview/codereview.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/codereview/codereview.py b/lib/codereview/codereview.py index b5fdf1338e..fdf11d1f48 100644 --- a/lib/codereview/codereview.py +++ b/lib/codereview/codereview.py @@ -389,7 +389,8 @@ class CL(object): if vcs: set_status("uploading base files") vcs.UploadBaseFiles(issue, rpc, patches, patchset, upload_options, files) - MySend("/" + issue + "/upload_complete/" + patchset, payload="") + if patchset != "1": + MySend("/" + issue + "/upload_complete/" + patchset, payload="") if send_mail: set_status("sending mail") MySend("/" + issue + "/mail", payload="")