From 602860db264040d266318d8e4c7df1cb4d64d961 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Thu, 27 Aug 2026 19:51:58 +0900 Subject: [PATCH 1/4] perf: Reduce allocations in `Client.Do` response decoding with a buffer pool Replace the per-call json.NewDecoder streaming decode with a pooled bytes.Buffer read followed by json.Unmarshal. io.ReadAll-style decoding allocated a geometrically growing buffer per response (about 1.57MB for a 500KB payload); reading into a pooled buffer cuts that to about 0.62MB and 13 allocations. Empty and whitespace-only bodies keep returning nil, matching the previous io.EOF handling. Original work by @merchantmoh-debug in #4195, scoped to Client.Do decoding only as agreed with the maintainers there. --- github/github.go | 32 +++++++++--- github/github_benchmark_test.go | 88 +++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 6 deletions(-) create mode 100644 github/github_benchmark_test.go diff --git a/github/github.go b/github/github.go index 07b08ea27bf..7906ee4afa0 100644 --- a/github/github.go +++ b/github/github.go @@ -797,6 +797,14 @@ func WithVersion(version string) RequestOption { } } +// requestBufferPool pools response read buffers so that Client.Do can decode +// JSON payloads without allocating a growing buffer per call. +var requestBufferPool = sync.Pool{ + New: func() any { + return new(bytes.Buffer) + }, +} + // NewRequest creates an API request. A relative URL can be provided in urlStr, // in which case it is resolved relative to the BaseURL of the Client. // Relative URLs should always be specified without a preceding slash. If @@ -1409,12 +1417,24 @@ func (c *Client) Do(req *http.Request, v any) (*Response, error) { case io.Writer: _, err = io.Copy(v, resp.Body) default: - decErr := json.NewDecoder(resp.Body).Decode(v) - if decErr == io.EOF { - decErr = nil // ignore EOF errors caused by empty response body - } - if decErr != nil { - err = decErr + respBuf := requestBufferPool.Get().(*bytes.Buffer) + defer func() { + respBuf.Reset() + requestBufferPool.Put(respBuf) + }() + + _, readErr := respBuf.ReadFrom(resp.Body) + if readErr != nil { + err = readErr + } else if respBuf.Len() > 0 { + b := respBuf.Bytes() + decErr := json.Unmarshal(b, v) + if decErr != nil && len(bytes.TrimSpace(b)) == 0 { + decErr = nil // ignore errors caused by whitespace-only response body + } + if decErr != nil { + err = decErr + } } } return resp, err diff --git a/github/github_benchmark_test.go b/github/github_benchmark_test.go new file mode 100644 index 00000000000..0b568205af9 --- /dev/null +++ b/github/github_benchmark_test.go @@ -0,0 +1,88 @@ +// Copyright 2026 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package github + +import ( + "bytes" + "encoding/json" + "io" + "net/http" + "strings" + "testing" +) + +// legacyDecodeResponse simulates the behavior before pooled decoding +// (io.ReadAll -> json.Unmarshal). +func legacyDecodeResponse(resp *http.Response, v any) error { + data, err := io.ReadAll(resp.Body) + if err != nil { + return err + } + if len(data) > 0 { + return json.Unmarshal(data, v) + } + return nil +} + +// pooledDecodeResponse simulates the new pooled decoding behavior +// (requestBufferPool -> ReadFrom -> json.Unmarshal). +func pooledDecodeResponse(resp *http.Response, v any) error { + respBuf := requestBufferPool.Get().(*bytes.Buffer) + defer func() { + respBuf.Reset() + requestBufferPool.Put(respBuf) + }() + + _, err := respBuf.ReadFrom(resp.Body) + if err != nil { + return err + } + if respBuf.Len() > 0 { + b := respBuf.Bytes() + return json.Unmarshal(b, v) + } + return nil +} + +type dummyReadCloser struct { + io.Reader +} + +func (d *dummyReadCloser) Close() error { return nil } + +func BenchmarkDecodeResponse_Legacy(b *testing.B) { + payload, _ := json.Marshal(map[string]string{"title": "benchmark_test", "body": strings.Repeat("a", 1024*500)}) // 500KB JSON + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + b.StopTimer() + resp := &http.Response{ + Body: &dummyReadCloser{Reader: bytes.NewReader(payload)}, + } + var v map[string]string + b.StartTimer() + + _ = legacyDecodeResponse(resp, &v) + } +} + +func BenchmarkDecodeResponse_Pooled(b *testing.B) { + payload, _ := json.Marshal(map[string]string{"title": "benchmark_test", "body": strings.Repeat("a", 1024*500)}) // 500KB JSON + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + b.StopTimer() + resp := &http.Response{ + Body: &dummyReadCloser{Reader: bytes.NewReader(payload)}, + } + var v map[string]string + b.StartTimer() + + _ = pooledDecodeResponse(resp, &v) + } +} From 20726635f96e5ccecd6147de27a47aa1347efdd1 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Thu, 27 Aug 2026 19:52:26 +0900 Subject: [PATCH 2/4] test: Cover `Client.Do` decoding paths Add tests for the io.Writer path (success and write error), invalid JSON, whitespace-only bodies and the nil-v no-op, covering the decoding branches touched by the pooled-buffer change. Test cases provided by @gmlewis in the #4195 review, adjusted to satisfy the current linters (extraneousnew, fmtpercentv, revive). --- github/github_test.go | 103 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/github/github_test.go b/github/github_test.go index ae585cede4d..33e92c2354c 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -3248,6 +3248,109 @@ func TestDo_noContent(t *testing.T) { } } +func TestDo_ioWriter(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, "hello world") + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var buf bytes.Buffer + _, err := client.Do(req, &buf) + assertNilError(t, err) + + if buf.String() != "hello world" { + t.Errorf("Response body = %q, want %q", buf.String(), "hello world") + } +} + +func TestDo_ioWriter_error(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, "hello world") + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var w writerThatErrors + _, err := client.Do(req, &w) + if err == nil { + t.Fatal("Expected error from io.Writer, got none") + } +} + +type writerThatErrors struct{} + +func (w *writerThatErrors) Write(_ []byte) (int, error) { + return 0, errors.New("write error") +} + +func TestDo_invalidJSON(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + type foo struct { + A string + } + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, "not valid json") + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var body foo + _, err := client.Do(req, &body) + if err == nil { + t.Fatal("Expected JSON decode error, got none") + } +} + +func TestDo_whitespaceOnlyBody(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + type foo struct { + A string + } + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, " \n\t ") + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var body foo + _, err := client.Do(req, &body) + assertNilError(t, err) + + if body.A != "" { + t.Errorf("Response body = %v, want empty foo", body) + } +} + +func TestDo_nilV_noop(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, `{"A":"a"}`) + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + resp, err := client.Do(req, nil) + assertNilError(t, err) + if resp.StatusCode != http.StatusOK { + t.Errorf("Expected status %v, got %v", http.StatusOK, resp.StatusCode) + } +} + func TestClient_checkRequestAPIVersionBeforeDo(t *testing.T) { t.Parallel() From 56f55d40eaaa842ef23435a1a175466df713c0f5 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Thu, 27 Aug 2026 22:49:39 +0900 Subject: [PATCH 3/4] test: Benchmark the real `Client.Do` path using `b.Loop` Replace the decode-helper benchmarks with a single BenchmarkDo that runs through Client.Do against a stubbed transport, so the measured numbers reflect the production code path instead of a re-implementation. The old-behavior baseline comes from running the same benchmark on master. Addresses review feedback from @alexandear in #4195. --- github/github_benchmark_test.go | 104 ++++++++++++-------------------- 1 file changed, 38 insertions(+), 66 deletions(-) diff --git a/github/github_benchmark_test.go b/github/github_benchmark_test.go index 0b568205af9..47a61fe7239 100644 --- a/github/github_benchmark_test.go +++ b/github/github_benchmark_test.go @@ -8,81 +8,53 @@ package github import ( "bytes" "encoding/json" + "fmt" "io" "net/http" "strings" "testing" ) -// legacyDecodeResponse simulates the behavior before pooled decoding -// (io.ReadAll -> json.Unmarshal). -func legacyDecodeResponse(resp *http.Response, v any) error { - data, err := io.ReadAll(resp.Body) - if err != nil { - return err - } - if len(data) > 0 { - return json.Unmarshal(data, v) - } - return nil -} - -// pooledDecodeResponse simulates the new pooled decoding behavior -// (requestBufferPool -> ReadFrom -> json.Unmarshal). -func pooledDecodeResponse(resp *http.Response, v any) error { - respBuf := requestBufferPool.Get().(*bytes.Buffer) - defer func() { - respBuf.Reset() - requestBufferPool.Put(respBuf) - }() - - _, err := respBuf.ReadFrom(resp.Body) - if err != nil { - return err - } - if respBuf.Len() > 0 { - b := respBuf.Bytes() - return json.Unmarshal(b, v) - } - return nil +// fixedResponseTransport serves a canned JSON payload without touching the +// network so that BenchmarkDo measures the real request/decode path alone. +type fixedResponseTransport struct { + payload []byte } -type dummyReadCloser struct { - io.Reader +func (t *fixedResponseTransport) RoundTrip(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(bytes.NewReader(t.payload)), + Request: req, + }, nil } -func (d *dummyReadCloser) Close() error { return nil } - -func BenchmarkDecodeResponse_Legacy(b *testing.B) { - payload, _ := json.Marshal(map[string]string{"title": "benchmark_test", "body": strings.Repeat("a", 1024*500)}) // 500KB JSON - - b.ReportAllocs() - b.ResetTimer() - for i := 0; i < b.N; i++ { - b.StopTimer() - resp := &http.Response{ - Body: &dummyReadCloser{Reader: bytes.NewReader(payload)}, - } - var v map[string]string - b.StartTimer() - - _ = legacyDecodeResponse(resp, &v) - } -} - -func BenchmarkDecodeResponse_Pooled(b *testing.B) { - payload, _ := json.Marshal(map[string]string{"title": "benchmark_test", "body": strings.Repeat("a", 1024*500)}) // 500KB JSON - - b.ReportAllocs() - b.ResetTimer() - for i := 0; i < b.N; i++ { - b.StopTimer() - resp := &http.Response{ - Body: &dummyReadCloser{Reader: bytes.NewReader(payload)}, - } - var v map[string]string - b.StartTimer() - - _ = pooledDecodeResponse(resp, &v) +func BenchmarkDo(b *testing.B) { + for _, sizeKB := range []int{1, 500} { + b.Run(fmt.Sprintf("%vKB", sizeKB), func(b *testing.B) { + payload, err := json.Marshal(map[string]string{"body": strings.Repeat("a", sizeKB*1024)}) + if err != nil { + b.Fatalf("json.Marshal returned error: %v", err) + } + client, err := NewClient(WithHTTPClient(&http.Client{ + Transport: &fixedResponseTransport{payload: payload}, + })) + if err != nil { + b.Fatalf("NewClient returned error: %v", err) + } + req, err := client.NewRequest(b.Context(), "GET", ".", nil) + if err != nil { + b.Fatalf("NewRequest returned error: %v", err) + } + + b.ReportAllocs() + for b.Loop() { + var v map[string]string + if _, err := client.Do(req, &v); err != nil { + b.Fatalf("Do returned error: %v", err) + } + } + }) } } From 7e436614e78fadc8deb6eab5ca036913f4aa1ca1 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Thu, 27 Aug 2026 22:49:42 +0900 Subject: [PATCH 4/4] perf: Drop oversized response buffers instead of pooling them Cap the capacity of buffers returned to requestBufferPool at 1MB so an occasional very large response cannot pin memory in the pool. Add tests for the cap, for large-then-small body reuse, and for response body read errors. Addresses review feedback from @alexandear in #4195. --- github/github.go | 21 ++++++++++--- github/github_test.go | 73 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 4 deletions(-) diff --git a/github/github.go b/github/github.go index 7906ee4afa0..e72fd1474c1 100644 --- a/github/github.go +++ b/github/github.go @@ -805,6 +805,22 @@ var requestBufferPool = sync.Pool{ }, } +// maxPooledBufferCap is the largest buffer capacity that putRequestBuffer +// returns to requestBufferPool. Buffers that grew beyond it while reading an +// unusually large response are dropped so they do not pin memory in the pool. +const maxPooledBufferCap = 1 << 20 + +// putRequestBuffer resets buf and returns it to requestBufferPool, dropping +// oversized buffers. It reports whether buf was pooled. +func putRequestBuffer(buf *bytes.Buffer) bool { + if buf.Cap() > maxPooledBufferCap { + return false + } + buf.Reset() + requestBufferPool.Put(buf) + return true +} + // NewRequest creates an API request. A relative URL can be provided in urlStr, // in which case it is resolved relative to the BaseURL of the Client. // Relative URLs should always be specified without a preceding slash. If @@ -1418,10 +1434,7 @@ func (c *Client) Do(req *http.Request, v any) (*Response, error) { _, err = io.Copy(v, resp.Body) default: respBuf := requestBufferPool.Get().(*bytes.Buffer) - defer func() { - respBuf.Reset() - requestBufferPool.Put(respBuf) - }() + defer putRequestBuffer(respBuf) _, readErr := respBuf.ReadFrom(resp.Body) if readErr != nil { diff --git a/github/github_test.go b/github/github_test.go index 33e92c2354c..671b18ec8a9 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -3351,6 +3351,79 @@ func TestDo_nilV_noop(t *testing.T) { } } +func TestDo_readError(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + // Announce more bytes than are sent so that reading the response + // body fails with an unexpected EOF. + w.Header().Set("Content-Length", "64") + fmt.Fprint(w, `{"A":"a"}`) + }) + + type foo struct { + A string + } + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var body foo + _, err := client.Do(req, &body) + if err == nil { + t.Fatal("Expected body read error, got none") + } +} + +func TestDo_largeThenSmallBody(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + large := strings.Repeat("a", 2*maxPooledBufferCap) + mux.HandleFunc("/large", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprintf(w, `{"A":%q}`, large) + }) + mux.HandleFunc("/small", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, `{"A":"small"}`) + }) + + type foo struct { + A string + } + + req, _ := client.NewRequest(t.Context(), "GET", "large", nil) + var body foo + _, err := client.Do(req, &body) + assertNilError(t, err) + if body.A != large { + t.Error("Large response body was not decoded correctly") + } + + req, _ = client.NewRequest(t.Context(), "GET", "small", nil) + body = foo{} + _, err = client.Do(req, &body) + assertNilError(t, err) + if body.A != "small" { + t.Errorf("Response body = %v, want %v", body.A, "small") + } +} + +func TestPutRequestBuffer(t *testing.T) { + t.Parallel() + + if !putRequestBuffer(new(bytes.Buffer)) { + t.Error("putRequestBuffer returned false for a small buffer, want true") + } + + oversized := new(bytes.Buffer) + oversized.Grow(maxPooledBufferCap + 1) + if putRequestBuffer(oversized) { + t.Error("putRequestBuffer returned true for an oversized buffer, want false") + } +} + func TestClient_checkRequestAPIVersionBeforeDo(t *testing.T) { t.Parallel()