skip to content

What do you require before approving sync.Pool in a shared library other teams import?

level: principalimportance: nice to knowfreq 25%

answer

  1. not a local optimisation
  2. a dirty entry crosses request boundaries
  3. make the package enforce the reset
  4. who can be overruled, and on what evidence

basics

~20 s

Require evidence the allocation matters, a reset enforced inside the package's own get and put wrappers rather than by callers, an API that returns nothing aliasing pooled memory, a capacity guard on release, and a test proving a reused object comes back clean.

solid answer

~60 s

Object reuse is not a normal micro-optimisation, because its failure mode is not slowness: an object handed back unreset gives one request the bytes of another, which in a multi-tenant service is a disclosure class that is silent, intermittent and load-dependent. So the review question is whether the invariant is enforced by the code or by everyone who will ever call it — and in a library other teams import, the second answer is not acceptable. I want a measurement on a genuinely hot path; the pool kept unexported; reset performed inside the package's own get and put wrappers so callers cannot skip it; nothing returned that still aliases the pooled buffer, copying on the way out if necessary; a capacity guard so one outlier cannot set the memory floor; and a test that poisons each object before it goes back and fails if a later get ever sees the marker. The library team owns that contract, because consumers cannot see it. If the contract can only be honoured by the caller, the answer is redesign, not documentation.

code

go · 18 lines
go
var pool sync.Pool // unexported: callers cannot Put values of their own

func getBuf() *bytes.Buffer {
	b, _ := pool.Get().(*bytes.Buffer)
	if b == nil {
		return new(bytes.Buffer)
	}
	b.Reset() // reset on the way out as well as on the way in
	return b
}

func putBuf(b *bytes.Buffer) {
	if b.Cap() > 64<<10 {
		return
	}
	b.Reset()
	pool.Put(b)
}

go deeper

for a junior

Know the rule you will be held to: anything returned to a sync.Pool must be reset first, because the next caller receives that same object with whatever was left inside it.

for a middle

Explain why a missed reset hides in testing — the pool may hand back a fresh object instead of the reused one — and why the reset therefore belongs in code the caller cannot bypass.

for a senior

Argue the concrete design: an unexported pool, get and put wrappers that reset on both sides, no returned value aliasing pooled memory, a capacity guard, and a test that proves reuse comes back clean.

for a principal

This is a call you can be overruled on and must be able to defend. Say what evidence justifies admitting a new failure class, who owns the reset contract once teams you never meet import the library, and what the default answer is when the evidence is thin.

## Why this is a review decision at all Most micro-optimisations fail in one direction: they add complexity and buy nothing. Object reuse fails in two. A `sync.Pool` handing back an object that was not fully reset is not a slow program — it is one request reading bytes that belong to another request. In a multi-tenant service that is a data-disclosure class, and it has every property that makes a bug expensive: it is silent, it is intermittent (the pool may return a fresh object instead of the reused one, so the same test passes most of the time), it does not reproduce under low load, and the blast radius is other people's data. So the question a reviewer answers is not "is this faster" but "is the invariant that keeps this safe enforced by the code, or by everyone who will ever call it". In a library other teams import, the second answer is not acceptable, because the callers cannot see the contract and the library author will not see the call sites. ## What to require before approving **1. Evidence that the allocation matters.** A measurement on the path that is actually hot in production, produced by the author. Without it, you are trading a class of correctness bug for a number nobody has. **2. The pool is unexported, and so is the pooled type's raw form.** If consumers can `Put` values of their own, no reviewer and no test can bound what comes out of `Get`. **3. Reset lives in the package's own wrappers, on both sides.** A `get` function that resets before returning and a `put` function that resets before storing. Resetting twice is nearly free and removes the single-point-of-failure. Callers must not be able to reach the pool without going through them. **4. Nothing that leaves the package aliases pooled memory.** This is the rule that most often kills an adoption. If the API returns a slice or a `[]byte`-backed value that shares an array with the buffer being returned to the pool, the caller reads memory another request is refilling. Either copy on the way out (`string(buf)`, `append([]byte(nil), buf...)`), or restructure so the borrow begins and ends inside one function, or do not pool. **5. A capacity guard on `put`.** Uniform entry cost is a pool's unwritten precondition; drop outliers rather than letting one of them set the memory floor. **6. A test that proves it, not a comment that claims it.** Fill each object with a marker before it goes back and fail if a later `get` ever sees the marker; run the package under the race detector in CI. Encode the invariant so that a new field added to the pooled struct next year, which nobody remembers to reset, shows up as a red build rather than a support ticket. ## Who owns the contract The library team, without exception — the consumer cannot see it, cannot test it, and did not choose it. That ownership has a design consequence: if the reset contract *can only* be honoured by the caller, the design is wrong and documentation will not rescue it. Send it back and either move the borrow inside the package or drop the pool. ## What you would overrule, and on what grounds - A measured win on a path that is not hot in the real workload — a few percent of a request that spends most of its time waiting on a database is not worth a class of silent data-mixing bugs. - An API whose returned value aliases the pool. This is a redesign, not a comment. - "We will pool everywhere for consistency." Pooling is a local remedy for a measured hot path, not a house style; every additional pooled type is another reset contract someone must maintain. - A pool introduced to work around allocations the compiler would have kept on the stack anyway, where the honest fix is to stop the value escaping in the first place. ## When the answer is straightforwardly yes A hot encoder or formatter inside the library's own inner loop, over objects of uniform size, where the borrowed object never leaves the function that took it and the reset is one call the package owns. That is the shape the pool was designed for, and it should be approved without ceremony. ## The default For a shared library, the default is no. Not because pooling is wrong, but because the burden of proof sits with the change that admits a new failure class: show the measurement, show the enforcement, show the test. Teams that adopt pooling as a habit accumulate reset contracts far faster than they accumulate the discipline to maintain them.

  • How do you decide who owns the reset when the pooled object crosses a package boundary?
    It cannot cross. Once the value leaves the package, the package can no longer know when the caller is finished, so either copy on the way out and keep the original pooled, or do not pool at all. Ownership that depends on a caller reading documentation is not ownership, and that is the version a reviewer should send back.
  • What would make you overrule an adoption that already has a benchmark behind it?
    A measured win on a path that is not hot in the real workload, or an API shape where the reset cannot be enforced. A few percent on a request that spends most of its time waiting on a network call does not buy a whole class of silent data-mixing bugs; keep the measurement and change the design.
  • How do you keep the invariant from rotting a year later?
    Encode it in tests rather than in review memory: fill each object with a marker before it goes back and fail if a later borrow ever sees the marker, and run the package under the race detector in CI. Keep the pool unexported so that a new field nobody resets shows up as a red build rather than as a support ticket.

saying these in an interview costs you the question

  • Treats pooling as purely a performance tradeoff
  • Leaves the reset to each caller and documents it
  • Exports the pool so consumers can Put their own values
  • Returns a value that still aliases the pooled buffer
  • Adopts pooling across the codebase without a measurement
  • Trusts a passing test that never got the reused object back