FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
bytes: avoid duplicate malloc/copy in Buffer.ReadString

Twice faster and twice less garbage.

Please review this at https://codereview.appspot.com/6849128/

Affected files:
M src/pkg/bytes/buffer.go
M src/pkg/bytes/buffer_test.go


Index: src/pkg/bytes/buffer.go
===================================================================
--- a/src/pkg/bytes/buffer.go
+++ b/src/pkg/bytes/buffer.go
@@ -379,8 +379,15 @@
// ReadString returns err != nil if and only if the returned data does not
end
// in delim.
func (b *Buffer) ReadString(delim byte) (line string, err error) {
- bytes, err := b.ReadBytes(delim)
- return string(bytes), err
+ i := IndexByte(b.buf[b.off:], delim)
+ end := b.off + i + 1
+ if i < 0 {
+ end = len(b.buf)
+ err = io.EOF
+ }
+ line = string(b.buf[b.off:end])
+ b.off = end
+ return
}

// NewBuffer creates and initializes a new Buffer using buf as its initial
Index: src/pkg/bytes/buffer_test.go
===================================================================
--- a/src/pkg/bytes/buffer_test.go
+++ b/src/pkg/bytes/buffer_test.go
@@ -375,6 +375,41 @@
}
}

+func TestReadString(t *testing.T) {
+ for _, test := range readBytesTests {
+ buf := NewBufferString(test.buffer)
+ var err error
+ for _, expected := range test.expected {
+ var s string
+ s, err = buf.ReadString(test.delim)
+ if s != expected {
+ t.Errorf("expected %q, got %q", expected, s)
+ }
+ if err != nil {
+ break
+ }
+ }
+ if err != test.err {
+ t.Errorf("expected error %v, got %v", test.err, err)
+ }
+ }
+}
+
+func BenchmarkReadString(b *testing.B) {
+ const n = 32 << 10
+
+ data := make([]byte, n)
+ data[n-1] = 'x'
+ b.SetBytes(int64(n))
+ for i := 0; i < b.N; i++ {
+ buf := NewBuffer(data)
+ _, err := buf.ReadString('x')
+ if err != nil {
+ b.Fatal(err)
+ }
+ }
+}
+
func TestGrow(t *testing.T) {
x := []byte{'x'}
y := []byte{'y'}

Search Discussions

  • Dave at Nov 30, 2012 at 8:19 pm

    On 2012/11/30 19:59:37, remyoudompheng wrote:
    Hello mailto:golang-dev@googlegroups.com (cc:
    mailto:golang-dev@googlegroups.com),
    I'd like you to review this change to
    https://go.googlecode.com/hg/
    Could you post a sample benchcmp ?

    https://codereview.appspot.com/6849128/
  • Daniel Morsing at Nov 30, 2012 at 8:30 pm
    You could pull the slicing code into a new method and have ReadBytes and
    ReadString copy from the resulting slice.

    https://codereview.appspot.com/6849128/
  • Bradfitz at Nov 30, 2012 at 8:35 pm
    https://codereview.appspot.com/6849128/diff/5001/src/pkg/bytes/buffer.go
    File src/pkg/bytes/buffer.go (right):

    https://codereview.appspot.com/6849128/diff/5001/src/pkg/bytes/buffer.go#newcode382
    src/pkg/bytes/buffer.go:382: i := IndexByte(b.buf[b.off:], delim)
    you could also introduce a lowercase readBytes method that's shared
    between ReadBytes and ReadString that returns a private []byte and then
    ReadBytes and ReadString only do the copying (to new []byte or a
    string).

    https://codereview.appspot.com/6849128/
  • Remyoudompheng at Nov 30, 2012 at 9:02 pm

    On 2012/11/30 20:18:32, dfc wrote:
    Could you post a sample benchcmp ?
    benchmark old ns/op new ns/op delta
    BenchmarkReadString 30335 15140 -50.09%

    benchmark old MB/s new MB/s speedup
    BenchmarkReadString 1080.17 2164.25 2.00x

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

    benchcmp doesn't show the comparison of allocated bytes. It's 70kB ->
    37kB in this example.

    https://codereview.appspot.com/6849128/
  • Dave Cheney at Nov 30, 2012 at 9:07 pm
    wow
    On Sat, Dec 1, 2012 at 8:02 AM, wrote:
    On 2012/11/30 20:18:32, dfc wrote:

    Could you post a sample benchcmp ?

    benchmark old ns/op new ns/op delta
    BenchmarkReadString 30335 15140 -50.09%

    benchmark old MB/s new MB/s speedup
    BenchmarkReadString 1080.17 2164.25 2.00x

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

    benchcmp doesn't show the comparison of allocated bytes. It's 70kB ->
    37kB in this example.

    https://codereview.appspot.com/6849128/
  • Remyoudompheng at Nov 30, 2012 at 9:05 pm
    Hello golang-dev@googlegroups.com, dave@cheney.net,
    daniel.morsing@gmail.com, bradfitz@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6849128/
  • Daniel Morsing at Nov 30, 2012 at 9:31 pm
  • Dave Cheney at Nov 30, 2012 at 9:49 pm
    Can ReadBytes just

    return b.readBytes(delim)

    ?

    Dave

    (Phone)
    On 01/12/2012, at 8:31, daniel.morsing@gmail.com wrote:

    LGTM

    https://codereview.appspot.com/6849128/
  • Dave Cheney at Nov 30, 2012 at 10:15 pm
    Sorry,

    return b.readSlice(delim)

    (Rietveld is impossible to use on iPhone)
    On 01/12/ 2012, at 8:49, Dave Cheney wrote:

    Can ReadBytes just

    return b.readBytes(delim)

    ?

    Dave

    (Phone)
    On 01/12/2012, at 8:31, daniel.morsing@gmail.com wrote:

    LGTM

    https://codereview.appspot.com/6849128/
  • Rémy Oudompheng at Nov 30, 2012 at 9:59 pm

    On 2012/11/30 Dave Cheney wrote:
    Sorry,

    return b.readSlice(delim)

    (Rietveld is impossible to use on iPhone)
    No, the bytes returned by readSlice may become invalid after another
    Read. It is not the case of the bytes returned by ReadBytes.

    Rémy.
  • Dave Cheney at Nov 30, 2012 at 10:27 pm
    OK, could you please add a short comment to that effect.
    On 01/12/2012, at 8:52, Rémy Oudompheng wrote:
    On 2012/11/30 Dave Cheney wrote:
    Sorry,

    return b.readSlice(delim)

    (Rietveld is impossible to use on iPhone)
    No, the bytes returned by readSlice may become invalid after another
    Read. It is not the case of the bytes returned by ReadBytes.

    Rémy.
  • Rémy Oudompheng at Nov 30, 2012 at 10:09 pm

    2012/11/30 Dave Cheney <dave@cheney.net>:
    OK, could you please add a short comment to that effect.
    readSlice has already a comment saying that it "returns a reference to
    the buffer's internal data". What would you want to add?

    Rémy.
  • Dave Cheney at Nov 30, 2012 at 10:32 pm
    I think those lines in ReadBytes need a comment to explain the
    unexpected use of append. I'm also a little worried about using append
    this way. I know you and I understand how append works under the hood,
    but there have been cases on the mailing list and IRC where we've told
    people not to do this with append, because they really wanted copy.

    Could you add a test that detects corruption in the []byte returned by
    ReadBytes that would fail if in the future someone refactored
    ReadBytes and removed the append logic, or if, in some other
    implementation of Go, append worked differently ?

    Cheers

    Dave

    On Sat, Dec 1, 2012 at 9:09 AM, Rémy Oudompheng
    wrote:
    2012/11/30 Dave Cheney <dave@cheney.net>:
    OK, could you please add a short comment to that effect.
    readSlice has already a comment saying that it "returns a reference to
    the buffer's internal data". What would you want to add?

    Rémy.
  • Brad Fitzpatrick at Nov 30, 2012 at 11:43 pm

    On Fri, Nov 30, 2012 at 2:32 PM, Dave Cheney wrote:

    I think those lines in ReadBytes need a comment to explain the
    unexpected use of append. I'm also a little worried about using append
    this way. I know you and I understand how append works under the hood,
    but there have been cases on the mailing list and IRC where we've told
    people not to do this with append, because they really wanted copy.
    How would append change at this point? Let's say there's some minor
    ambiguity in the language spec: do you think we'd really go break an
    unknown number of existing Go 1 programs in hard-to-detect (still compiles,
    but slightly different results ways? I really doubt it. I think the spec
    wording would just be tightened.

    I think this use of append is fine. The language spec also guarantees that
    result parameters are zero-initialized.
  • Bradfitz at Nov 30, 2012 at 11:48 pm
    LGTM



    https://codereview.appspot.com/6849128/diff/10001/src/pkg/bytes/buffer.go
    File src/pkg/bytes/buffer.go (right):

    https://codereview.appspot.com/6849128/diff/10001/src/pkg/bytes/buffer.go#newcode368
    src/pkg/bytes/buffer.go:368: // readSlice is like readBytes but returns
    a reference to internal buffer data.
    like ReadBytes

    https://codereview.appspot.com/6849128/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 30, '12 at 7:59p
activeNov 30, '12 at 11:48p
posts16
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase