perf: Reduce allocations in Client.Do response decoding with a buffer pool

[JamBalaya56562]

Closes: #4195 — all credit for the idea and the core implementation goes to @merchantmoh-debug. That PR reached a shape the maintainers agreed on (scoped strictly to Client.Do decoding, with the NewRequest pooling stripped per the review discussion), but has been inactive for about three and a half months with only the coverage gap left, so this picks it up and carries it over the line on top of the current master.

What it does

Replaces the per-call json.NewDecoder(resp.Body).Decode(v) in Client.Do with a read into a pooled bytes.Buffer followed by json.Unmarshal. The streaming decoder's internal buffering allocated a geometrically growing buffer per response; the pool amortizes that across calls. Buffers whose capacity grew beyond 1MB while reading an unusually large response are dropped instead of pooled, so they cannot pin memory.

Benchmarks

BenchmarkDo runs through the real Client.Do against a stubbed transport; the baseline is the same benchmark run on master (34cec4ff). benchstat, n=8:

	master B/op	this PR B/op	master allocs/op	this PR allocs/op
Do/1KB	4.758Ki ± 0%	2.580Ki ± 0% (−45.8%)	23	20 (−13.0%)
Do/500KB	1527.8Ki ± 0%	530.2Ki ± 3% (−65.3%)	31	20 (−35.5%)

sec/op improved as well (−70.7% / −82.9%, p=0.000), though the baseline runs had high variance on the measurement machine (±56% / ±122%), so the allocation metrics above are the reliable signal.

Semantics

• Empty and whitespace-only bodies keep returning nil, matching the previous io.EOF handling (covered by TestDo_noContent and the new TestDo_whitespaceOnlyBody).
• json.Unmarshal is stricter about trailing data than the streaming decoder — this was discussed on perf: Boost decoding throughput and eliminate payload buffer allocations in core client #4195 and the affected test fixtures were already fixed in test: Fix invalid JSON payloads in actions workflow runs tests #4197.

What's new relative to #4195

• Rebased onto current master.
• The Client.Do coverage tests that @gmlewis wrote and posted in the perf: Boost decoding throughput and eliminate payload buffer allocations in core client #4195 review are now actually applied (TestDo_ioWriter, TestDo_ioWriter_error, TestDo_invalidJSON, TestDo_whitespaceOnlyBody, TestDo_nilV_noop), lightly adjusted to satisfy the current linters (extraneousnew, fmtpercentv, revive). This closes the codecov gap that was the last open item there.
• @alexandear's review feedback from perf: Boost decoding throughput and eliminate payload buffer allocations in core client #4195 is addressed: the benchmark exercises the real Client.Do path with b.Loop and an on-master baseline instead of decode-helper re-implementations; oversized buffers are dropped from the pool (maxPooledBufferCap), with tests for the cap, large-then-small body reuse, and response body read errors.

Verified with go build ./..., gofmt, the full ./github/ test suite, the benchmarks above, and custom-gcl (no findings on the changed code).

cc @merchantmoh-debug — hope you don't mind me carrying this forward; happy to hand it back if you'd like to finish it yourself.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.52%. Comparing base (f9fec07) to head (d1e48b2).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4494   +/-   ##
=======================================
  Coverage   98.52%   98.52%           
=======================================
  Files         195      195           
  Lines       17729    17742   +13     
=======================================
+ Hits        17468    17481   +13     
  Misses        261      261           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[alexandear]

My comments are the same as in #4195.

[JamBalaya56562]

@alexandear Addressed all three in 56f55d4 and 7e43661:

• Benchmark accuracy / running through Client.Do: the decode-helper benchmarks are gone. BenchmarkDo now runs through the real Client.Do against a stubbed transport, using b.Loop, and the old-behavior baseline is the same benchmark run on master (34cec4f) — so both sides measure the actual production path (json.NewDecoder before, pooled json.Unmarshal after). benchstat:

             │    master     │              this PR                │
             │    sec/op     │    sec/op     vs base               │
  Do/1KB      122.10µ ±  56%   35.76µ ± 12%  -70.71% (p=0.000 n=8)
  Do/500KB    55.285m ± 122%   9.436m ±  7%  -82.93% (p=0.000 n=8)
  
             │     B/op      │
  Do/1KB      4.758Ki  ±  0%   2.580Ki ± 0%  -45.77% (p=0.000 n=8)
  Do/500KB    1527.8Ki ±  0%   530.2Ki ± 3%  -65.30% (p=0.000 n=8)
  
             │   allocs/op   │
  Do/1KB      23.00 ± 0%       20.00 ± 0%    -13.04% (p=0.000 n=8)
  Do/500KB    31.00 ± 0%       20.00 ± 0%    -35.48% (p=0.000 n=8)
  

  The baseline sec/op runs had high variance on my measurement machine, so I would treat B/op and allocs/op as the reliable signal here.

• Pool-specific edge risks: buffers whose capacity grew beyond 1MB while reading an unusually large response are now dropped instead of returned to the pool (putRequestBuffer / maxPooledBufferCap), so occasional large responses cannot pin memory. New tests cover the cap itself (TestPutRequestBuffer), a 2MB-body-then-small-body sequence through Client.Do (TestDo_largeThenSmallBody), and response body read errors (TestDo_readError) — the last one also closes the codecov/patch gap.

On the broader question of whether this is worth shipping at all: the numbers above are now measured on the production path rather than a re-implementation, which is the most concrete data I can offer; I'll leave that call to the maintainers.

[gmlewis]

Thank you, @JamBalaya56562 and @merchantmoh-debug!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @alexandear

[merchantmoh-debug]

@alexandear Addressed all three in 56f55d4 and 7e43661:

• Benchmark accuracy / running through Client.Do: the decode-helper benchmarks are gone. BenchmarkDo now runs through the real Client.Do against a stubbed transport, using b.Loop, and the old-behavior baseline is the same benchmark run on master (34cec4f) — so both sides measure the actual production path (json.NewDecoder before, pooled json.Unmarshal after). benchstat:

             │    master     │              this PR                │
             │    sec/op     │    sec/op     vs base               │
  Do/1KB      122.10µ ±  56%   35.76µ ± 12%  -70.71% (p=0.000 n=8)
  Do/500KB    55.285m ± 122%   9.436m ±  7%  -82.93% (p=0.000 n=8)
  
             │     B/op      │
  Do/1KB      4.758Ki  ±  0%   2.580Ki ± 0%  -45.77% (p=0.000 n=8)
  Do/500KB    1527.8Ki ±  0%   530.2Ki ± 3%  -65.30% (p=0.000 n=8)
  
             │   allocs/op   │
  Do/1KB      23.00 ± 0%       20.00 ± 0%    -13.04% (p=0.000 n=8)
  Do/500KB    31.00 ± 0%       20.00 ± 0%    -35.48% (p=0.000 n=8)
  

  The baseline sec/op runs had high variance on my measurement machine, so I would treat B/op and allocs/op as the reliable signal here.

• Pool-specific edge risks: buffers whose capacity grew beyond 1MB while reading an unusually large response are now dropped instead of returned to the pool (putRequestBuffer / maxPooledBufferCap), so occasional large responses cannot pin memory. New tests cover the cap itself (TestPutRequestBuffer), a 2MB-body-then-small-body sequence through Client.Do (TestDo_largeThenSmallBody), and response body read errors (TestDo_readError) — the last one also closes the codecov/patch gap.

On the broader question of whether this is worth shipping at all: the numbers above are now measured on the production path rather than a re-implementation, which is the most concrete data I can offer; I'll leave that call to the maintainers.

My personal opinion on the logic of "If it aint broke don't fix it" is: Every single performance optimization in of itself is not a goal in a vacuum; instead, it's the culminative effect that is the genuine end goal. No one was really complaining about stringify being too slow when I dropped an almost 3x perf boost to it. We still merged it. Because it was a no-brainer.

As long as the code can be understood by the maintainers (Not AI slop - or at least someone whose really good at using AI instead of a generic user copy pasting the code and prompting "Plz make dis faster, no mistakes" --- I don't see this being the latter. Happy to see it merged as long as it benefits the repo & its users which I believe this would. Least it would motivate them to see that GO is still improving.

[gmlewis]

Thank you, @JamBalaya56562 and @alexandear!
Merging.