{"action":"created","issue":{"url":"https://api.github.com/repos/bradfitz/gomemcache/issues/45","repository_url":"https://api.github.com/repos/bradfitz/gomemcache","labels_url":"https://api.github.com/repos/bradfitz/gomemcache/issues/45/labels{/name}","comments_url":"https://api.github.com/repos/bradfitz/gomemcache/issues/45/comments","events_url":"https://api.github.com/repos/bradfitz/gomemcache/issues/45/events","html_url":"https://github.com/bradfitz/gomemcache/pull/45","id":137062141,"number":45,"title":"Use buffer pool to read from parseGetResponse bufio.Reader instead of using ReadAll everytime","user":{"login":"asticode","id":11619296,"avatar_url":"https://avatars.githubusercontent.com/u/11619296?v=3","gravatar_id":"","url":"https://api.github.com/users/asticode","html_url":"https://github.com/asticode","followers_url":"https://api.github.com/users/asticode/followers","following_url":"https://api.github.com/users/asticode/following{/other_user}","gists_url":"https://api.github.com/users/asticode/gists{/gist_id}","starred_url":"https://api.github.com/users/asticode/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/asticode/subscriptions","organizations_url":"https://api.github.com/users/asticode/orgs","repos_url":"https://api.github.com/users/asticode/repos","events_url":"https://api.github.com/users/asticode/events{/privacy}","received_events_url":"https://api.github.com/users/asticode/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-02-28T15:12:17Z","updated_at":"2016-05-06T02:06:27Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/bradfitz/gomemcache/pulls/45","html_url":"https://github.com/bradfitz/gomemcache/pull/45","diff_url":"https://github.com/bradfitz/gomemcache/pull/45.diff","patch_url":"https://github.com/bradfitz/gomemcache/pull/45.patch"},"body":"My main concern is using `ioutil.ReadAll` everytime the `bufio.Reader` is read in `parseGetResponse` since it will then trigger `bytes.makeSlice` and allocate new space each and every time the function is called.\r\n\r\nInstead I think using a buffer pool like `fmt` does would be a fairly better solution.\r\n\r\nI've added a benchmark in `memcache_test.go` that `Set` 4 []byte of different sizes in memcache, and then randomly `Get` them. Here's the result before using the buffer pool:\r\n\r\n```\r\n$ go test ./memcache -v -run=^$ -bench=BenchmarkGet$ -benchtime=10s -parallel=4 -memprofile=prof.mem\r\nPASS\r\nBenchmarkGet-4\t  200000\t     53816 ns/op\t   74349 B/op\t      36 allocs/op\r\n$ go tool pprof --alloc_space memcache.test prof.mem\r\n(pprof) list parseGetResponse\r\nTotal: 15.85GB\r\nROUTINE ======================== github.com/asticode/gomemcache/memcache.parseGetResponse in /home/asticode/projects/go/src/github.com/asticode/gomemcache/memcache/memcache.go\r\n   51.50MB    15.41GB (flat, cum) 97.23% of Total\r\n         .          .    453:\t\t\treturn err\r\n         .          .    454:\t\t}\r\n         .          .    455:\t\tif bytes.Equal(line, resultEnd) {\r\n         .          .    456:\t\t\treturn nil\r\n         .          .    457:\t\t}\r\n      46MB       46MB    458:\t\tit := new(Item)\r\n         .       66MB    459:\t\tsize, err := scanGetResponseLine(line, it)\r\n         .          .    460:\t\tif err != nil {\r\n         .          .    461:\t\t\treturn err\r\n         .          .    462:\t\t}\r\n    5.50MB    15.30GB    463:\t\tit.Value, err = ioutil.ReadAll(io.LimitReader(r, int64(size)+2))\r\n         .          .    464:\t\tif err != nil {\r\n         .          .    465:\t\t\treturn err\r\n         .          .    466:\t\t}\r\n         .          .    467:\t\tif !bytes.HasSuffix(it.Value, crlf) {\r\n         .          .    468:\t\t\treturn fmt.Errorf(\"memcache: corrupt get result read\")\r\n```\r\n\r\nAnd here's the result after using the buffer pool:\r\n\r\n```\r\n$ go test ./memcache -v -run=^$ -bench=BenchmarkGet$ -benchtime=10s -parallel=4 -memprofile=prof.mem\r\nPASS\r\nBenchmarkGet-4\t  500000\t     27828 ns/op\t    1082 B/op\t      31 allocs/op\r\n$ go tool pprof --alloc_space memcache.test prof.mem\r\n(pprof) list parseGetResponse\r\nTotal: 549.90MB\r\nROUTINE ======================== github.com/asticode/gomemcache/memcache.parseGetResponse in /home/asticode/projects/go/src/github.com/asticode/gomemcache/memcache/memcache.go\r\n      49MB   341.24MB (flat, cum) 62.06% of Total\r\n         .          .    466:\t\t\treturn err\r\n         .          .    467:\t\t}\r\n         .          .    468:\t\tif bytes.Equal(line, resultEnd) {\r\n         .          .    469:\t\t\treturn nil\r\n         .          .    470:\t\t}\r\n   33.50MB    33.50MB    471:\t\tit := new(Item)\r\n         .   115.50MB    472:\t\tsize, err := scanGetResponseLine(line, it)\r\n         .          .    473:\t\tif err != nil {\r\n         .          .    474:\t\t\treturn err\r\n         .          .    475:\t\t}\r\n         .          .    476:\t\tbuf := bufferPool.Get().(*bytes.Buffer)\r\n   15.50MB   192.23MB    477:\t\t_, err = buf.ReadFrom(io.LimitReader(r, int64(size)+2))\r\n         .          .    478:\t\tif err != nil {\r\n         .          .    479:\t\t\tfreeBuffer(buf)\r\n         .          .    480:\t\t\treturn err\r\n         .          .    481:\t\t}\r\n         .          .    482:\t\tit.Value = buf.Bytes()\r\n\r\n```"},"comment":{"url":"https://api.github.com/repos/bradfitz/gomemcache/issues/comments/217330998","html_url":"https://github.com/bradfitz/gomemcache/pull/45#issuecomment-217330998","issue_url":"https://api.github.com/repos/bradfitz/gomemcache/issues/45","id":217330998,"user":{"login":"twmb","id":1283109,"avatar_url":"https://avatars.githubusercontent.com/u/1283109?v=3","gravatar_id":"","url":"https://api.github.com/users/twmb","html_url":"https://github.com/twmb","followers_url":"https://api.github.com/users/twmb/followers","following_url":"https://api.github.com/users/twmb/following{/other_user}","gists_url":"https://api.github.com/users/twmb/gists{/gist_id}","starred_url":"https://api.github.com/users/twmb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/twmb/subscriptions","organizations_url":"https://api.github.com/users/twmb/orgs","repos_url":"https://api.github.com/users/twmb/repos","events_url":"https://api.github.com/users/twmb/events{/privacy}","received_events_url":"https://api.github.com/users/twmb/received_events","type":"User","site_admin":false},"created_at":"2016-05-06T02:06:27Z","updated_at":"2016-05-06T02:06:27Z","body":"This races; `buf.Reset` will reuse the bytes you just returned on the next `Get`."}}