Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2728)

Issue 8819049: code review 8819049: bufio: make Reader buffer transient (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by bradfitz
Modified:
10 years, 10 months ago
Reviewers:
r
CC:
r, adg, dvyukov, gobot, golang-dev, rog
Visibility:
Public.

Description

bufio: make Reader buffer transient Share garbage between different bufio Readers. When a Reader has zero buffered data, put its buffer into a pool. This acknowledges that most bufio.Readers eventually get read to completion, and their buffers are then no longer needed. benchmark old ns/op new ns/op delta BenchmarkReaderEmpty 2993 1058 -64.65% benchmark old allocs new allocs delta BenchmarkReaderEmpty 3 2 -33.33% benchmark old bytes new bytes delta BenchmarkReaderEmpty 4278 133 -96.89% Update Issue 5100

Patch Set 1 #

Patch Set 2 : diff -r 2489327864d7 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 2489327864d7 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 30c566874b83 https://go.googlecode.com/hg/ #

Total comments: 6

Patch Set 5 : diff -r 8f1fb6b6f141 https://go.googlecode.com/hg/ #

Total comments: 12

Patch Set 6 : diff -r 4cb93e2900d0 https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r 43b3233f0b5b https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -7 lines) Patch
M src/pkg/bufio/bufio.go View 1 2 3 4 5 6 11 chunks +56 lines, -7 lines 0 comments Download
M src/pkg/bufio/bufio_test.go View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download

Messages

Total messages: 13
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
10 years, 10 months ago (2013-05-12 23:26:53 UTC) #1
adg
https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newcode193 src/pkg/bufio/bufio.go:193: if b.r == b.w { I think you can ...
10 years, 10 months ago (2013-05-12 23:55:23 UTC) #2
adg
https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newcode79 src/pkg/bufio/bufio.go:79: for i := range b.buf { why zero this?
10 years, 10 months ago (2013-05-12 23:57:53 UTC) #3
dvyukov
https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newcode79 src/pkg/bufio/bufio.go:79: for i := range b.buf { On 2013/05/12 23:57:53, ...
10 years, 10 months ago (2013-05-13 04:00:32 UTC) #4
bradfitz
https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newcode79 src/pkg/bufio/bufio.go:79: for i := range b.buf { On 2013/05/13 04:00:32, ...
10 years, 10 months ago (2013-05-13 04:07:59 UTC) #5
rog
https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/8819049/diff/8001/src/pkg/bufio/bufio.go#newcode78 src/pkg/bufio/bufio.go:78: case b.buf = <-bufCache: is this right when b.bufSize ...
10 years, 10 months ago (2013-05-14 16:00:33 UTC) #6
bradfitz
Hello golang-dev@googlegroups.com, adg@golang.org, dvyukov@google.com, rogpeppe@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 10 months ago (2013-05-14 16:19:27 UTC) #7
bradfitz
Good catch! PTAL. On Tue, May 14, 2013 at 9:00 AM, <rogpeppe@gmail.com> wrote: > > ...
10 years, 10 months ago (2013-05-14 16:19:35 UTC) #8
gobot
R=r (assigned by bradfitz)
10 years, 10 months ago (2013-05-14 22:14:36 UTC) #9
r
https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newcode61 src/pkg/bufio/bufio.go:61: if size != defaultBufSize { why not if size ...
10 years, 10 months ago (2013-05-15 19:48:35 UTC) #10
bradfitz
Done. PTAL. The buffer is only recycled on EOF now. https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/8819049/diff/18001/src/pkg/bufio/bufio.go#newcode61 ...
10 years, 10 months ago (2013-05-17 21:31:31 UTC) #11
r
LGTM
10 years, 10 months ago (2013-05-17 21:50:30 UTC) #12
bradfitz
10 years, 10 months ago (2013-05-17 22:16:14 UTC) #13
*** Submitted as https://code.google.com/p/go/source/detail?r=bce231eb0fdd ***

bufio: make Reader buffer transient

Share garbage between different bufio Readers. When a Reader
has zero buffered data, put its buffer into a pool.

This acknowledges that most bufio.Readers eventually get
read to completion, and their buffers are then no longer
needed.

benchmark               old ns/op    new ns/op    delta
BenchmarkReaderEmpty         2993         1058  -64.65%

benchmark              old allocs   new allocs    delta
BenchmarkReaderEmpty            3            2  -33.33%

benchmark               old bytes    new bytes    delta
BenchmarkReaderEmpty         4278          133  -96.89%

Update Issue 5100

R=r
CC=adg, dvyukov, gobot, golang-dev, rogpeppe
https://codereview.appspot.com/8819049
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b