⚡ Bolt: pool compileContext to reduce query compilation allocations#93
Conversation
Introduces a `sync.Pool` for `compileContext` in `pkg/rain/query_compile.go` to reuse `strings.Builder` and `argPlan` slices during SQL compilation. Key changes: - Implement `compileContextPool` with `reset(dialect.Dialect)` logic. - Update all compilation paths (SELECT, INSERT, UPDATE, DELETE, DDL) to release contexts back to the pool. - Ensure safety by explicitly copying the escaping `argPlan` slice in `compiledQuery()`. - Refactor internal tests to use prompt subtests/blocks for optimal context lifecycle management. Performance impact (BenchmarkSQLiteSelectPointLookup/medium): - Allocations: -2 allocs/op (46 -> 44) - Memory: -416 B/op (1600 -> 1184) - Execution time: ~3-6% faster compilation path. Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge — all call sites correctly pair acquire with deferred release, both the SQL string and argPlan are independently copied before the context is returned to the pool, and the reset() method properly zeroes GC-retained values. The ownership model is clean: each compilation function gets a context, builds the query, calls compiledQuery() which creates independent copies of both the SQL string and argPlan, and the defer returns the context to the pool only after those copies exist. The clear()+reslice ordering in reset() correctly prevents reference leaks. No data is shared across concurrent uses. No files require special attention. query_compile.go is the most critical file and its logic is sound.
|
| Filename | Overview |
|---|---|
| pkg/rain/query_compile.go | Core pooling logic: adds sync.Pool, reset(), releaseCompileContext(), and updates compiledQuery() to copy argPlan before pool return. Logic is correct — clear()+reslice order preserves GC safety, and both sql string and argPlan are independent copies by the time defer fires. |
| pkg/rain/ddl.go | Adds defer releaseCompileContext(ctx) to createViewSQL; ctx.String() and ctx.err are evaluated into return values before the defer fires, so the string copy is safe. |
| pkg/rain/query_select.go | Adds defer releaseCompileContext to compile() and compileAggregate(); compiledQuery() is called before defer fires in both paths. |
| pkg/rain/query_compile_internal_test.go | Converts inline newCompileContext() one-liners (that previously leaked into the pool) into proper named subtests with defer releaseCompileContext. All new/existing subtests correctly pair acquire/release. |
| pkg/rain/query_common_internal_test.go | Restructures flat test cases into named subtests with proper context lifecycle; ctx.dialect captured into SelectQuery before defer fires, so no stale-reference issue. |
| pkg/rain/query_insert.go | Adds defer releaseCompileContext to both ToSQL() and toSelectSQL(); straightforward and correct. |
| pkg/rain/query_delete.go | Adds defer releaseCompileContext to ToSQL(); correct. |
| pkg/rain/query_update.go | Adds defer releaseCompileContext to ToSQL(); correct. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Pool as sync.Pool
participant Ctx as compileContext
participant CQ as compiledQuery
Caller->>Pool: newCompileContext(dialect)
Pool-->>Ctx: Get() / New()
Ctx->>Ctx: reset(dialect) builder.Reset(), clear(argPlan), argPlan[:0]
Pool-->>Caller: "*compileContext"
Caller->>Ctx: writeSQL / writeString / writeExpression ...
Note over Ctx: builds SQL in builder, appends to argPlan
Caller->>Ctx: compiledQuery()
Ctx->>CQ: "sql = builder.String() (copy)"
Ctx->>CQ: "argPlan = append(nil, argPlan...) (copy)"
CQ-->>Caller: "compiledQuery{sql, argPlan, hasNames}"
Caller->>Pool: releaseCompileContext(ctx) [via defer]
Pool->>Ctx: Put(ctx)
Note over Pool: ctx available for next Get()
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
pkg/rain/query_compile.go:109-112
When a context with a very large `argPlan` (e.g., from a big IN-list query) is returned to the pool, the oversized backing array is kept indefinitely because `c.argPlan[:0]` only resets the length — the capacity is never trimmed. Under high query-variety workloads this can cause pool items to retain far more memory than needed. Capping the slice when it has grown well beyond its initial size avoids that steady-state bloat.
```suggestion
// Clear the argPlan slice to ensure any values it contains can be
// garbage collected before we reset its length for reuse.
clear(c.argPlan)
if cap(c.argPlan) > 64 {
c.argPlan = make([]compiledArg, 0, 8)
} else {
c.argPlan = c.argPlan[:0]
}
```
Reviews (2): Last reviewed commit: "⚡ Bolt: pool compileContext to reduce qu..." | Re-trigger Greptile
Introduces a `sync.Pool` for `compileContext` in `pkg/rain/query_compile.go` to reuse `strings.Builder` and `argPlan` slices during SQL compilation. Key changes: - Implement `compileContextPool` with `reset(dialect.Dialect)` logic. - Use `clear(c.argPlan)` in `reset` to prevent memory leaks for pooled objects. - Update all compilation paths (SELECT, INSERT, UPDATE, DELETE, DDL) to release contexts back to the pool. - Ensure safety by explicitly copying the escaping `argPlan` slice in `compiledQuery()`. - Refactor internal tests to use prompt subtests/blocks for optimal context lifecycle management. Performance impact (BenchmarkSQLiteSelectPointLookup/medium): - Allocations: -2 allocs/op (46 -> 44) - Memory: -416 B/op (1600 -> 1184) - Execution time: ~3-6% faster compilation path. Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
I implemented
compileContextpooling to reduce heap allocations and GC pressure during query compilation. By using async.Poolto reusestrings.Builderand argument tracking slices, I reduced allocations by 2 and memory usage by ~416 bytes for common point lookup queries.To maintain absolute correctness and thread safety, I updated the
compiledQuery()method to explicitly copy theargPlanslice before the context is returned to the pool. This prevents any use-after-release data corruption if a query is compiled and its result is used while the context is being reused by another goroutine.I also refactored internal test suites to properly manage the context lifecycle, ensuring that they serve as a verification for the pooling logic. All integration and unit tests pass.
PR created automatically by Jules for task 17694720199844462160 started by @cungminh2710