code review 4344060: bufio: Write and WriteString improvements

bufio: Write and WriteString improvements

Write never writes less than the buffer size and WriteString takes advantage
of the copy built-in to improve write efficiency.

[eds, 2011-04-04 06:02:17]

Hello golang-dev@googlegroups.com,

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

[ality, 2011-04-04 09:04:47]

http://codereview.appspot.com/4344060/diff/3/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/4344060/diff/3/src/pkg/bufio/bufio.go#newcode507
src/pkg/bufio/bufio.go:507: return b.Write([]byte(s))
The []byte(s) here will cause an allocation.
I'm not sure if it's worth worrying about.

[eds, 2011-04-04 09:14:05]

On Mon, Apr 4, 2011 at 9:04 PM,  <ality@pbrane.org> wrote:
> The []byte(s) here will cause an allocation.
> I'm not sure if it's worth worrying about.

True, but it will have fewer bounds checks and do at least one less
Flush than the old code. I guess whether it's worth it or not mostly
depends on how expensive the writes are.

I'll leave it as is for now and see what the verdict is.

- Evan

[rog, 2011-04-04 09:58:30]

http://codereview.appspot.com/4344060/diff/3/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/4344060/diff/3/src/pkg/bufio/bufio.go#newcode507
src/pkg/bufio/bufio.go:507: return b.Write([]byte(s))
On 2011/04/04 09:04:48, ality wrote:
> The []byte(s) here will cause an allocation.
> I'm not sure if it's worth worrying about.

i think it's important that this doesn't allocate if possible (for instance when
writing a large string, it will double the amount of memory used).

how about something like this, which also avoids small flushes when the size of
the string is close to the size of the buffer?


// WriteString writes a string.
// It returns the number of bytes written.
// If the count is less than len(s), it also returns an error explaining
// why the write is short.
func (b *Writer) WriteString(s string) (int, os.Error) {
	tot := 0
	// Copy the string and flush piece by piece.
	for len(s) - tot > b.Available() && b.err == nil {
		n := copy(b.buf[b.n:], s[tot:])
		b.n += n
		tot += n
		b.Flush()
	}
	if b.err != nil {
		return tot, b.err
	}
	// Copy the remainder of the string into the buffer.
	b.n += copy(b.buf[b.n:], s[tot:])
	return len(s), nil
}

[rsc, 2011-04-04 12:40:35]

http://codereview.appspot.com/4344060/diff/3/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/4344060/diff/3/src/pkg/bufio/bufio.go#newcode507
src/pkg/bufio/bufio.go:507: return b.Write([]byte(s))
Allocating here is not okay.
Allocation is expensive enough to offset any
other constant factor speedup.

If you really care about large string writes,
then the thing to do is copy the implementation
of Write and drop the "Large write" case.

[rog, 2011-04-04 13:52:51]

On 4 April 2011 13:40,  <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/4344060/diff/3/src/pkg/bufio/bufio.go
> File src/pkg/bufio/bufio.go (right):
>
> http://codereview.appspot.com/4344060/diff/3/src/pkg/bufio/bufio.go#newcode507
> src/pkg/bufio/bufio.go:507: return b.Write([]byte(s))
> Allocating here is not okay.
> Allocation is expensive enough to offset any
> other constant factor speedup.
>
> If you really care about large string writes,
> then the thing to do is copy the implementation
> of Write and drop the "Large write" case.

as you say this, i realise that that's essentially what i had done,
except that the Write method is unnecessarily complex for what it
does.

how about this pair of methods - a simplified Write and a similar
WriteString?

[there is one semantic difference: if you do a Write of a large slice
when there are a few bytes already in the buffer, then it will first
fill the buffer and write it before writing the whole slice.
i think that's an improvement, because it means bufio will never write
any less than bufsize bytes unless explicitly flushed - for clients
wanting to use bufio to minimise the total number of writes it
can reduce the write count by half]

// Write writes the contents of p into the buffer.
// It returns the number of bytes written.
// If nn < len(p), it also returns an error explaining
// why the write is short.
func (b *Writer) Write(p []byte) (int, os.Error) {
	nn := 0
	for len(p) > b.Available() && b.err == nil {
		var n int
		if b.Buffered() == 0 {
			// Large write, empty buffer.
			// Write directly from p to avoid copy.
			n, b.err = b.wr.Write(p)
		} else {
			n = copy(b.buf[b.n:], p)
			b.n += n
			b.Flush()
		}
		nn += n
		p = p[n:]
	}
	if b.err != nil {
		return nn, b.err
	}
	n := copy(b.buf[b.n:], p)
	b.n += n
	nn += n
	return nn, nil
}

// WriteString writes a string.
// It returns the number of bytes written.
// If the count is less than len(s), it also returns an error explaining
// why the write is short.
func (b *Writer) WriteString(s string) (int, os.Error) {
	nn := 0
	// Copy the string and flush piece by piece.
	for len(s) > b.Available() && b.err == nil {
		n := copy(b.buf[b.n:], s)
		b.n += n
		nn += n
		s = s[n:]
		b.Flush()
	}
	if b.err != nil {
		return nn, b.err
	}
	// Copy the remainder of the string into the buffer.
	n := copy(b.buf[b.n:], s)
	b.n += n
	nn += n
	return nn, nil
}

[eds, 2011-04-04 18:26:14]

Hello rsc, ality, rog (cc: golang-dev@googlegroups.com),

Please take another look.

[rog, 2011-04-04 18:49:06]

LGTM, but then it would.

On 4 April 2011 19:26,  <chickencha@gmail.com> wrote:
> Hello rsc, ality, rog (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4344060/
>

[rsc, 2011-04-04 19:11:31]

LGTM

[rsc, 2011-04-04 19:57:11]

*** Submitted as e1efe298340c ***

bufio: Write and WriteString cleanup

Write never writes less than the buffer size and WriteString takes advantage
of the copy built-in to improve write efficiency.

R=rsc, ality, rog
CC=golang-dev
http://codereview.appspot.com/4344060

Committer: Russ Cox <rsc@golang.org>