strings: optimize WriteTo to use an intermediate buffer for large strings #13848

dsnet · 2016-01-07T00:16:40Z

Using go1.5

NOTE: This issue used to be about using an intermediate buffer in io.WriteString, but will instead perform the optimization in strings.Reader.WriteTo instead. The description in this issue still refers to WriteString, but the performance numbers will probably be the same once applied to strings.Reader.WriteTo.

Currently WriteString does w.Write([]byte(s)) if w is not a stringWriter. This causes a memory allocation proportional to len(s). Instead, we should use an intermediate buffer for large strings.

Using this test code:

var (
    large = strings.Repeat("the quick brown fox jumped over the lazy dog", 1024*1024)
    writerOnly = struct{ io.Writer }{ioutil.Discard}
)

// WriteString2 is modified from io.WriteString to use an
// intermediate buffer for large strings.
func WriteString2(w io.Writer, s string) (n int, err error) {
    const chunkSize = 32*1024
    if sw, ok := w.(interface {WriteString(s string) (n int, err error)}); ok {
        return sw.WriteString(s)
    }
    if len(s) < chunkSize {
        return w.Write([]byte(s))
    }

    buf := make([]byte, chunkSize)
    for len(s) > 0 {
        cnt := copy(buf, s)
        s = s[cnt:]
        cnt, err = w.Write(buf[:cnt])
        n += cnt
        if err != nil {
            break
        }
    }
    return n, err
}

func BenchmarkWriteString(b *testing.B) {
    b.ReportAllocs()
    for i := 0; i < b.N; i++ {
        if _, err := io.WriteString(writerOnly, large); err != nil {
            b.Error(err)
        }
    }
}

func BenchmarkWriteString2(b *testing.B) {
    b.ReportAllocs()
    for i := 0; i < b.N; i++ {
        if _, err := WriteString2(writerOnly, large); err != nil {
            b.Error(err)
        }
    }
}

We can get the number of bytes allocated to be capped at some maximum:

BenchmarkWriteString-4       200       6482427 ns/op    46137405 B/op          2 allocs/op
BenchmarkWriteString2-4      500       2437841 ns/op       32784 B/op          2 allocs/op

I was pleasantly surprised that the runtime also decreased, but this may be because the small buffer fits entirely in the cache.

The text was updated successfully, but these errors were encountered:

bradfitz · 2016-01-07T00:20:31Z

I don't think we can do this for compatibility reasons. I think we're stuck with the implicit guarantee that 1 WriteString call means 1 Write call.

davecheney · 2016-01-07T00:26:58Z

Is it possible to address the types that don't have WriteString instead?

On Thu, 7 Jan 2016, 11:20 Brad Fitzpatrick notifications@github.com wrote:

I don't think we can do this for compatibility reasons. I think we're
stuck with the implicit guarantee that 1 WriteString call means 1 Write
call.

—
Reply to this email directly or view it on GitHub
#13848 (comment).

dsnet · 2016-01-07T00:29:44Z

I don't think we can do this for compatibility reasons. I think we're stuck with the implicit guarantee that 1 WriteString call means 1 Write call.

Where did this implicit guarantee come from?

The documentation for WriteString is:

// WriteString writes the contents of the string s to w, which accepts a slice of bytes.
// If w implements a WriteString method, it is invoked directly.

If this guarantee does exist, it seems important enough that it should be documented.

EDIT:
Also, wouldn't a user just do w.Write([]byte(s)) directly if they depended on that behavior?

dsnet · 2016-01-07T00:29:49Z

Is it possible to address the types that don't have WriteString instead?

My test implementation does exactly that; it only does intermediate buffering for non stringWriters.

davecheney · 2016-01-07T00:33:46Z

I meant add WriteString to the types that you need.

On Thu, 7 Jan 2016, 11:29 Joe Tsai notifications@github.com wrote:

Is it possible to address the types that don't have WriteString instead?

My test implementation does exactly that; it only does intermediate
buffering for non stringWriters.

—
Reply to this email directly or view it on GitHub
#13848 (comment).

bradfitz · 2016-01-07T00:35:56Z

Where did this implicit guarantee come from?

It's implicit because it's not written down. If it were documented, it would be explicit. :-)

As some sort of precedent,

https://golang.org/pkg/log/#Logger says "Each logging operation makes a single call to the Writer's Write method"

https://golang.org/pkg/fmt/#Fprint and friends don't document a single Write call, but they do guarantee it.

But yes, perhaps we could document it.

dsnet · 2016-01-07T00:52:35Z

I see. I can definitely see how the single Write is important in console applications and possibly others.

I filed this ticket because I noticed large allocations due to writings strings and when I saw io.WriteString, my first thought was "oh sweet, here's a library method that'll efficiently write a string for me!" Alas, I was wrong.

In theory, another way to do what I wanted would be:

io.Copy(w, strings.NewReader(s))

But... that doesn't work well either, since strings.Reader just calls io.WriteString, and we're back to the same problem.

What are your thoughts on doing the following:

Add the chunking optimization to strings.Reader.WriteTo. Given that WriteTo was developed for io.Copy, which does chunking, I don't think there is any expectation that it performs a single Write.
Filing a separate issue to document that WriteString guarantees at most one call to Write.

bradfitz · 2016-01-07T00:54:55Z

Both SGTM

gopherbot · 2016-02-20T05:00:45Z

CL https://golang.org/cl/19754 mentions this issue.

bradfitz · 2016-02-20T06:05:05Z

After seeing https://golang.org/cl/19754 I'm not really a fan of this. I left some comments there, but let's discuss here.

Unfortunately I don't have a proposal other than just not doing anything.

dsnet · 2016-02-20T07:40:00Z

When performance optimizing a write of a large string without WriteString, this may be a reasonable set of solutions:

io.WriteString(w, s)
This doesn't work since io.WriteString calls Write([]byte(s)) once as well and as discussed above. Once the implicit single Write is documented, people will know this isn't what they want.
io.Copy(w, strings.NewReader(s))
First, io.Copy is a fairly reasonable function to use in this scenario, since most people instinctively know that Copy does some type of chunked transfer. Second, strings.Reader is the probably the most sensible type to use to convert a string to an io.Reader. Thus, it's the perfect place to make the change. Arguably, someone could use bytes.NewBufferString(s), but I don't think use of that is popular and certainly wouldn't be the clearer choice in this scenario. Thus, it is unlikely that we will have to make this optimization to other implementors of io.WriterTo given the clear choice to use strings.Reader in this situation. I believe that's the concern in the CL.
Add w.WriteString to types that need it
Reality is that many writers do not support WriteString (zip.Writer, tar.Writer, flate.Writer, gzip.Writer, lzw.Writer, etc...) and I don't think it's reasonable to expect all writers to have WriteString as well. The implementation of which may not be under your control. Plus, I'm not a fan of the expansion of the API for Writers with WriteString.
Allocate a buffer and chunk it yourself (few lines or so)
Not a huge deal if people just chunk stuff themselves. I'm okay with this as well.

OneOfOne · 2016-02-20T14:40:18Z

I submitted a different PR, for something that I personally use in my code, it shows a major improvement for io.WriteString, I did a quick search and couldn't find anything against using unsafe in the stdlib.

➜ benchcmp  /tmp/old.txt /tmp/new.txt 
benchmark                       old ns/op     new ns/op     delta
BenchmarkWriteStringLong-8      5219513       65.9          -100.00%  # long string is 44 * 1024 * 1024 bytes.
BenchmarkWriteStringShort-8     112           65.7          -41.34%   # short is 44 bytes.

gopherbot · 2016-02-20T15:00:32Z

CL https://golang.org/cl/19722 mentions this issue.

dsnet · 2016-02-20T16:26:53Z

Copy of my comment CL/19722 (which uses unsafe to directly cast a string as a []byte in io.WriteString):

As much as I love seeing performance improve, I object against this because it breaks memory safety. In an ideal world, it probably would be okay to directly convert a string to a []byte. However, this CL relies on the fact that every implementation of io.Writer perfectly obeys the documented rules that the Writer never mutates the input slice. Reality is, I have seen buggy Writers that did alter the input for short period (yes, that's wrong), but this change would make the presence of those types of Writers worse. I certainly don't think this should be done at the level of the standard level.

I am aware of this method used in other packages, like CockroadDB. But, at least in their use, they have a firm understanding that the concrete underlying method does not alter the input. However, io.WriteString cannot make that assumption safely.

As a secondary argument against it, there's a warning in the reflect package against the use of StringHeader or SliceHeader for portability reason. I think it be wise to respect those warnings.

bradfitz · 2016-02-20T17:10:02Z

@dsnet, if io.Copy doesn't work for you,

io.Copy(w, strings.NewReader(s))

What about:

   io.Copy(w, struct { io.Reader }{ strings.NewReader(s) })

That should work, no?

dsnet · 2016-04-12T01:20:16Z

@bradfitz

That would work, but there are some detriments:

If w.WriteString is implemented, then it is not used, which is unfortunate.
If w.ReadFrom is not implemented, this always allocates 32KiB, even if s is very short.

In regard to your comments on CL/19754:

The benefit of the special casing is that there is 2x speed boost (probably due to better caching), and a guaranteed bound on the memory allocation (i.e. never greater than 32KiB).
Also, I don't think this optimization will end up appearing in all io.WriterTo since strings.Reader is the way to convert a string to an io.Reader. Other readers typically represent a buffer under-the-hood as a []byte, in which case, string writing isn't an issue.

ianlancetaylor added this to the Go1.7 milestone Jan 7, 2016

dsnet changed the title ~~io: optimize WriteString to use an intermediate buffer for large strings~~ strings: optimize WriteTo to use an intermediate buffer for large strings Jan 7, 2016

dsnet mentioned this issue Jan 7, 2016

io: document that WriteString only calls Write once #13849

Closed

dsnet modified the milestones: Unplanned, Go1.7 May 17, 2016

dsnet mentioned this issue Aug 2, 2016

proposal: io: CopyBuffer should avoid ReadFrom/WriteTo #16474

Open

dsnet added the Performance label Oct 4, 2016

dsnet mentioned this issue Jan 27, 2017

cmd/compile: optimize Write([]byte(stringVal)) to not copy the string #18822

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

strings: optimize WriteTo to use an intermediate buffer for large strings #13848

strings: optimize WriteTo to use an intermediate buffer for large strings #13848

dsnet commented Jan 7, 2016

bradfitz commented Jan 7, 2016

davecheney commented Jan 7, 2016

dsnet commented Jan 7, 2016

dsnet commented Jan 7, 2016

davecheney commented Jan 7, 2016

bradfitz commented Jan 7, 2016

dsnet commented Jan 7, 2016

bradfitz commented Jan 7, 2016

gopherbot commented Feb 20, 2016

bradfitz commented Feb 20, 2016

dsnet commented Feb 20, 2016

OneOfOne commented Feb 20, 2016

gopherbot commented Feb 20, 2016

dsnet commented Feb 20, 2016

bradfitz commented Feb 20, 2016

dsnet commented Apr 12, 2016

strings: optimize WriteTo to use an intermediate buffer for large strings #13848

strings: optimize WriteTo to use an intermediate buffer for large strings #13848

Comments

dsnet commented Jan 7, 2016

bradfitz commented Jan 7, 2016

davecheney commented Jan 7, 2016

dsnet commented Jan 7, 2016

dsnet commented Jan 7, 2016

davecheney commented Jan 7, 2016

bradfitz commented Jan 7, 2016

dsnet commented Jan 7, 2016

bradfitz commented Jan 7, 2016

gopherbot commented Feb 20, 2016

bradfitz commented Feb 20, 2016

dsnet commented Feb 20, 2016

OneOfOne commented Feb 20, 2016

gopherbot commented Feb 20, 2016

dsnet commented Feb 20, 2016

bradfitz commented Feb 20, 2016

dsnet commented Apr 12, 2016