add sync.Pools for high-impact functions by kke · Pull Request #350 · k0sproject/rig

kke · 2026-05-25T14:08:31Z

There were a couple of points where allocating buffers through sync.Pool will be meaningful, perhaps even more so than what already is pooled.

Copilot

Pull request overview

This PR introduces additional sync.Pool usage to reduce allocations in hot paths (PowerShell command construction, command execution output buffering, and Windows remote directory listing).

Changes:

Reuse bytes.Buffer instances in Executor.ExecOutputContext via a pool with a size cap.
Pool gzip compression state (bytes.Buffer + gzip.Writer) used by powershell.CompressedCmd.
Pool strings.Builder instances for PowerShell quoting helpers, and sync pooled slice growth back in Windows ReadDir.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`remotefs/windir.go`	Synces the possibly-grown pooled slice header back before returning it to the pool.
`powershell/powershell.go`	Adds pools for gzip compression buffers and string builders used in quoting/compression helpers.
`cmd/executor.go`	Adds a pooled `bytes.Buffer` for `ExecOutputContext` stdout capture with a capacity limit.

Comments suppressed due to low confidence (1)

powershell/powershell.go:154

Same issue as SingleQuote: returning buf.String() while deferring buf.Reset()+Put() is unsafe with strings.Builder because the returned string can share the builder's backing array. Reset/reuse can mutate the returned string contents. Prefer not pooling builders in quoting helpers, or copy the result before resetting/putting back.

	defer func() {
		buf.Reset()
		builderPool.Put(buf)
	}()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

powershell/powershell.go:141

The loop variable name rune is easy to confuse with the predeclared rune type. Renaming it to something like r/ch would improve readability without changing behavior.

This issue also appears on line 164 of the same file.

	buf.Grow(len(str) + 3)
	buf.WriteRune('\'')
	for _, rune := range str {
		switch rune {
		case '\n', '\r', '\t', '\v', '\f', '\a', '\b', '\'', '`', '\x00':
			buf.WriteString("`")
			buf.WriteRune(rune)
		default:
			buf.WriteRune(rune)
		}

powershell/powershell.go:172

Same readability issue here: using rune as the iteration variable name can be confusing since rune is also a predeclared type name. Consider renaming the variable (e.g., r/ch).

	buf.Grow(len(str) + 4)
	buf.WriteRune('"')
	for _, rune := range str {
		switch rune {
		case '"':
			buf.WriteString("`\"")
		default:
			buf.WriteRune(rune)
		}

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

fileinfos is copied from *fileinfosptr as a local slice header. After json.Decode appends into it the grown backing array is never written back, so the pool always recycles a zero-length slice and loses the capacity. Write *fileinfosptr = fileinfos after <-done (the happens-before barrier) so the pool item retains whatever capacity Decode allocated. Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

Every ExecOutput call allocates a bytes.Buffer, fills it, calls String, and discards it. Add a package-level sync.Pool to recycle buffers across calls. Buffers larger than 64 KiB are not returned to the pool to avoid pinning memory after large one-off outputs. Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

…strings.Builder in SingleQuote/DoubleQuote CompressedCmd allocates a bytes.Buffer and gzip.Writer on every call. Pool them together in a compressBuf struct so both are reset and reused: buf.Reset then gz.Reset(buf) reinstates the gzip header state. SingleQuote and DoubleQuote are called per-argument when building PowerShell commands. Pool a strings.Builder (same pattern as sh/shellescape) to avoid per-call heap allocations. Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

Large inputs could leave oversized buffers pinned in the pool forever. Apply the same cap-guard pattern already used in cmd/executor.go bufferPool: - compressPool: discard compressBuf when buf.Cap() > 64 KiB - builderPool: discard strings.Builder when Cap() > 64 KiB - fileInfoPool: discard slice when cap > 1000 entries Also restructure windir.ReadDir to early-return when buffer is already populated, reducing nesting depth and bringing nestif complexity within the linter threshold. Add a comment to builderPool explaining why Reset()+pool reuse is safe: Reset() nils b.buf, so the next Grow allocates fresh memory and the previously returned String() value retains its own reference to the old backing array. Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

…tests windir.go: clear the backing array before returning the slice to fileInfoPool so pooled items do not retain stale *winFileInfo pointers across ReadDir calls, allowing the GC to collect previously decoded directory entries promptly. powershell_test.go: add table-driven tests for SingleQuote, DoubleQuote, and CompressedCmd covering edge cases (empty string, special characters, already-quoted input) and repeated-call tests to exercise sync.Pool reuse paths and confirm correctness is preserved across multiple uses of the pooled builder/buffer. Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

Reset() only sets len=0; the compressed script bytes (compressPool) and captured command output (bufferPool) remained readable in the backing array across pool reuses. Call clear(buf.Bytes()) before Reset() to zero the live data region before the item is returned to the pool. strings.Builder is unaffected: its Reset() nils b.buf entirely, so the old backing array is disconnected and GC-eligible immediately. Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

…r clear On Start failure only pipeW was closed, leaving pipeR open until GC. Close both pipe ends on the early-return path. Use the three-index full-slice expression (s[:cap:cap]) when reslicing the fileinfos backing array before clear, making the capacity-extend intent explicit and unambiguous. Signed-off-by: Kimmo Lehto <klehto@mirantis.com>

Copilot