|
|
|
Created:
11 years, 9 months ago by ruiu Modified:
11 years ago Reviewers:
CC:
golang-codereviews, gobot, rsc, pongad, dave_cheney.net, gri Visibility:
Public. |
bufio: improve ReadFrom
Writer.ReadFrom currently uses underlying writer's
ReadFrom method only when internal buffer is empty.
Also, it does not utilize given Reader's WriteTo
method even if it implements WriterTo. It's not
optional.
This patch is to use underlying value's ReadFrom and
WriteTo methods as long as they are available.
Patch Set 1 #Patch Set 2 : diff -r 3ca54dd30864 https://code.google.com/p/go #Patch Set 3 : diff -r 3ca54dd30864 https://code.google.com/p/go #Patch Set 4 : diff -r 3ca54dd30864 https://code.google.com/p/go #
Total comments: 2
Patch Set 5 : code review 79110043: bufio: improve ReadFrom #Patch Set 6 : diff -r c5f72a685e25 https://code.google.com/p/go #Patch Set 7 : diff -r c5f72a685e25 https://code.google.com/p/go #Patch Set 8 : diff -r 8e05add24c7e https://code.google.com/p/go #Total messages: 21
|
ruiu
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
|
11 years, 9 months ago (2014年03月22日 23:03:17 UTC) #1 |
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
We're in a feature freeze for 1.3. Please ping us after the 1.3 release.
R=rsc@golang.org (assigned by r@golang.org)
Ping.
test, sorry
https://1349-161c4bc39310-tainted-dot-codereview.appspot.com/79110043/diff/60... File src/pkg/bufio/bufio.go (left): https://1349-161c4bc39310-tainted-dot-codereview.appspot.com/79110043/diff/60... src/pkg/bufio/bufio.go:628: m, err = r.Read(b.buf[b.n:]) It seems to me that you could keep all the existing code and just move the buffered == 0 check here, resulting in something like a 5-line diff. You could set first := true before the loop and make the check if first && b.Buffered() == 0 { first = false if w, ok := ... } and that would avoid doing the check each time through the loop. The rewrite in the CL introduces a new helper method that seems to be unnecessary, and despite that, ends up duplicating quite a bit of code. Just moving the check here in the original would not duplicate anything.
another test, sorry
final test for now, sorry
Thank you for reviewing. PTAL. https://codereview.appspot.com/79110043/diff/60001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (left): https://codereview.appspot.com/79110043/diff/60001/src/pkg/bufio/bufio.go#old... src/pkg/bufio/bufio.go:628: m, err = r.Read(b.buf[b.n:]) Good point. This code does not even have to be in the loop. I moved it to the beginning of the function and removed the helper function.
This only partly addresses the issue discussed in https://groups.google.com/forum/#!msg/golang-nuts/oYYggbLntVE/DMsqEyIU-yIJ Though I think if you move the two if statements into the end of the loop similar to rsc's suggestion, it should solve the issue quite handsomely. I am a little worried that we are using from WriteTo and ReadFrom. It seems like if we are not careful reading a bufio.Reader into a bufio.Writer might try to shortcut into each other's ReadTo/WriteFrom and end in infinite recursion. Could you also post some benchmark numbers? I have two new benchmarks in https://codereview.appspot.com/103710044/ that should benchmark for cases discussed in the thread I linked above. Hopefully we will see some improvements. Michael
On 2014年07月03日 04:41:42, pongad wrote: > This only partly addresses the issue discussed in > https://groups.google.com/forum/#!msg/golang-nuts/oYYggbLntVE/DMsqEyIU-yIJ > Though I think if you move the two if statements into the end of the loop > similar to rsc's suggestion, it should solve the issue quite handsomely. Are you suggesting code like this? https://codereview.appspot.com/106410043/diff/20001/src/pkg/bufio/bufio.go > I am a little worried that we are using from WriteTo and ReadFrom. It seems like > if we are not careful reading a bufio.Reader into a bufio.Writer might try to > shortcut into each other's ReadTo/WriteFrom and end in infinite recursion. It wouldn't cause a infinite recursion. On each iteration, ReadFrom or WriteTo methods are called against a bufio's underlying reader, so the recursion will terminate at some point. (If the bufio wraps itself it won't stop, but it's an apparent programmer's error.) > Could you also post some benchmark numbers? I have two new benchmarks in > https://codereview.appspot.com/103710044/ that should benchmark for cases > discussed in the thread I linked above. Hopefully we will see some improvements. See the following result: benchmark old ns/op new ns/op delta ReaderCopyOptimal 182 188 +3.30% ReaderCopyUnoptimal 299 296 -1.00% ReaderCopyNoWriteTo 12491 12453 -0.30% ReaderWriteToOptimal 575 578 +0.52% WriterCopyOptimal 179 180 +0.56% WriterCopyUnoptimal 259 266 +2.70% WriterCopyNoReadFrom 14053 13423 -4.48% WriterReadFromNotEmpty 1835 1301 -29.10% WriterReadFromSmallChunks 64928 67688 +4.25% ReaderEmpty 2490 2479 -0.44% WriterEmpty 2624 2526 -3.73% WriterFlush 19.2 19.4 +1.04%
You are right; my fears are banished. I'm a little surprised that SmallChunks benchmark got worse, though the NotEmpty benchmark looks encouraging. If you want to change the description of the CL to also fix Issue 6373, we will probably be able to shoot two issues with one CL. If that works out, I'll just close my CL.
On 2014年07月05日 23:01:56, pongad wrote: > You are right; my fears are banished. > > I'm a little surprised that SmallChunks benchmark got worse, though the NotEmpty > benchmark looks encouraging. > > If you want to change the description of the CL to also fix Issue 6373, we will > probably be able to shoot two issues with one CL. If that works out, I'll just > close my CL. I'd like to see these test additions in their own CL. I think they are of value. As for this CL, i'm on the fence, ReaderFrom/WriterTo are subtle and there isn't enough justification in this CL to warrant fixing something that isn't demonstrably broken. If you were to add some benchmarks or tests, possibly that count the number of invocations of ReaderFrom/WriteTo rather than just the bytes shoved through them, I think that would be valuable.
R=gri ... since he's expressed concerns about the bufio package most recently.
This seems unrelated to issue 7611 which has been fixed before. At least the CL desc needs to be updated. Also, I'm not sure I understand this change. For one, the test that you added doesn't actually seem to test your changes. If I simply appy the changes to bufio_test.go (w/o the bufio.go changes), the tests still pass. So the test is either not correct or not exposing the problem. Please clarify.
Updated both the CL description and the test. The test now correctly test my change to check if ReadFrom or WriteTo is used as expected. Please take another look.
The test still does not test the change. If I remove the bufio.go changes and run the tests, the tests pass.
R=gri@golang.org (assigned by satish.puranam@gmail.com)
This CL needs to be updated first to match the new directory structure (no pkg anymore in path).
R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/79110043 is best) in the description in your new CL. Thanks very much.