Issue 5986054: code review 5986054: runtime: add parallel for algorithm (Closed)
|
Created:
12 years ago by dvyukov Modified:
11 years, 11 months ago Reviewers:
CC:
bsiegert, mpimenov, rsc, minux1, r, golang-dev Visibility:
Public. |
Descriptionruntime: add parallel for algorithm
This is factored out part of:
http://codereview.appspot.com/5279048/
(parallel GC)
Patch Set 1 #Patch Set 2 : diff -r d685ccae1668 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r d685ccae1668 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r d685ccae1668 https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 512073e3b7f2 https://go.googlecode.com/hg/ #Patch Set 6 : diff -r 512073e3b7f2 https://go.googlecode.com/hg/ #Patch Set 7 : diff -r 512073e3b7f2 https://go.googlecode.com/hg/ #Patch Set 8 : diff -r 512073e3b7f2 https://go.googlecode.com/hg/ #
Total comments: 16
Patch Set 9 : diff -r 0051c7442fed https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 10 : diff -r 0051c7442fed https://go.googlecode.com/hg/ #
Total comments: 30
Patch Set 11 : diff -r 14c38c23c819 https://go.googlecode.com/hg/ #Patch Set 12 : diff -r 14c38c23c819 https://go.googlecode.com/hg/ #
Total comments: 12
Patch Set 13 : diff -r 4a18b0e071b1 https://go.googlecode.com/hg/ #Patch Set 14 : diff -r 4a18b0e071b1 https://go.googlecode.com/hg/ #Patch Set 15 : diff -r 26086374b931 https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 16 : diff -r c93b5846e97b https://go.googlecode.com/hg/ #
MessagesTotal messages: 25
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
Just two comments on the comments (heh). http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c File src/pkg/runtime/parfor.c (right): http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:96: // obtain 0-based thread index Shouldn't these be complete sentences, starting with a capital letter and ending with a full stop? Similarly below. http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:115: // while have work, bump low index and execute the iteration s,have,there is,
Sign in to reply to this message.
http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c File src/pkg/runtime/parfor.c (right): http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:66: d = n % nthr; move out of the loop? http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:69: end += (i+1); probably remove parentheses? http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:171: m->gcstats.nsteal += 1; s/+= 1/++/ http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:181: // if a user asked to not wait for others, exit now s/to not/not to/ s/others/the others/ http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:375: // otherwise parfor may return while other threads still working s/still working/are still working/ http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:685: * otherwise, threads can return while other threads still finishing processing. s/still/are still/
Sign in to reply to this message.
http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c File src/pkg/runtime/parfor.c (right): http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:66: d = n % nthr; On 2012/04/09 14:47:34, mpimenov wrote: > move out of the loop? Done. http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:69: end += (i+1); On 2012/04/09 14:47:34, mpimenov wrote: > probably remove parentheses? Done. http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:96: // obtain 0-based thread index On 2012/04/09 14:12:26, bsiegert wrote: > Shouldn't these be complete sentences, starting with a capital letter and ending > with a full stop? Similarly below. Done. http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:115: // while have work, bump low index and execute the iteration On 2012/04/09 14:12:26, bsiegert wrote: > s,have,there is, Done. http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:171: m->gcstats.nsteal += 1; On 2012/04/09 14:47:34, mpimenov wrote: > s/+= 1/++/ Done. http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:181: // if a user asked to not wait for others, exit now On 2012/04/09 14:47:34, mpimenov wrote: > s/to not/not to/ > s/others/the others/ Done. http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:375: // otherwise parfor may return while other threads still working On 2012/04/09 14:47:34, mpimenov wrote: > s/still working/are still working/ Done. http://codereview.appspot.com/5986054/diff/1010/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:685: * otherwise, threads can return while other threads still finishing processing. On 2012/04/09 14:47:34, mpimenov wrote: > s/still/are still/ Done.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, bsiegert@gmail.com, mpimenov@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM after this. http://codereview.appspot.com/5986054/diff/6010/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/5986054/diff/6010/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:360: // executed for each element One more thing, sorry I only saw this now. In the other struct definition in this file, the comments are in the same line as the definition, with an appropriate number of tabs. Please follow the same style.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, bsiegert@gmail.com, mpimenov@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/5986054/diff/6010/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/5986054/diff/6010/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:360: // executed for each element On 2012/04/10 07:05:50, bsiegert wrote: > One more thing, sorry I only saw this now. In the other struct definition in > this file, the comments are in the same line as the definition, with an > appropriate number of tabs. Please follow the same style. Done. Yeah, it's more compact now.
Sign in to reply to this message.
http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.go File src/pkg/runtime/parfor_test.go (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.g... src/pkg/runtime/parfor_test.go:44: (*d)[i] = (*d)[i]*(*d)[i] + 1 Will x := (*d)[i] (*d)[i] = x*x + 1 make it more readable? http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.g... src/pkg/runtime/parfor_test.go:109: go func() { Can't you just go ParforDo(desc) here?
Sign in to reply to this message.
looks pretty good; lots of tiny stuff http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/export_test.go File src/pkg/runtime/export_test.go (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/export_test.g... src/pkg/runtime/export_test.go:41: func parforsetup2(desc *Parfor, nthr, n uint32, ctx *byte, wait bool, body interface{}) Sure looks like this should be body func(*Parfor, uint32), not body interface{}. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/export_test.g... src/pkg/runtime/export_test.go:43: func parforgetiters(desc *Parfor, tid uint32) (uint32, uint32) This says uint32 but your code says uintptr. Change to match code. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c File src/pkg/runtime/parfor.c (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:26: desc->nthrmax = nthrmax; desc->thr = (byte*)(desc+1) + CacheLineSize; http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:40: getthread(Parfor *desc, uint32 i) Can delete; just use desc->thr[i]. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:46: runtime·parforsetup(Parfor *desc, uint32 nthr, uint32 n, void *ctx, bool wait, void (*body)(Parfor*, uint32)) void *body would be fine here (to match the change to a func type) http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:162: if(runtime·cas64(victimpos, &pos, (uint64)begin | (((uint64)begin2)<<32))) { The () around (uint64)begin2 are unnecessary. (uint64)begin | (uint64)begin2<<32 http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:171: runtime·atomicstore64(mypos, (uint64)begin | ((uint64)end)<<32); Same. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:172: m->gcstats.nsteal++; This isn't really gcstats anymore. Maybe put these in the ParforThread instead and gather them at the end? http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:200: // func parforgetiters(desc *Parfor, thr uint32) (uint32, uint32) s/uint32/uintptr/g http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.go File src/pkg/runtime/parfor_test.go (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.g... src/pkg/runtime/parfor_test.go:44: (*d)[i] = (*d)[i]*(*d)[i] + 1 On 2012/04/10 16:12:17, mpimenov wrote: > Will > x := (*d)[i] > (*d)[i] = x*x + 1 > make it more readable? Or just d := *(*[]uint64)(unsafe.Pointer(desc.Ctx)) d[i] = d[i]*d[i] + 1 http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.g... src/pkg/runtime/parfor_test.go:109: go func() { On 2012/04/10 16:12:17, mpimenov wrote: > Can't you just go ParforDo(desc) here? Yes, please do that. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:76: typedef struct ParforThread ParforThread; http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:369: }; ParforThread *thr;
Sign in to reply to this message.
http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/export_test.go File src/pkg/runtime/export_test.go (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/export_test.g... src/pkg/runtime/export_test.go:41: func parforsetup2(desc *Parfor, nthr, n uint32, ctx *byte, wait bool, body interface{}) On 2012/04/10 20:37:53, rsc wrote: > Sure looks like this should be body func(*Parfor, uint32), not body interface{}. Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c File src/pkg/runtime/parfor.c (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:26: desc->nthrmax = nthrmax; On 2012/04/10 20:37:53, rsc wrote: > desc->thr = (byte*)(desc+1) + CacheLineSize; > Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:40: getthread(Parfor *desc, uint32 i) On 2012/04/10 20:37:53, rsc wrote: > Can delete; just use desc->thr[i]. Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:46: runtime·parforsetup(Parfor *desc, uint32 nthr, uint32 n, void *ctx, bool wait, void (*body)(Parfor*, uint32)) On 2012/04/10 20:37:53, rsc wrote: > void *body > would be fine here (to match the change to a func type) Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:162: if(runtime·cas64(victimpos, &pos, (uint64)begin | (((uint64)begin2)<<32))) { On 2012/04/10 20:37:53, rsc wrote: > The () around (uint64)begin2 are unnecessary. > > (uint64)begin | (uint64)begin2<<32 > Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:171: runtime·atomicstore64(mypos, (uint64)begin | ((uint64)end)<<32); On 2012/04/10 20:37:53, rsc wrote: > Same. Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:200: // func parforgetiters(desc *Parfor, thr uint32) (uint32, uint32) On 2012/04/10 20:37:53, rsc wrote: > s/uint32/uintptr/g Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.go File src/pkg/runtime/parfor_test.go (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.g... src/pkg/runtime/parfor_test.go:44: (*d)[i] = (*d)[i]*(*d)[i] + 1 On 2012/04/10 16:12:17, mpimenov wrote: > Will > x := (*d)[i] > (*d)[i] = x*x + 1 > make it more readable? Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.g... src/pkg/runtime/parfor_test.go:44: (*d)[i] = (*d)[i]*(*d)[i] + 1 On 2012/04/10 20:37:53, rsc wrote: > On 2012/04/10 16:12:17, mpimenov wrote: > > Will > > x := (*d)[i] > > (*d)[i] = x*x + 1 > > make it more readable? > > Or just > > d := *(*[]uint64)(unsafe.Pointer(desc.Ctx)) > d[i] = d[i]*d[i] + 1 Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.g... src/pkg/runtime/parfor_test.go:109: go func() { On 2012/04/10 16:12:17, mpimenov wrote: > Can't you just go ParforDo(desc) here? Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor_test.g... src/pkg/runtime/parfor_test.go:109: go func() { On 2012/04/10 20:37:53, rsc wrote: > On 2012/04/10 16:12:17, mpimenov wrote: > > Can't you just go ParforDo(desc) here? > > Yes, please do that. Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:76: On 2012/04/10 20:37:53, rsc wrote: > typedef struct ParforThread ParforThread; Done. http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:369: }; On 2012/04/10 20:37:53, rsc wrote: > ParforThread *thr; Done.
Sign in to reply to this message.
http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h#new... src/pkg/runtime/runtime.h:369: }; On 2012/04/10 20:37:53, rsc wrote: > ParforThread *thr; With this change it does not build: go/src$ ./make.bash # Building C bootstrap tool. cmd/dist # Building compilers and Go bootstrap tool for host, linux/amd64. lib9 ... pkg/runtime go/src/pkg/runtime/zruntime_defs_linux_amd64.go:211: undefined: parforthread go tool dist: FAILED: go/pkg/tool/linux_amd64/6g -o $WORK/_go_.6 -p runtime -+ go/src/pkg/runtime/compiler.go go/src/pkg/runtime/debug.go go/src/pkg/runtime/error.go go/src/pkg/runtime/extern.go go/src/pkg/runtime/mem.go go/src/pkg/runtime/softfloat64.go go/src/pkg/runtime/type.go go/src/pkg/runtime/zgoarch_amd64.go go/src/pkg/runtime/zgoos_linux.go go/src/pkg/runtime/zruntime_defs_linux_amd64.go go/src/pkg/runtime/zversion.go zruntime_defs_linux_amd64.go contains: type parfor struct { body func(*parfor, uint) done uint nthr uint nthrmax uint thrseq uint cnt uint ctx unsafe.Pointer wait uint8 thr *parforthread } but ParforThread is defined only in parfor.c. I can't put ParforThread definition into runtime.h, because it contains CacheLineSize, which is defined in arch.h. Cgo depends on runtime.h, but does not have arch.h. I am not sure what is the way to resolve it.
Sign in to reply to this message.
http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c File src/pkg/runtime/parfor.c (right): http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/parfor.c#newc... src/pkg/runtime/parfor.c:172: m->gcstats.nsteal++; On 2012/04/10 20:37:53, rsc wrote: > This isn't really gcstats anymore. Maybe put these in the ParforThread instead > and gather them at the end? I've thought about it. First, it's some additional code. Second, it's unclear how to handle it when wait==false (that is, threads return from parfordo, while other threads still working). Parfor is going to be used only in GC, so in that sense it's still GCStats. Perhaps, rename GCStats to something more general?
Sign in to reply to this message.
On Thu, Apr 12, 2012 at 5:59 PM, <dvyukov@google.com> wrote: > > http://codereview.appspot.com/**5986054/diff/2018/src/pkg/** > runtime/runtime.h<http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h> > File src/pkg/runtime/runtime.h (right): > > http://codereview.appspot.com/**5986054/diff/2018/src/pkg/** > runtime/runtime.h#newcode369<http://codereview.appspot.com/5986054/diff/2018/src/pkg/runtime/runtime.h#newcode369> > src/pkg/runtime/runtime.h:369: }; > On 2012/04/10 20:37:53, rsc wrote: > >> ParforThread *thr; >> > > With this change it does not build: > > go/src$ ./make.bash > # Building C bootstrap tool. > cmd/dist > > # Building compilers and Go bootstrap tool for host, linux/amd64. > lib9 > ... > pkg/runtime > go/src/pkg/runtime/zruntime_**defs_linux_amd64.go:211: undefined: > parforthread > go tool dist: FAILED: go/pkg/tool/linux_amd64/6g -o $WORK/_go_.6 -p > runtime -+ go/src/pkg/runtime/compiler.go go/src/pkg/runtime/debug.go > go/src/pkg/runtime/error.go go/src/pkg/runtime/extern.go > go/src/pkg/runtime/mem.go go/src/pkg/runtime/**softfloat64.go > go/src/pkg/runtime/type.go go/src/pkg/runtime/zgoarch_**amd64.go > go/src/pkg/runtime/zgoos_**linux.go > go/src/pkg/runtime/zruntime_**defs_linux_amd64.go > go/src/pkg/runtime/zversion.go > > > zruntime_defs_linux_amd64.go contains: > type parfor struct { > body func(*parfor, uint) > done uint > nthr uint > nthrmax uint > thrseq uint > cnt uint > ctx unsafe.Pointer > wait uint8 > thr *parforthread > } > but ParforThread is defined only in parfor.c. > > I can't put ParforThread definition into runtime.h, because it contains > CacheLineSize, which is defined in arch.h. Cgo depends on runtime.h, but > does not have arch.h. > > I am not sure what is the way to resolve it. > add parfor.c to runtimedefs table in cmd/dist/buildruntime.c.
Sign in to reply to this message.
Hello bsiegert@gmail.com, mpimenov@google.com, rsc@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2012/04/12 10:51:51, minux wrote: > add parfor.c to runtimedefs table in cmd/dist/buildruntime.c. Works for me Thanks!
Sign in to reply to this message.
Looks very close. http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/export_test.go File src/pkg/runtime/export_test.go (right): http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/export_test.... src/pkg/runtime/export_test.go:59: var ParforGetIters = func(desc *Parfor, tid uint32) (uint32, uint32) { func ParforIters(desc *Parfor, tid uint32) (uint32, uint32) { ... } http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c File src/pkg/runtime/parfor.c (right): http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:58: begin = n / nthr * i; Instead of the next 9 lines, it seems like you can do begin = (uint64)n*i / nthr; end = (uint64)n*(i+1) / nthr; (and then you don't need mod = n % nthr above either). http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:72: // func parforsetup2(desc *Parfor, nthr, n uint32, ctx unsafe.Pointer, wait bool, body interface{}) This comment looks like it is out of date. Should say body func(Parfor*, uint32) http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:112: if(begin<end) { begin < end (spaces) http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:165: m->gcstats.nsteal++; These are not really gcstats anymore. It seems like these could be local variables and then added to the desc atomically before returning? http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:195: runtime·parforgetiters(Parfor *desc, uintptr tid, uintptr start, uintptr end) s/get// http://golang.org/doc/effective_go.html#Getters
Sign in to reply to this message.
All done. PTAL. http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/export_test.go File src/pkg/runtime/export_test.go (right): http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/export_test.... src/pkg/runtime/export_test.go:59: var ParforGetIters = func(desc *Parfor, tid uint32) (uint32, uint32) { On 2012/04/25 02:34:00, rsc wrote: > func ParforIters(desc *Parfor, tid uint32) (uint32, uint32) { > ... > } > Done. http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c File src/pkg/runtime/parfor.c (right): http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:58: begin = n / nthr * i; On 2012/04/25 02:34:00, rsc wrote: > Instead of the next 9 lines, it seems like you can do > > begin = (uint64)n*i / nthr; > end = (uint64)n*(i+1) / nthr; > > (and then you don't need mod = n % nthr above either). Done. http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:72: // func parforsetup2(desc *Parfor, nthr, n uint32, ctx unsafe.Pointer, wait bool, body interface{}) On 2012/04/25 02:34:00, rsc wrote: > This comment looks like it is out of date. Should say body func(Parfor*, > uint32) Done. http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:112: if(begin<end) { On 2012/04/25 02:34:00, rsc wrote: > begin < end (spaces) Done. http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:165: m->gcstats.nsteal++; On 2012/04/25 02:34:00, rsc wrote: > These are not really gcstats anymore. It seems like these could be local > variables and then added to the desc atomically before returning? Done. http://codereview.appspot.com/5986054/diff/15009/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:195: runtime·parforgetiters(Parfor *desc, uintptr tid, uintptr start, uintptr end) On 2012/04/25 02:34:00, rsc wrote: > s/get// > http://golang.org/doc/effective_go.html#Getters Done.
Sign in to reply to this message.
dumb dumb question: shouldn't this be ParFor?
Sign in to reply to this message.
On 2012/04/26 21:07:28, r wrote: > dumb dumb question: shouldn't this be ParFor? Done. PTAL.
Sign in to reply to this message.
On 2012/04/27 18:39:11, dvyukov wrote: > On 2012/04/26 21:07:28, r wrote: > > dumb dumb question: shouldn't this be ParFor? > > Done. PTAL. ping
Sign in to reply to this message.
LGTM http://codereview.appspot.com/5986054/diff/35001/src/pkg/runtime/parfor.c File src/pkg/runtime/parfor.c (right): http://codereview.appspot.com/5986054/diff/35001/src/pkg/runtime/parfor.c#new... src/pkg/runtime/parfor.c:26: byte pad[CacheLineSize-sizeof(ParForThreadUnpadded)]; Don't you still want byte pad[CacheLineSize] here? And then don't need the Unpadded struct. When there was just one field, you could rely on the fact that the padding would put pos from one struct in a different cache line than pos from another struct. However, for a larger struct, there is no guarantee that one ParForThread's pos won't be in the same cache line as a different ParForThread's nsleep. In fact, unless you get lucky and the struct is exactly cache-line-aligned, you'll get exactly this overlap. Using byte pad[CacheLineSize] avoids that, at a small memory cost.
Sign in to reply to this message.
On 2012/05/08 19:19:58, rsc wrote: > LGTM > > http://codereview.appspot.com/5986054/diff/35001/src/pkg/runtime/parfor.c > File src/pkg/runtime/parfor.c (right): > > http://codereview.appspot.com/5986054/diff/35001/src/pkg/runtime/parfor.c#new... > src/pkg/runtime/parfor.c:26: byte > pad[CacheLineSize-sizeof(ParForThreadUnpadded)]; > Don't you still want byte pad[CacheLineSize] here? And then don't need the > Unpadded struct. > > When there was just one field, you could rely on the fact that the padding would > put pos from one struct in a different cache line than pos from another struct. > However, for a larger struct, there is no guarantee that one ParForThread's pos > won't be in the same cache line as a different ParForThread's nsleep. In fact, > unless you get lucky and the struct is exactly cache-line-aligned, you'll get > exactly this overlap. Using byte pad[CacheLineSize] avoids that, at a small > memory cost. Makes sense. Done.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=345cbca96c55 *** runtime: add parallel for algorithm This is factored out part of: http://codereview.appspot.com/5279048/ (parallel GC) R=bsiegert, mpimenov, rsc, minux.ma, r CC=golang-dev http://codereview.appspot.com/5986054
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
