|
|
Created:
12 years, 4 months ago by atom Modified:
10 years, 6 months ago CC:
golang-dev Visibility:
Public. |
Descriptionruntime, reflect, ld, gc: garbage collection precision improvements
Patch Set 1 #Patch Set 2 : diff -r cc8a35781b5e https://go.googlecode.com/hg/ #Patch Set 3 : diff -r cc8a35781b5e https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 2eec2501961c https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 4a18b0e071b1 https://go.googlecode.com/hg/ #Patch Set 6 : diff -r 4a18b0e071b1 https://go.googlecode.com/hg/ #Patch Set 7 : diff -r d063a7844d48 https://go.googlecode.com/hg/ #Patch Set 8 : diff -r 95456588a8e1 https://go.googlecode.com/hg/ #Patch Set 9 : diff -r 95456588a8e1 https://go.googlecode.com/hg/ #Patch Set 10 : diff -r 9182664c616f https://go.googlecode.com/hg/ #Patch Set 11 : diff -r 9e5ed0741dc8 https://go.googlecode.com/hg/ #
Total comments: 53
Patch Set 12 : diff -r 5eb6fe483779 https://go.googlecode.com/hg/ #Patch Set 13 : diff -r 645947213cac https://go.googlecode.com/hg/ #Patch Set 14 : diff -r 645947213cac https://go.googlecode.com/hg/ #Patch Set 15 : diff -r 645947213cac https://go.googlecode.com/hg/ #Patch Set 16 : diff -r d8bd45866999 https://go.googlecode.com/hg/ #Patch Set 17 : diff -r 7cbb8aa08f8e https://go.googlecode.com/hg/ #Patch Set 18 : diff -r dfb1b5655e21 https://go.googlecode.com/hg/ #Patch Set 19 : diff -r 8d919bfe75d3 https://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 20 : diff -r 9d15015fc6e2 https://go.googlecode.com/hg/ #Patch Set 21 : diff -r aa5d9f234a8e https://go.googlecode.com/hg/ #Patch Set 22 : diff -r e8cfa948baf2 https://go.googlecode.com/hg/ #Patch Set 23 : diff -r ecab7a7e7c7e https://go.googlecode.com/hg/ #Patch Set 24 : diff -r e4df7915ab89 https://go.googlecode.com/hg/ #Patch Set 25 : diff -r 49d533bb7cd3 https://go.googlecode.com/hg/ #Patch Set 26 : diff -r 8d4bd93dcd41 https://go.googlecode.com/hg/ #Patch Set 27 : diff -r 8f9b0fbf4c15 https://go.googlecode.com/hg/ #Patch Set 28 : diff -r 85d759cdb33d https://go.googlecode.com/hg/ #Patch Set 29 : diff -r f3e8dfc67e45 https://go.googlecode.com/hg/ #Patch Set 30 : diff -r a565c143b41b https://code.google.com/p/go/ #Patch Set 31 : diff -r f545866390ab https://code.google.com/p/go/ #Patch Set 32 : diff -r fed789ce8072 https://code.google.com/p/go/ #Patch Set 33 : diff -r dcb835ff82e0 https://code.google.com/p/go/ #Patch Set 34 : diff -r dcb835ff82e0 https://code.google.com/p/go/ #
Total comments: 1
Patch Set 35 : diff -r 5957d9d08900 https://code.google.com/p/go/ #Patch Set 36 : diff -r 1f90b5850ae2 https://code.google.com/p/go/ #Patch Set 37 : diff -r d6e06d0f3c29 https://code.google.com/p/go/ #
MessagesTotal messages: 63
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.
You're not in the A+C files yet, but I see you've filled in the CLA. How do you want to appear there?
Sign in to reply to this message.
If possible I would like somebody to apply this patch on a 64-bit machine and run src/all.bash.
Sign in to reply to this message.
> > If possible I would like somebody to apply this patch on a 64-bit > machine and run src/all.bash. # Building C bootstrap tool. cmd/dist # Building compilers and Go bootstrap tool for host, linux/amd64. lib9 libbio <...> pkg/go/build cmd/go unexpected fault address 0x3f497a6730 throw: fault [signal 0xb code=0x1 addr=0x3f497a6730 pc=0x43daa6] goroutine 1 [running]: unicode.init() go/src/pkg/unicode/tables.go:48 +0x54 go/parser.init() go/src/pkg/go/parser/parser.go:2343 +0x43 main.init() go/src/cmd/go/bootstrap.go:18 +0x51 goroutine 2 [runnable]: created by runtime.main go/src/pkg/runtime/proc.c:221
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, mpimenov@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2012/04/25 16:14:43, atom wrote: > Hello mailto:golang-dev@googlegroups.com, mailto:dsymonds@golang.org, mailto:mpimenov@google.com (cc: > mailto:golang-dev@googlegroups.com), > > Please take another look. On FreeBSD/amd64: ALL TESTS PASSED I haven't looked at the code, so this shouldn't say anything about that.
Sign in to reply to this message.
linux/amd64: all tests passed darwin/amd64: hung on testing go/printer (spining) the first attempt, then i interrupted it and ran go test just in that directory, which passed. subsequently all.bash passed without issue twice.
Sign in to reply to this message.
On 2012/04/25 16:42:18, aam wrote: > darwin/amd64: hung on testing go/printer (spining) the first attempt, > then i interrupted it and ran go test just in that directory, which > passed. subsequently all.bash passed without issue twice. Are you sure the spinning test was go/printer? A valid method for determining which test is spinning is to look at the list of processes on the machine (Linux: top, htop).
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, mpimenov@google.com, devon.odell@gmail.com, mirtchovski@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Patch set 5: - Fixes a potential race condition when setting typeinfo in MSpan. - Adds simple compression for the typeinfo stored in MSpan. This works well for synthetic benchmarks, I am not sure how well it works in normal programs. Results for test/bench/garbage/tree2.go benchmark: tip: 115 MB heap patchset4: 143 MB heap patchset5: 120 MB heap
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, mpimenov@google.com, devon.odell@gmail.com, mirtchovski@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Patch set 6: - Add support for cases such as: type S struct {a,b,c T} s := new(S) x := &s.a // 'x' and 's' point to the same object, // but their types are different (*T, *S)
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, mpimenov@google.com, devon.odell@gmail.com, mirtchovski@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Patch set 7: - add a mini buffer in scanblock(), and consequently do not use runtime·casp() - handle interface{} - optimize symtab.c
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, mpimenov@google.com, devon.odell@gmail.com, mirtchovski@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Patch set 8 is the last major patch set in this code review. There will be no changes to this patch set, except for the cases of bugs in the code and except source code documentation additions and except coding style changes. * I would like somebody to start a review of the source code. * Patch set 8: - handle Go maps - handle interfaces (in C: Iface) - split stack preallocation at the start of GC (works on i386 only, not sure why it does not work on amd64 so it is disabled on amd64) - optimize mprof.goc
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, mpimenov@google.com, devon.odell@gmail.com, mirtchovski@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On Mon, May 7, 2012 at 4:19 AM, <0xE2.0x9A.0x9B@gmail.com> wrote: > * I would like somebody to start a review of the source code. * I'm sorry for not getting to this yet. I'm trying to get out of a backlog of code reviews, and I'm making progress. I am certainly looking forward to reviewing this. Russ
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, mpimenov@google.com, devon.odell@gmail.com, mirtchovski@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
... some files were missing from the patches because I forgot to update the file list via "hg change 6114046".
Sign in to reply to this message.
Thanks for working on this. I understand that this code is primarily about adding precise garbage collection, but it seems to be making quite a few changes to things like the handoff algorithm and parallelization of the collection, which Dmitry already has in progress. I am thrilled that both of you are improving the GC, and I don't want you to step on each other's toes. Dmitry's parallelization code is partially submitted. I hope we can get that fully submitted in the next couple weeks, and then I hope that you'll be able to update this CL to use that, so that the changes focus on the precise GC part. To that end, in this CL my plan is to review primarily the parts concerned with precise GC and to ignore for now the parallelization aspects, since I expect that they might change. Does that seem reasonable? Thanks. Russ
Sign in to reply to this message.
On Thu, May 10, 2012 at 8:56 PM, Russ Cox <rsc@golang.org> wrote: > Thanks for working on this. > > I understand that this code is primarily about adding precise garbage > collection, but it seems to be making quite a few changes to things > like the handoff algorithm and parallelization of the collection, > which Dmitry already has in progress. I am thrilled that both of you > are improving the GC, and I don't want you to step on each other's > toes. I am aware of the existence of Dmitry's changes, but I haven't examined them. I would like to know the relevant links on http://codereview.appspot.com. The GC is already parallel, so I am assuming that Dmitry's changes are about further improvements to running GC on multiple threads. > Dmitry's parallelization code is partially submitted. I hope we can > get that fully submitted in the next couple weeks, and then I hope > that you'll be able to update this CL to use that, so that the changes > focus on the precise GC part. > > To that end, in this CL my plan is to review primarily the parts > concerned with precise GC and to ignore for now the parallelization > aspects, since I expect that they might change. > > Does that seem reasonable? It seems reasonable to me.
Sign in to reply to this message.
I believe these are the relevant CLs by Dmitry: http://codereview.appspot.com/5986054/ http://codereview.appspot.com/5534044/ This was the original and has been the source for a sequence of smaller pieces: http://codereview.appspot.com/5279048/ Russ
Sign in to reply to this message.
Just a note: The more precise GC can handle the i386 build issue that started appearing at http://build.golang.org since revision 345cbca96c55.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, mpimenov@google.com, devon.odell@gmail.com, mirtchovski@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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#newco... 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#newco... 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#newco... 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#newco... 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#newco... src/pkg/runtime/mgc0.c:1837: case 2: unindent http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newco... 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#ne... 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#new... 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#newc... 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#old... 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#new... 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#new... 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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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#new... src/pkg/runtime/symtab.c:223: if(!writestr) { On 2012/05/24 16:36:45, rsc wrote: > This block (the new code) should be its own function, since it appears here and > then 10 lines later again. Done. http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/symtab.c#new... src/pkg/runtime/symtab.c:226: if(l > 0) { On 2012/05/24 16:36:45, rsc wrote: > Why make l > 0 a special case? The code here would work fine for l == 0 too. > Same below. If it is the last one and l==0, then hugestring.str+hugestring.len may point just beyond the memory region allocated for the hugestring. This may cause a memory leak if the pointed-to address belongs to a completely different object.
Sign in to reply to this message.
Hello dvyukov@google.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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) On 2012/05/24 16:36:45, rsc wrote: > Please change r to off (as in offset). Done. http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode831 src/cmd/ld/data.c:831: align_symsize(int32 s) On 2012/05/24 16:36:45, rsc wrote: > s/_// to match the rest of the functions here Done. 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) On 2012/05/24 16:36:45, rsc wrote: > s/_// Done. 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) On 2012/05/24 16:36:45, rsc wrote: > s/_// Done. 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) On 2012/05/24 17:00:33, rsc wrote: > Please s/ofs/off/ to match other code here. Done. 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++) { On 2012/05/24 17:00:33, rsc wrote: > 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); > } Done. http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode883 src/cmd/ld/data.c:883: if(PtrSize == 4) { On 2012/05/24 17:00:33, rsc wrote: > Use adduintxx. Done. http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode901 src/cmd/ld/data.c:901: Sym *data_gc, *bss_gc; On 2012/05/24 16:36:45, rsc wrote: > s/_// Done. http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode956 src/cmd/ld/data.c:956: /* Thinking. http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode964 src/cmd/ld/data.c:964: if(PtrSize == 4) { On 2012/05/24 17:00:33, rsc wrote: > can avoid the if. > adduintxx(data_gc, 0, PtrSize); > adduintxx(bss_gc, 0, PtrSize); Done. http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode1012 src/cmd/ld/data.c:1012: if(PtrSize == 4) { On 2012/05/24 17:00:33, rsc wrote: > Same. Done. http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode1135 src/cmd/ld/data.c:1135: if(PtrSize == 4) On 2012/05/24 17:00:33, rsc wrote: > Use setuintxx. Done. http://codereview.appspot.com/6114046/diff/42001/src/cmd/ld/data.c#newcode1152 src/cmd/ld/data.c:1152: if(PtrSize == 4) On 2012/05/24 17:00:33, rsc wrote: > Use setuintxx. Done. 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, On 2012/05/24 16:36:45, rsc wrote: > 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. Done. 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 This was copied from cmd/ld/dwarf.c 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#newco... src/pkg/runtime/mgc0.c:90: { On 2012/05/24 16:36:45, rsc wrote: > Please sync with dmitriy's latest code. Done. http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newco... src/pkg/runtime/mgc0.c:434: if( ((uintptr)obj & ((uintptr)PtrSize-1)) != 0 ) { On 2012/05/24 16:36:45, rsc wrote: > Avoid spaces inside if( ) parens. Done. But I am unable to quickly read it without the spaces. http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newco... src/pkg/runtime/mgc0.c:517: struct Y *ybufpos = ybuf; On 2012/05/24 16:36:45, rsc wrote: > Please move all variable declarations to top of function, and don't initialize > and declare in the same statement. Done. http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newco... src/pkg/runtime/mgc0.c:829: Hmap *h = (Hmap*)b; On 2012/05/24 16:36:45, rsc wrote: > Please move variables to top, avoiding { } around case blocks. > Done. http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newco... src/pkg/runtime/mgc0.c:1837: case 2: On 2012/05/24 16:36:45, rsc wrote: > unindent Done. http://codereview.appspot.com/6114046/diff/42001/src/pkg/runtime/mgc0.c#newco... src/pkg/runtime/mgc0.c:2089: int32 i; On 2012/05/24 16:36:45, rsc wrote: > Move variables to top. Done. 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#ne... src/pkg/runtime/runtime.c:114: // runtime·dopanic(0); On 2012/05/24 16:36:45, rsc wrote: > Remove this file from the CL? Done. 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#new... src/pkg/runtime/sema.goc:46: union semtable_t On 2012/05/24 16:36:45, rsc wrote: > s/_t// Done. 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#newc... src/pkg/runtime/slice.c:45: if(cap != 0) { On 2012/05/24 16:36:45, rsc wrote: > 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 Done.
Sign in to reply to this message.
Hello dvyukov@google.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Patch set 13: - a couple of bugfixes - support for slices - support for unsafe_New and unsafe_NewArray - minor performance improvements when GOMAXPROCS >= 2
Sign in to reply to this message.
On 2012/06/24 16:06:04, atom wrote: > Patch set 13: > > - a couple of bugfixes > - support for slices > - support for unsafe_New and unsafe_NewArray > - minor performance improvements when GOMAXPROCS >= 2 It seems that you hit the same issue as me: http://codereview.appspot.com/6114046/diff2/49002:64001/src/pkg/runtime/parfor.c Can you revert SysAlloc with this change? http://codereview.appspot.com/6339051/
Sign in to reply to this message.
On 2012/06/25 07:04:52, dvyukov wrote: > On 2012/06/24 16:06:04, atom wrote: > > Patch set 13: > > > > - a couple of bugfixes > > - support for slices > > - support for unsafe_New and unsafe_NewArray > > - minor performance improvements when GOMAXPROCS >= 2 > > It seems that you hit the same issue as me: > http://codereview.appspot.com/6114046/diff2/49002:64001/src/pkg/runtime/parfor.c > > Can you revert SysAlloc with this change? > http://codereview.appspot.com/6339051/ Ah I see: 41 if(runtime·gcing) 42 runtime·throw("malloc called while gcing");
Sign in to reply to this message.
Hello dvyukov@google.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2012/06/25 07:04:52, dvyukov wrote: > It seems that you hit the same issue as me: > http://codereview.appspot.com/6114046/diff2/49002:64001/src/pkg/runtime/parfor.c > > Can you revert SysAlloc with this change? > http://codereview.appspot.com/6339051/ I reverted the SysAlloc, and rewrote the code so that calls to parforalloc() happen a bit sooner.
Sign in to reply to this message.
On 2012/06/25 08:00:10, atom wrote: > On 2012/06/25 07:04:52, dvyukov wrote: > > It seems that you hit the same issue as me: > > > http://codereview.appspot.com/6114046/diff2/49002:64001/src/pkg/runtime/parfor.c > > > > Can you revert SysAlloc with this change? > > http://codereview.appspot.com/6339051/ > > I reverted the SysAlloc, and rewrote the code so that calls to parforalloc() > happen a bit sooner. Please restore m->locks++/-- around parforalloc.
Sign in to reply to this message.
Hello dvyukov@google.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello. This CL does not apply cleanly after d8bd45866999, could you please re mail it, i'd like to see how it goes on arm. On Mon, Jun 25, 2012 at 6:24 PM, <0xE2.0x9A.0x9B@gmail.com> wrote: > Hello dvyukov@google.com, rsc@golang.org (cc: > golang-dev@googlegroups.com), > > Please take another look. > > > http://codereview.appspot.com/6114046/
Sign in to reply to this message.
Hello dvyukov@google.com, rsc@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Some intersting results on linux/arm benchmark old ns/op new ns/op delta BenchmarkBinaryTree17 37367981000 53971314000 +44.43% BenchmarkFannkuch11 34773498000 34745026000 -0.08% BenchmarkGobDecode 124826050 125396700 +0.46% BenchmarkGobEncode 59490960 59654540 +0.27% BenchmarkGzip 5533813000 5578705000 +0.81% BenchmarkGunzip 1193909000 1199798000 +0.49% BenchmarkJSONEncode 817492600 860882600 +5.31% BenchmarkJSONDecode 2742920000 2766296000 +0.85% BenchmarkMandelbrot200 45582280 45672620 +0.20% BenchmarkParse 63706660 70501700 +10.67% BenchmarkRevcomp 142904600 138769500 -2.89% BenchmarkTemplate 5985047000 6121277000 +2.28% benchmark old MB/s new MB/s speedup BenchmarkGobDecode 6.15 6.12 1.00x BenchmarkGobEncode 12.90 12.87 1.00x BenchmarkGzip 3.51 3.48 0.99x BenchmarkGunzip 16.25 16.17 1.00x BenchmarkJSONEncode 2.37 2.25 0.95x BenchmarkJSONDecode 0.71 0.70 0.99x BenchmarkParse 0.91 0.82 0.90x BenchmarkRevcomp 17.79 18.32 1.03x BenchmarkTemplate 0.32 0.32 1.00x
Sign in to reply to this message.
On 2012/06/28 22:48:36, dfc wrote: > Some intersting results on linux/arm > > benchmark old ns/op new ns/op delta > BenchmarkBinaryTree17 37367981000 53971314000 +44.43% > BenchmarkParse 63706660 70501700 +10.67% The slowdown (on i386: +17%) in BenchmarkBinaryTree17 is for the most part caused by setting type information of Go objects at allocation time. In this benchmark, the performance of GC itself is approximately the same as performance of the old GC. Maybe this is also happening on ARM. You can check this by executing: GOGCTRACE=1 binary-tree -n=17 and observing the number of milliseconds consumed by individual garbage collections.
Sign in to reply to this message.
On 2012/06/28 22:48:36, dfc wrote: > Some intersting results on linux/arm > > benchmark old ns/op new ns/op delta > BenchmarkBinaryTree17 37367981000 53971314000 +44.43% > BenchmarkParse 63706660 70501700 +10.67% The slowdown (on i386: +17%) in BenchmarkBinaryTree17 is for the most part caused by setting type information of Go objects at allocation time. In this benchmark, the performance of GC itself is approximately the same as performance of the old GC. Maybe this is also happening on ARM. You can check this by executing: GOGCTRACE=1 binary-tree -n=17 and observing the number of milliseconds consumed by individual garbage collections.
Sign in to reply to this message.
Hello dvyukov@google.com, rsc@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Patchset 17: - sync with the current tip of Go repository - fixes a bug in mgc0.c that was introduced in a previous patchset - adds memorydump() debugging function
Sign in to reply to this message.
Hi atom, could you please hg sync and upload a new Patch Set against current tip? I tried to merge myself, but failed. Thank you.
Sign in to reply to this message.
Hello dvyukov@google.com, rsc@golang.org, dave@cheney.net, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hi atom, I just applied Patch Set 18 to tip, and ./make.bash failed when it tried to run go_bootstrap. panic: runtime error: invalid memory address or nil pointer dereference [signal 0xb code=0x1 addr=0x3 pc=0x444a3e] goroutine 1 [running]: goroutine 2 [runnable]: created by runtime.main /home/minux/go/go.hg3/src/pkg/runtime/proc.c:222 I'm using Linux/amd64 (Linux/386 works fine). maybe because the recent 64-bit int change?
Sign in to reply to this message.
Hello atom, Is it possible to update the patchset now that CL6569057 has been submitted? Thanks.
Sign in to reply to this message.
Hello dvyukov@google.com, rsc@golang.org, dave@cheney.net, minux.ma@gmail.com, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): http://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c#newc... src/pkg/runtime/mgc0.c:516: // Nearly the same code as the single-threaded version. is it really necessary to duplicate the whole code? can't we just have the ifs around the differences? http://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c#newc... src/pkg/runtime/mgc0.c:2024: // This allows the arguments to be easily passed via reflect·call. and what do the arguments mean?
Sign in to reply to this message.
http://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): http://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c#newc... src/pkg/runtime/mgc0.c:1930: runtime·memorydump(void) Is it possible to send a separate review for memorydump or does it have dependencies?
Sign in to reply to this message.
https://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c#new... src/pkg/runtime/mgc0.c:516: // Nearly the same code as the single-threaded version. On 2012/10/26 07:01:14, remyoudompheng wrote: > is it really necessary to duplicate the whole code? > can't we just have the ifs around the differences? This is hard to say. It may happen that in year 2013 the C code for this function will be partially generated by a program. The reason why I think this may happen is that it is hard to see errors in the garbage collector implementation. Assuming the C code will be generated automatically, duplication of code pieces shouldn't be hard to implement in the generator. Also, it is performance-sensitive code. https://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c#new... src/pkg/runtime/mgc0.c:2024: // This allows the arguments to be easily passed via reflect·call. On 2012/10/26 07:01:14, remyoudompheng wrote: > and what do the arguments mean? The argument 'force' isn't documented in previous versions of mgc0.c, so I left it undocumented.
Sign in to reply to this message.
https://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/6114046/diff/113001/src/pkg/runtime/mgc0.c#new... src/pkg/runtime/mgc0.c:1930: runtime·memorydump(void) On 2012/10/27 08:48:43, remyoudompheng wrote: > Is it possible to send a separate review for memorydump or does it have > dependencies? Done.
Sign in to reply to this message.
How do you suggest to proceed with the remaining 1500 lines of diffs? It is still a bit big to digest in one CL. Thanks.
Sign in to reply to this message.
On 2012/11/01 18:48:51, rsc wrote: > How do you suggest to proceed with the remaining 1500 lines of diffs? It is > still a bit big to digest in one CL. > > Thanks. 1. CL1: Add moreframesize_minalloc to struct M and use reflect·call() to call function mgc0.c:gc() 2. Ignore mprof.goc for now 3. CL2: Temporarily use "goto trailing_pointers" in most cases in mgc0.c:/switch(pc[0]). This should reduce the size of function scanblock() and postpone the need to merge hashmap.{h,c}. Ignore the constants DebugTypeAtBlockEnd and DebugStat. The resulting diff should be less than 1000 lines and scanblock() should be easier to review. In CL2 certain code lines related to object marking will be duplicated 3 times. This seems unavoidable for now (the goal is to achieve higher performance, but the C compiler does not have enough information to specialize and inline functions). Does the above seem reasonable?
Sign in to reply to this message.
On 2012/11/03 11:02:28, atom wrote: > 1. CL1: Add moreframesize_minalloc to struct M and use reflect·call() to call > function mgc0.c:gc() > > 2. Ignore mprof.goc for now > > 3. CL2: Temporarily use "goto trailing_pointers" in most cases in > mgc0.c:/switch(pc[0]). This should reduce the size of function scanblock() and > postpone the need to merge hashmap.{h,c}. Ignore the constants > DebugTypeAtBlockEnd and DebugStat. The resulting diff should be less than 1000 > lines and scanblock() should be easier to review. > > In CL2 certain code lines related to object marking will be duplicated 3 times. > This seems unavoidable for now (the goal is to achieve higher performance, but > the C compiler does not have enough information to specialize and inline > functions). > > Does the above seem reasonable? I suggest: 1. you send CL1 2. you send a non-duplicated CL2 (it may have poor performance, i'm not sure it's perceptible enough and people are not supposed to use tip in production) 3. you send a performance patch (that may duplicate code), the important parts will have been reviewed using a more comfortable codebase. I say we can discuss the best way we are going to maybe do something, but probably you should just prepare the CLs if it is quick and we'll see whether they look digestible.
Sign in to reply to this message.
Hi atom, after applying the latest patch set (20), and update to rev 7ea16cd2859c, encoding/xml fails with strange errors on darwin/amd64 (I suspect linux/amd64 will also fail). Also note that ./all.bash passes on darwin/386 and darwin/arm on that revision. the patch works quite well before that rev. Do you have any ideas why?
Sign in to reply to this message.
On 2012/12/23 19:26:27, minux wrote: > Hi atom, > after applying the latest patch set (20), and update to rev 7ea16cd2859c, > encoding/xml fails with strange errors on darwin/amd64 (I suspect linux/amd64 > will also fail). Also note that ./all.bash passes on darwin/386 and darwin/arm > on that revision. > the patch works quite well before that rev. > Do you have any ideas why? Hi. I am unable to reproduce the issue on linux/amd64, rev 87f67aadaed6. If it also fails with rev 87f67aadaed6 on your machine: Does the failure occur randomly, or every time you test encoding/xml?
Sign in to reply to this message.
On Mon, Dec 24, 2012 at 4:08 AM, <0xE2.0x9A.0x9B@gmail.com> wrote: > If it also fails with rev 87f67aadaed6 on your machine: Does the failure > occur randomly, or every time you test encoding/xml? > randomly but with a high probability of failure (all about the same test TestUnmarshalFeed). for example: --- FAIL: TestUnmarshalFeed (0.00 seconds) read_test.go:21: have xml.Feed{XMLName:xml.Name{Space:" http://www.w3.org/2005/Atom", Local:"feed"}, Title:"Code Review - My issues", Id:"http://codereview.appspot.com/", Link:[]xml.Link{xml.Link{Rel:"alternate", Href:" http://codereview.appspot.com/"}, xml.Link{Rel:"self", Href:" http://codereview.appspot.com/rss/mine/rsc"}}, Updated:time.Time{sec:63390216958, nsec:0, loc:(*time.Location)(0xc2000f4180)}, Author:xml.Person{Name:"rietveld<>", URI:"", Email:"", InnerXML:"<name>rietveld<></name>"}, Entry:[]xml.Entry{xml.Entry{Title:"rietveld: an attempt at pubsubhubbub\n", Id:"urn:md5:134d9179c41f806be79b3a5f7877d19a", Link:[]xml.Link{xml.Link{Rel:"alternate", Href:" http://codereview.appspot.com/126085"}}, Updated:time.Time{sec:63390216958, nsec:0, loc:(*time.Location)(0xc2000f41e0)}, Author:xml.Person{Name:"email-address-removed", URI:"", Email:"", InnerXML:"<name>email-address-removed</name>"}, Summary:xml.Text{Type:"html", Body:"\n An attempt at adding pubsubhubbub support to Rietveld.\nhttp:// code.google.com/p/pubsubhubbub\nhttp://code.google.com/p/rietveld/issues/detail?id=155\n\nTheserver side of the protocol is trivial:\n 1. add a <link rel="hub" href="hub-server"> tag to all\n feeds that will be pubsubhubbubbed.\n 2. every time one of those feeds changes, tell the hub\n with a simple POST request.\n\nI have tested this by adding debug prints to a local hub\nserver and checking that the server got the right publish\nrequests.\n\nI can't quite get the server to work, but I think the bug\nis not in my code. I think that the server expects to be\nable to grab the feed and see the feed's actual URL in\nthe link rel="self", but the default value for that drops\nthe :port from the URL, and I cannot for the life of me\nfigure out how to get the Atom generator deep inside\ndjango not to do that, or even where it is doing that,\nor even what code is running to generate the Atom feed.\n(I thought I knew but I added some assert False statements\nand it kept running!)\n\nIgnoring that particular problem, I would appreciate\nfeedback on the right way to get the two values at\nthe top of feeds.py marked NOTE(rsc).\n\n\n"}}, xml.Entry{Title:"rietveld: correct tab handling\n", Id:"urn:md5:0a2a4f19bb815101f0ba2904aed7c35a", Link:[]xml.Link{xml.Link{Rel:"\x00\x18\x0f\x00\xc2\x00\x00\x00\x01", Href:"\x00f\x0f\x00\xc2\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\ x00w.appspot.com/124106"}}, Updated:time.Time{sec:63390207737, nsec:0, loc:(*time.Location)(0xc2000f4240)}, Author:xml.Person{Name:"email-address-removed", URI:"", Email:"", InnerXML:"<name>email-address-removed</name>"}, Summary:xml.Text{Type:"html", Body:"\n This fixes the buggy tab rendering that can be seen at\nhttp://codereview.appspot.com/116075/diff/1/2\n\nThefundamental problem was that the tab code was\nnot being told what column the text began in, so it\ndidn't know where to put the tab stops. Another problem\nwas that some of the code assumed that string byte\noffsets were the same as column offsets, which is only\ntrue if there are no tabs.\n\nIn the process of fixing this, I cleaned up the arguments\nto Fold and ExpandTabs and renamed them Break and\n_ExpandTabs so that I could be sure that I found all the\ncall sites. I also wanted to verify that ExpandTabs was\nnot being used from outside intra_region_diff.py.\n\n\n"}}}} want xml.Feed{XMLName:xml.Name{Space:"http://www.w3.org/2005/Atom", Local:"feed"}, Title:"Code Review - My issues", Id:" http://codereview.appspot.com/", Link:[]xml.Link{xml.Link{Rel:"alternate", Href:"http://codereview.appspot.com/"}, xml.Link{Rel:"self", Href:" http://codereview.appspot.com/rss/mine/rsc"}}, Updated:time.Time{sec:63390216958, nsec:0, loc:(*time.Location)(0xc2000881e0)}, Author:xml.Person{Name:"rietveld<>", URI:"", Email:"", InnerXML:"<name>rietveld<></name>"}, Entry:[]xml.Entry{xml.Entry{Title:"rietveld: an attempt at pubsubhubbub\n", Id:"urn:md5:134d9179c41f806be79b3a5f7877d19a", Link:[]xml.Link{xml.Link{Rel:"alternate", Href:" http://codereview.appspot.com/126085"}}, Updated:time.Time{sec:63390216958, nsec:0, loc:(*time.Location)(0xc200088240)}, Author:xml.Person{Name:"email-address-removed", URI:"", Email:"", InnerXML:"<name>email-address-removed</name>"}, Summary:xml.Text{Type:"html", Body:"\n An attempt at adding pubsubhubbub support to Rietveld.\nhttp:// code.google.com/p/pubsubhubbub\nhttp://code.google.com/p/rietveld/issues/detail?id=155\n\nTheserver side of the protocol is trivial:\n 1. add a <link rel="hub" href="hub-server"> tag to all\n feeds that will be pubsubhubbubbed.\n 2. every time one of those feeds changes, tell the hub\n with a simple POST request.\n\nI have tested this by adding debug prints to a local hub\nserver and checking that the server got the right publish\nrequests.\n\nI can't quite get the server to work, but I think the bug\nis not in my code. I think that the server expects to be\nable to grab the feed and see the feed's actual URL in\nthe link rel="self", but the default value for that drops\nthe :port from the URL, and I cannot for the life of me\nfigure out how to get the Atom generator deep inside\ndjango not to do that, or even where it is doing that,\nor even what code is running to generate the Atom feed.\n(I thought I knew but I added some assert False statements\nand it kept running!)\n\nIgnoring that particular problem, I would appreciate\nfeedback on the right way to get the two values at\nthe top of feeds.py marked NOTE(rsc).\n\n\n"}}, xml.Entry{Title:"rietveld: correct tab handling\n", Id:"urn:md5:0a2a4f19bb815101f0ba2904aed7c35a", Link:[]xml.Link{xml.Link{Rel:"alternate", Href:" http://codereview.appspot.com/124106"}}, Updated:time.Time{sec:63390207737, nsec:0, loc:(*time.Location)(0xc2000882a0)}, Author:xml.Person{Name:"email-address-removed", URI:"", Email:"", InnerXML:"<name>email-address-removed</name>"}, Summary:xml.Text{Type:"html", Body:"\n This fixes the buggy tab rendering that can be seen at\nhttp://codereview.appspot.com/116075/diff/1/2\n\nThefundamental problem was that the tab code was\nnot being told what column the text began in, so it\ndidn't know where to put the tab stops. Another problem\nwas that some of the code assumed that string byte\noffsets were the same as column offsets, which is only\ntrue if there are no tabs.\n\nIn the process of fixing this, I cleaned up the arguments\nto Fold and ExpandTabs and renamed them Break and\n_ExpandTabs so that I could be sure that I found all the\ncall sites. I also wanted to verify that ExpandTabs was\nnot being used from outside intra_region_diff.py.\n\n\n"}}}} FAIL FAIL encoding/xml 0.051s maybe you can upload a newer version of your patch here and I can verify if I've made any errors in merging.
Sign in to reply to this message.
On 2012/12/25 07:17:16, minux wrote: > On Mon, Dec 24, 2012 at 4:08 AM, <mailto:0xE2.0x9A.0x9B@gmail.com> wrote: > > > If it also fails with rev 87f67aadaed6 on your machine: Does the failure > > occur randomly, or every time you test encoding/xml? > > > randomly but with a high probability of failure (all about the same > test TestUnmarshalFeed). > > for example: > --- FAIL: TestUnmarshalFeed (0.00 seconds) > read_test.go:21: have xml.Feed{XMLName:xml.Name{Space:" > [cut] > intra_region_diff.py.\n\n\n"}}}} > FAIL > FAIL encoding/xml 0.051s You are right, the test does fail. I am getting the same results when running from command line like this: while true; do ./xml.test; done > maybe you can upload a newer version of your patch here and I can verify if > I've made any errors in merging.
Sign in to reply to this message.
On 2012/12/23 19:26:27, minux wrote: > Hi atom, > after applying the latest patch set (20), and update to rev 7ea16cd2859c, > encoding/xml fails with strange errors on darwin/amd64 (I suspect linux/amd64 > will also fail). Also note that ./all.bash passes on darwin/386 and darwin/arm > on that revision. > the patch works quite well before that rev. > Do you have any ideas why? The cause of the problem is in package "reflect": http://code.google.com/p/go/source/browse/src/pkg/reflect/value.go?r=2c8a88c1... http://code.google.com/p/go/source/browse/src/pkg/reflect/value.go?r=2c8a88c1... The garbage collector found the *[]byte and interpreted its contents as bytes instead of []xml.Link. Fix: https://codereview.appspot.com/7000059/
Sign in to reply to this message.
R=close I believe this CL turned into multiple other CLs, already submitted. https://codereview.appspot.com/6114046/diff/201001/src/pkg/runtime/malloc.goc File src/pkg/runtime/malloc.goc (right): https://codereview.appspot.com/6114046/diff/201001/src/pkg/runtime/malloc.goc... src/pkg/runtime/malloc.goc:493: if(false) *buf = 0; What's this?
Sign in to reply to this message.
|