Issue 6964043: code review 6964043: net/http: buffer before chunking (Closed)
|
Created:
13 years, 1 month ago by bradfitz Modified:
13 years ago Reviewers:
albert.strasheim CC:
golang-dev, dave_cheney.net, minux1, rsc, adg, sanjay.m Visibility:
Public. |
Descriptionnet/http: buffer before chunking
This introduces a buffer between writing from a handler and
writing chunks. Further, it delays writing the header until
the first full chunk is ready. In the case where the first
full chunk is also the final chunk (for small responses), that
means we can also compute a Content-Length, which is a nice
side effect for certain benchmarks.
Fixes issue 2357
Patch Set 1 #Patch Set 2 : diff -r 0f491e663a44 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 0f491e663a44 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 4 : diff -r 0f491e663a44 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 5 : diff -r 74e2affcfe39 https://go.googlecode.com/hg/ #
Total comments: 7
Patch Set 6 : diff -r fe640aeda5f2 https://go.googlecode.com/hg/ #Patch Set 7 : diff -r ec1e87068221 https://go.googlecode.com/hg/ #Patch Set 8 : diff -r ec1e87068221 https://go.googlecode.com/hg/ #
MessagesTotal messages: 31
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.
> the first full chunk is ready. In the case where the first > full chunk is also the final chunk (for small responses), that > means we can also compute a Content-Length, which is a nice > side effect for certain benchmarks. ^ awesome!
Sign in to reply to this message.
On 2012/12/19 02:35:37, brad wrote: > the first full chunk is ready. In the case where the first > full chunk is also the final chunk (for small responses), that > means we can also compute a Content-Length, which is a nice > side effect for certain benchmarks. Any benchmark data available? my tests shows minor performance gain but doubles memory allocation: benchmark old ns/op new ns/op delta BenchmarkServer 141029 140079 -0.67% benchmark old allocs new allocs delta BenchmarkServer 40 47 17.50% benchmark old bytes new bytes delta BenchmarkServer 2277 4666 104.92%
Sign in to reply to this message.
On Wed, Dec 19, 2012 at 9:53 AM, <minux.ma@gmail.com> wrote: > On 2012/12/19 02:35:37, brad wrote: > >> the first full chunk is ready. In the case where the first >> full chunk is also the final chunk (for small responses), that >> means we can also compute a Content-Length, which is a nice >> side effect for certain benchmarks. >> > Any benchmark data available? > > my tests shows minor performance gain but doubles memory allocation: > No benchmark data. The goal of this CL isn't performance. But I think things are simpler now (e.g. the MIME sniffing is in one place, and not sprinkled about), and I think this CL provides a better base to optimize from. The extra allocations are expected: I'm now allocating things which I wasn't before. But they're also trivially avoided by re-using the toConnWriter & buffer. But many such objects could be re-used in the same way. I'd planned to do those in a future CL, to keep this one easier to read. Or do you think this CL should grow to achieve parity first?
Sign in to reply to this message.
On Thu, Dec 20, 2012 at 2:29 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > The goal of this CL isn't performance. > > But I think things are simpler now (e.g. the MIME sniffing is in one > place, and not sprinkled about), and I think this CL provides a better base > to optimize from. > > The extra allocations are expected: I'm now allocating things which I > wasn't before. But they're also trivially avoided by re-using the > toConnWriter & buffer. But many such objects could be re-used in the same > way. I'd planned to do those in a future CL, to keep this one easier to > read. Or do you think this CL should grow to achieve parity first? > Understood. I'm just curios to benchmark this CL, and report my findings. Thank you for your explanations. why we need to deep copy the headers if we don't support trailers (yet)?
Sign in to reply to this message.
On Wed, Dec 19, 2012 at 10:33 AM, minux <minux.ma@gmail.com> wrote: > > On Thu, Dec 20, 2012 at 2:29 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > >> The goal of this CL isn't performance. >> >> But I think things are simpler now (e.g. the MIME sniffing is in one >> place, and not sprinkled about), and I think this CL provides a better base >> to optimize from. >> >> The extra allocations are expected: I'm now allocating things which I >> wasn't before. But they're also trivially avoided by re-using the >> toConnWriter & buffer. But many such objects could be re-used in the same >> way. I'd planned to do those in a future CL, to keep this one easier to >> read. Or do you think this CL should grow to achieve parity first? >> > Understood. I'm just curios to benchmark this CL, and report my findings. > Yeah, that's fine. Please do. > Thank you for your explanations. > why we need to deep copy the headers if we don't support trailers (yet)? > Now that WriteHeader is lazy, I wanted to be conservative and preserve the same semantics: h := rw.Header() h.Set("Content-Type", "text/foo") rw.WriteHeader(200) h.Set("Content-Type", "XXXXXXXXXX") // should be ignored The XXXXXX was previously "ignored" because WriteHeader wrote the header map to the wire (or at least the rwc buf writer) immediately. Now that it's lazy, I want to snapshot the contents of the header map at the time of WriteHeader. Those allocations could also be amortized in a future CL, since the private map clone in toConnWriter and its []string fields never escape the net/http package.
Sign in to reply to this message.
Ping. R=rsc + anybody else On Tue, Dec 18, 2012 at 6:33 PM, <bradfitz@golang.org> wrote: > Reviewers: golang-dev_googlegroups.com, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://go.googlecode.com/hg/ > > > Description: > net/http: buffer before chunking > > This introduces a buffer between writing from a handler and > writing chunks. Further, it delays writing the header until > the first full chunk is ready. In the case where the first > full chunk is also the final chunk (for small responses), that > means we can also compute a Content-Length, which is a nice > side effect for certain benchmarks. > > Fixes issue 2357 > > Please review this at https://codereview.appspot.**com/6964043/<https://codereview.appspot.com/6964... > > Affected files: > M src/pkg/net/http/serve_test.go > M src/pkg/net/http/server.go > > >
Sign in to reply to this message.
I have read through this a few times and I'm not sure I understand it fully. It has a bit of a spaghetti flavor, particularly because the twc reaches into the response conn. Note that I feel a bit lame making this criticism, because the problem is complex and I it's not in my head, so I can't offer a simpler approach. Sorry :/ https://codereview.appspot.com/6964043/diff/5001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/6964043/diff/5001/src/pkg/net/http/server.go#n... src/pkg/net/http/server.go:358: if w.req.Method == "HEAD" || code == StatusNotModified { consider using a switch statement https://codereview.appspot.com/6964043/diff/8001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/6964043/diff/8001/src/pkg/net/http/server.go#n... src/pkg/net/http/server.go:195: func cloneHeader(h Header) Header { this could be func (h Header) clone() Header
Sign in to reply to this message.
https://codereview.appspot.com/6964043/diff/5001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/6964043/diff/5001/src/pkg/net/http/server.go#n... src/pkg/net/http/server.go:358: if w.req.Method == "HEAD" || code == StatusNotModified { On 2013/01/08 05:15:08, adg wrote: > consider using a switch statement This is just code movement. I could try to move things around so the diff looks smaller, at the cost of overall ordering. Or I could reorder it in a follow-up CL?
Sign in to reply to this message.
On 7 January 2013 21:33, <bradfitz@golang.org> wrote: > This is just code movement. I could try to move things around so the > diff looks smaller, at the cost of overall ordering. Or I could reorder > it in a follow-up CL? > It might be nice to avoid the big diff. I saw that it was just code moving, but maybe the reduced diff would help.
Sign in to reply to this message.
https://codereview.appspot.com/6964043/diff/8001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/6964043/diff/8001/src/pkg/net/http/server.go#n... src/pkg/net/http/server.go:195: func cloneHeader(h Header) Header { On 2013/01/08 05:15:08, adg wrote: > this could be > > func (h Header) clone() Header Done, and moved to header.go. Putting it here was lazy.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dave@cheney.net, minux.ma@gmail.com, rsc@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Code is now rearranged so the diff is more readable. I can rearrange it back later. On Tue, Jan 8, 2013 at 11:23 AM, <bradfitz@golang.org> wrote: > Hello golang-dev@googlegroups.com, dave@cheney.net, minux.ma@gmail.com, > rsc@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.**com/6964043/<https://codereview.appspot.com/6964... >
Sign in to reply to this message.
LGTM All these wrapped writers are hard to keep straight in my head, but now I understand what is going on it's quite clear, and seems correct to me. My main request is more documentation about what's going on. Perhaps a higher-level comment that describes the layers from ResponseWriter to the underlying net.Conn would be helpful. https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go#... src/pkg/net/http/server.go:199: // toConnWriter writes to a response's conn buffer, and is the writer that's not all it does. can you expand this comment a little? https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go#... src/pkg/net/http/server.go:206: // Set after header is written: perhaps "set by the writeHeader method" ? https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go#... src/pkg/net/http/server.go:471: log.Printf("http: invalid Content-Length of %q sent", cl) sent or specified? does it actually get sent? https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go#... src/pkg/net/http/server.go:485: func (cw *toConnWriter) writeHeader(p []byte) { i feel this should be named 'tcw' to reflect the field name, so you know they're one and the same https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go#... src/pkg/net/http/server.go:492: code := cw.res.status w.status https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go#... src/pkg/net/http/server.go:558: cw.header.Set("Content-Type", DetectContentType(p)) nice
Sign in to reply to this message.
drive-by typo... https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/6964043/diff/17001/src/pkg/net/http/server.go#... src/pkg/net/http/server.go:283: handlerDone bool // set true when the handler exists s/exists/exits/
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dave@cheney.net, minux.ma@gmail.com, rsc@golang.org, adg@golang.org, balasanjay@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
All done, and lots of comments and TODOs added. PTAL. On Wed, Jan 9, 2013 at 4:46 PM, <bradfitz@golang.org> wrote: > Hello golang-dev@googlegroups.com, dave@cheney.net, minux.ma@gmail.com, > rsc@golang.org, adg@golang.org, balasanjay@gmail.com (cc: > > golang-dev@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.**com/6964043/<https://codereview.appspot.com/6964... >
Sign in to reply to this message.
Andrew, What about the name "chunkWriter" instead of "toConnWriter"? It is the writer that writes chunks and the chunk headers now. It just also happens to the header fixups also. Naming where it is in the pipeline is weird, and naming all the things it does is also kinda weird. The big comment should make it easier to understand now without having to use the type name to describe its position. Thoughts? On Wed, Jan 9, 2013 at 4:48 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > All done, and lots of comments and TODOs added. PTAL. > > > On Wed, Jan 9, 2013 at 4:46 PM, <bradfitz@golang.org> wrote: > >> Hello golang-dev@googlegroups.com, dave@cheney.net, minux.ma@gmail.com, >> rsc@golang.org, adg@golang.org, balasanjay@gmail.com (cc: >> >> golang-dev@googlegroups.com), >> >> Please take another look. >> >> >> https://codereview.appspot.**com/6964043/<https://codereview.appspot.com/6964... >> > >
Sign in to reply to this message.
On 11 January 2013 12:51, Brad Fitzpatrick <bradfitz@golang.org> wrote: > What about the name "chunkWriter" instead of "toConnWriter"? It is the > writer that writes chunks and the chunk headers now. It just also happens > to the header fixups also. Naming where it is in the pipeline is weird, > and naming all the things it does is also kinda weird. > I agree. chunkWriter is much more appropriate.
Sign in to reply to this message.
On Thu, Jan 10, 2013 at 6:55 PM, Andrew Gerrand <adg@golang.org> wrote: > > On 11 January 2013 12:51, Brad Fitzpatrick <bradfitz@golang.org> wrote: > >> What about the name "chunkWriter" instead of "toConnWriter"? It is the >> writer that writes chunks and the chunk headers now. It just also happens >> to the header fixups also. Naming where it is in the pipeline is weird, >> and naming all the things it does is also kinda weird. >> > > I agree. chunkWriter is much more appropriate. > Renamed. Submitting.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=fc9545bf5a1f *** net/http: buffer before chunking This introduces a buffer between writing from a handler and writing chunks. Further, it delays writing the header until the first full chunk is ready. In the case where the first full chunk is also the final chunk (for small responses), that means we can also compute a Content-Length, which is a nice side effect for certain benchmarks. Fixes issue 2357 R=golang-dev, dave, minux.ma, rsc, adg, balasanjay CC=golang-dev https://codereview.appspot.com/6964043
Sign in to reply to this message.
Message was sent while issue was closed.
Howdy There seems to be something wrong according to the torture box: FAIL: TestServerBufferedChunking-10 at serve_test.go:1165 response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Mon, 14 Jan 2013 08:24:10 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" Looks a bit like this stuff from a while ago, but maybe that's just a coincidence. http://code.google.com/p/go/issues/detail?id=4021 Cheers Albert
Sign in to reply to this message.
This CL changed nothing about concurrency. It did, however, change chunking. But the tests were adjusted. Is this a sporadic failure? How to reproduce? I have machines with over 10 cores. On Mon, Jan 14, 2013 at 12:30 AM, <fullung@gmail.com> wrote: > Howdy > > There seems to be something wrong according to the torture box: > > FAIL: TestServerBufferedChunking-10 at serve_test.go:1165 > > response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 > OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Mon, 14 Jan 2013 > 08:24:10 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > > Looks a bit like this stuff from a while ago, but maybe that's just a > coincidence. > > http://code.google.com/p/go/**issues/detail?id=4021<http://code.google.com/p/... > > Cheers > > Albert > > https://codereview.appspot.**com/6964043/<https://codereview.appspot.com/6964... >
Sign in to reply to this message.
Hello On Tue, Jan 15, 2013 at 3:02 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > This CL changed nothing about concurrency. > It did, however, change chunking. But the tests were adjusted. > Is this a sporadic failure? How to reproduce? I have machines with over 10 > cores. Xeon E5-2670 0 @ 2.60GHz, performance governor, F16 64-bit kernel 3.3.8. Test always passes with GOMAXPROCS=1 and fails about 80% of the time with GOMAXPROCS=2 or greater. Here's a few runs: $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking --- PASS: TestServerBufferedChunking (0.00 seconds) PASS $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) serve_test.go:1165: response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:27 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" FAIL $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- PASS: TestServerBufferedChunking-2 (0.00 seconds) PASS $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking --- PASS: TestServerBufferedChunking (0.00 seconds) PASS $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- PASS: TestServerBufferedChunking-2 (0.00 seconds) PASS $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) serve_test.go:1165: response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:40 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" FAIL $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) serve_test.go:1165: response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:41 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" FAIL $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- PASS: TestServerBufferedChunking-2 (0.00 seconds) PASS $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- PASS: TestServerBufferedChunking-2 (0.00 seconds) PASS $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) serve_test.go:1165: response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" FAIL $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) serve_test.go:1165: response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" FAIL $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) serve_test.go:1165: response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" FAIL $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking === RUN TestServerBufferedChunking-2 --- PASS: TestServerBufferedChunking-2 (0.00 seconds) PASS Regards Albert
Sign in to reply to this message.
I know it is a big ask, but it would be a very useful data point for me if you were able to test this under another Linux distro on this hardware. It would be sufficient to do this from a live cd using a prebuilt binary from go test -c. Is that an option ? On 15/01/2013, at 20:16, Albert Strasheim <fullung@gmail.com> wrote: > Hello > > On Tue, Jan 15, 2013 at 3:02 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: >> This CL changed nothing about concurrency. >> It did, however, change chunking. But the tests were adjusted. >> Is this a sporadic failure? How to reproduce? I have machines with over 10 >> cores. > > Xeon E5-2670 0 @ 2.60GHz, performance governor, F16 64-bit kernel 3.3.8. > > Test always passes with GOMAXPROCS=1 and fails about 80% of the time > with GOMAXPROCS=2 or greater. > > Here's a few runs: > > $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking > --- PASS: TestServerBufferedChunking (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:27 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking > --- PASS: TestServerBufferedChunking (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:40 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:41 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > > Regards > > Albert
Sign in to reply to this message.
Hello On Tue, Jan 15, 2013 at 11:21 AM, Dave Cheney <dave@cheney.net> wrote: > I know it is a big ask, but it would be a very useful data point for me if you were able to test this under another Linux distro on this hardware. It would be sufficient to do this from a live cd using a prebuilt binary from go test -c. > Is that an option ? This Xeon is in use by multiple people, so that's a bit tricky. I can also reproduce this on F17 on a i7-3720QM running 3.6.3-1.fc17.x86_64. I'll fire up an Ubuntu live CD to see what I see. Cheers Albert
Sign in to reply to this message.
Thanks, that would be a very useful datapoint. On Tue, Jan 15, 2013 at 8:36 PM, Albert Strasheim <fullung@gmail.com> wrote: > Hello > > On Tue, Jan 15, 2013 at 11:21 AM, Dave Cheney <dave@cheney.net> wrote: >> I know it is a big ask, but it would be a very useful data point for me if you were able to test this under another Linux distro on this hardware. It would be sufficient to do this from a live cd using a prebuilt binary from go test -c. >> Is that an option ? > > This Xeon is in use by multiple people, so that's a bit tricky. > > I can also reproduce this on F17 on a i7-3720QM running > 3.6.3-1.fc17.x86_64. I'll fire up an Ubuntu live CD to see what I see. > > Cheers > > Albert
Sign in to reply to this message.
Reproduced on my laptop with GOMAXPROCS=4. Fix is https://codereview.appspot.com/7109043 On Tue, Jan 15, 2013 at 1:16 AM, Albert Strasheim <fullung@gmail.com> wrote: > Hello > > On Tue, Jan 15, 2013 at 3:02 AM, Brad Fitzpatrick <bradfitz@golang.org> > wrote: > > This CL changed nothing about concurrency. > > It did, however, change chunking. But the tests were adjusted. > > Is this a sporadic failure? How to reproduce? I have machines with > over 10 > > cores. > > Xeon E5-2670 0 @ 2.60GHz, performance governor, F16 64-bit kernel 3.3.8. > > Test always passes with GOMAXPROCS=1 and fails about 80% of the time > with GOMAXPROCS=2 or greater. > > Here's a few runs: > > $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking > --- PASS: TestServerBufferedChunking (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:27 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking > --- PASS: TestServerBufferedChunking (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:40 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:41 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) > serve_test.go:1165: response didn't end with a single 3 byte 'xyz' > chunk; got: > "HTTP/1.1 200 OK\r\nContent-Type: text/plain; > charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 > GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > FAIL > $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking > === RUN TestServerBufferedChunking-2 > --- PASS: TestServerBufferedChunking-2 (0.00 seconds) > PASS > > Regards > > Albert >
Sign in to reply to this message.
Btw, we really should have at least one GOMAXPROCS >1 builder on the dashboard. But thanks again for being our dashboard in the meantime and finding all these. On Mon, Jan 14, 2013 at 12:30 AM, <fullung@gmail.com> wrote: > Howdy > > There seems to be something wrong according to the torture box: > > FAIL: TestServerBufferedChunking-10 at serve_test.go:1165 > > response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 > OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Mon, 14 Jan 2013 > 08:24:10 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" > > Looks a bit like this stuff from a while ago, but maybe that's just a > coincidence. > > http://code.google.com/p/go/**issues/detail?id=4021<http://code.google.com/p/... > > Cheers > > Albert > > https://codereview.appspot.**com/6964043/<https://codereview.appspot.com/6964... >
Sign in to reply to this message.
I set my Darwin builders to GOMAXPROCS=4 yesterday. On 16/01/2013, at 2:09, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Btw, we really should have at least one GOMAXPROCS >1 builder on the dashboard. But thanks again for being our dashboard in the meantime and finding all these. > > On Mon, Jan 14, 2013 at 12:30 AM, <fullung@gmail.com> wrote: >> Howdy >> >> There seems to be something wrong according to the torture box: >> >> FAIL: TestServerBufferedChunking-10 at serve_test.go:1165 >> >> response didn't end with a single 3 byte 'xyz' chunk; got: "HTTP/1.1 200 >> OK\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Mon, 14 Jan 2013 >> 08:24:10 GMT\r\nTransfer-Encoding: chunked\r\n\r\n" >> >> Looks a bit like this stuff from a while ago, but maybe that's just a >> coincidence. >> >> http://code.google.com/p/go/issues/detail?id=4021 >> >> Cheers >> >> Albert >> >> https://codereview.appspot.com/6964043/ >
Sign in to reply to this message.
Great catch Brad! Albert, thank you for being so persistent with this issue. I'm sorry that I thought this was some weird Fedora kernel issue. At least for every failure we find and fix we gain more confidence that the next failure reported _is_ real. On Wed, Jan 16, 2013 at 2:06 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Reproduced on my laptop with GOMAXPROCS=4. > > Fix is https://codereview.appspot.com/7109043 > > > On Tue, Jan 15, 2013 at 1:16 AM, Albert Strasheim <fullung@gmail.com> wrote: >> >> Hello >> >> On Tue, Jan 15, 2013 at 3:02 AM, Brad Fitzpatrick <bradfitz@golang.org> >> wrote: >> > This CL changed nothing about concurrency. >> > It did, however, change chunking. But the tests were adjusted. >> > Is this a sporadic failure? How to reproduce? I have machines with >> > over 10 >> > cores. >> >> Xeon E5-2670 0 @ 2.60GHz, performance governor, F16 64-bit kernel 3.3.8. >> >> Test always passes with GOMAXPROCS=1 and fails about 80% of the time >> with GOMAXPROCS=2 or greater. >> >> Here's a few runs: >> >> $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking >> --- PASS: TestServerBufferedChunking (0.00 seconds) >> PASS >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) >> serve_test.go:1165: response didn't end with a single 3 byte 'xyz' >> chunk; got: >> "HTTP/1.1 200 OK\r\nContent-Type: text/plain; >> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:27 >> GMT\r\nTransfer-Encoding: chunked\r\n\r\n" >> FAIL >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- PASS: TestServerBufferedChunking-2 (0.00 seconds) >> PASS >> $ GOMAXPROCS=1 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking >> --- PASS: TestServerBufferedChunking (0.00 seconds) >> PASS >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- PASS: TestServerBufferedChunking-2 (0.00 seconds) >> PASS >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) >> serve_test.go:1165: response didn't end with a single 3 byte 'xyz' >> chunk; got: >> "HTTP/1.1 200 OK\r\nContent-Type: text/plain; >> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:40 >> GMT\r\nTransfer-Encoding: chunked\r\n\r\n" >> FAIL >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) >> serve_test.go:1165: response didn't end with a single 3 byte 'xyz' >> chunk; got: >> "HTTP/1.1 200 OK\r\nContent-Type: text/plain; >> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:41 >> GMT\r\nTransfer-Encoding: chunked\r\n\r\n" >> FAIL >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- PASS: TestServerBufferedChunking-2 (0.00 seconds) >> PASS >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- PASS: TestServerBufferedChunking-2 (0.00 seconds) >> PASS >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) >> serve_test.go:1165: response didn't end with a single 3 byte 'xyz' >> chunk; got: >> "HTTP/1.1 200 OK\r\nContent-Type: text/plain; >> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 >> GMT\r\nTransfer-Encoding: chunked\r\n\r\n" >> FAIL >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) >> serve_test.go:1165: response didn't end with a single 3 byte 'xyz' >> chunk; got: >> "HTTP/1.1 200 OK\r\nContent-Type: text/plain; >> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 >> GMT\r\nTransfer-Encoding: chunked\r\n\r\n" >> FAIL >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- FAIL: TestServerBufferedChunking-2 (0.00 seconds) >> serve_test.go:1165: response didn't end with a single 3 byte 'xyz' >> chunk; got: >> "HTTP/1.1 200 OK\r\nContent-Type: text/plain; >> charset=utf-8\r\nDate: Tue, 15 Jan 2013 09:12:43 >> GMT\r\nTransfer-Encoding: chunked\r\n\r\n" >> FAIL >> $ GOMAXPROCS=2 ./http.test -test.v -test.run=TestServerBufferedChunking >> === RUN TestServerBufferedChunking-2 >> --- PASS: TestServerBufferedChunking-2 (0.00 seconds) >> PASS >> >> Regards >> >> Albert > >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
