Skip to content

perf: pool []queryValues slices to reduce per-request allocations#753

Draft
mykaul wants to merge 2 commits intoscylladb:masterfrom
mykaul:pool_query_values
Draft

perf: pool []queryValues slices to reduce per-request allocations#753
mykaul wants to merge 2 commits intoscylladb:masterfrom
mykaul:pool_query_values

Conversation

@mykaul
Copy link
Copy Markdown

@mykaul mykaul commented Mar 8, 2026

Use bucketed sync.Pool (caps 8/16/32/64/128) to reuse []queryValues slices in executeQuery() and executeBatch(). Slices are returned to the pool immediately after c.exec() serializes the frame, with clear() to release string/[]byte references.

Benchmark Results
Benchmark Pool make() Speedup Alloc savings
Sequential (8 values) ~53 ns/op, 27 B ~132 ns/op, 387 B ~2.5x 360 B/op
Parallel (8 values) ~31 ns/op, 27 B ~180 ns/op, 387 B ~5.8x 360 B/op

@mykaul mykaul marked this pull request as draft March 8, 2026 20:48
@mykaul
Copy link
Copy Markdown
Author

mykaul commented Mar 11, 2026

CI failure is a transient issue, will send a separate fix.

@mykaul
Copy link
Copy Markdown
Author

mykaul commented Mar 11, 2026

CI failure is a transient issue, will send a separate fix.

#766

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces per-request allocations in the query execution path by pooling []queryValues slices (bucketed by common sizes) and reusing them across executeQuery() and executeBatch() calls.

Changes:

  • Add bucketed sync.Pool helpers for []queryValues allocation/reuse (caps 8/16/32/64/128).
  • Use the pool in Conn.executeQuery() / Conn.executeBatch() and return slices after request frame serialization.
  • Add unit tests and benchmarks covering bucket selection, pooling behavior, and allocation improvements.

Reviewed changes

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

File Description
frame.go Introduces the bucketed []queryValues pooling helpers (getQueryValues/putQueryValues/putBatchQueryValues).
conn.go Switches query/batch execution to allocate params.values / b.values via the pool and returns them after frame serialization (plus cleanup on errors).
frame_test.go Adds tests for bucket sizing and pooling behavior, plus benchmarks comparing pooled vs make() allocations.

Comment thread frame.go
Comment thread frame_test.go Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread frame.go Outdated
Comment thread conn.go Outdated
Comment thread conn.go Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread frame_test.go
Comment thread conn.go Outdated
Comment thread conn.go Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread conn.go
Comment thread conn.go
@mykaul mykaul force-pushed the pool_query_values branch from d7f412d to d43cce5 Compare March 17, 2026 17:49
@mykaul
Copy link
Copy Markdown
Author

mykaul commented Mar 17, 2026

Addressed the review feedback:

Fixed (this push):

  • Clarified the misleading comments in conn.go (lines 1575 and 1799). Changed "no longer needed after c.exec() serialized the frame" → "values were consumed during frame serialization inside c.exec()". The original wording implied c.exec() only serializes, but it also sends; the key point is that the values slice has been fully read by that point and is safe to return to the pool.

Not a bug (Copilot false positive):

  • The claim that s[:cap(s)] in getQueryValues will panic is factually wrong. Go allows reslicing up to the capacity of the original slice — this is a well-defined language feature, not a bug.

Acknowledged but not changing:

  • The suggestion to defer pool returns to after error checking is a style preference. The current placement is correct: values are consumed during c.exec(), so returning them immediately after (before checking the error) is safe and keeps the pool return close to the consumption point.

@mykaul mykaul force-pushed the pool_query_values branch from d43cce5 to da3e671 Compare March 24, 2026 19:22
mykaul added 2 commits April 10, 2026 20:08
Use bucketed sync.Pool (caps 8/16/32/64/128) to reuse []queryValues
slices in executeQuery() and executeBatch(). Slices are returned to the
pool immediately after c.exec() serializes the frame, with clear() to
release string/[]byte references.

Benchmark (8 values, sequential): ~3.9x faster than make().
Benchmark (8 values, parallel):   ~7.7x faster than make().
@mykaul mykaul force-pushed the pool_query_values branch from 9bb9a9e to c3c37e9 Compare April 10, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants