io: ReadAll buffer resizing is inefficient #50774
Comments
mvdan
added
Performance
NeedsInvestigation
ianlancetaylor
changed the title
This comment has been minimized.
This comment has been minimized.
randall77
changed the title
|
Is the underlying question here if append should resize x2? It feels as if „memory-growing slice-like structures“ should potentially behave similarly throughout Go? |
|
|
|
Consistency is a fine thing, but I would expect blogs, books, etc., to advise “never call Stretching my imagination, some cases can avoid memory-growing: for instance when reading the body of an |
|
We could maybe make func ReadAll(r Reader) ([]byte, error) {
sw := new(sliceWriter)
_, err := Copy(sw, r)
return *sw, err
}
type sliceWriter []byte
func (sw *sliceWriter) Write(b []byte) (int, error) {
*sw = append(*sw, b...)
return len(b), nil
}This would also add support for the Alternatively, if forwarding to Footnotes
|
|
@CAFxX can you clarify how this helps with the resizing of the destination buffer? |
|
@bboreham sure, it boils down to io.Copy being able to optimize the copy by forwarding to io.ReaderFrom on the destination io.Writer, or to io.WriterTo on the source io.Reader. In this specific case, the source io.Reader implements io.WriterTo, so the copy is optimized - and this completely sidesteps the resizing issue in the "reading the whole response body" case. Incidentally, this is almost the same thing that you did manually by calling bytes.Buffer.ReadFrom. Benchmark source codepackage issue50774
import (
"bytes"
"io"
"net/http"
"net/http/httptest"
"testing"
)
func benchmark(b *testing.B, readall func(io.Reader) ([]byte, error)) {
body := bytes.Repeat([]byte("x"), 1<<16)
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write(body)
}))
defer s.Close()
var buf []byte
for i := 0; i < b.N; i++ {
res, err := http.Get(s.URL)
if err != nil {
b.Fatal(err)
}
buf, err = readall(res.Body)
if err != nil {
b.Fatal(err)
}
res.Body.Close()
}
_ = buf
}
func BenchmarkOriginal(b *testing.B) {
benchmark(b, io.ReadAll)
}
func BenchmarkPatched(b *testing.B) {
benchmark(b, ReadAll)
}
func ReadAll(r io.Reader) ([]byte, error) {
sw := new(sliceWriter)
_, err := io.Copy(sw, r)
return *sw, err
}
type sliceWriter []byte
func (sw *sliceWriter) Write(b []byte) (int, error) {
*sw = append(*sw, b...)
return len(b), nil
}update: just for confirmation, I also tried with bytes.Buffer.ReadFrom, and it turns out my approach is even faster than that: func BenchmarkBufferReadFrom(b *testing.B) {
benchmark(b, func(r io.Reader) ([]byte, error) {
b := &bytes.Buffer{}
_, err := b.ReadFrom(r)
if err != nil {
return nil, err
}
return b.Bytes(), nil
})
} |
|
I think your approach is taking advantage of being able to read the entire buffer in one go. I pasted your Before: After: |
|
@bboreham My benchmark is using httptest.NewServer to simulate reading the http response body from a real socket. You can also confirm this by inspecting the benchmark CPU profile and noticing how it's spending most of its time in read syscalls deep into the net package.
On the other hand, your benchmark does not seem to make sense as you are instantiating a bytes.Buffer, but not using it. And you're throwing the actual results of ReadAll away... |
|
Sorry, going a bit too fast. I corrected the Prometheus client-library version to return the body, and the benchmark results are the same. Maybe it's just that your test response is quite small at 64K (if I read that right)? |
|
Yup. And you're right, with singnficantly larger response sizes (1MB or more) the larger buffer growth of bytes.Buffer will make bytes.Buffer.ReadFrom faster. I will try in the next days to see if there's something that can be done. |
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Called
ioutil.ReadAllto read everything from an http response.What did you expect to see?
Reasonable memory and CPU usage.
What did you see instead?
~60% more memory is allocated when using
ioutil.ReadAllcompared withbytes.Buffer.ReadFrom.See prometheus/client_golang#976.
#46564 is making the same complaint, although it was solved in a different way.
The underlying reason is that
ReadAll()usesappendto resize its buffer, which goes up by a factor of 1.25 (slightly more in latest Go, but asymptotically the same) each time.One might measure these alternatives on various axes:
The current implementation is better on the last one, and much worse on all the others.
The text was updated successfully, but these errors were encountered: