Repository navigation
perf(parser): co-allocate small slices and size lists exactly [W4-04] - #931
ajitpratap0 wants to merge 8 commits into
Conversation
…atch stacks [W4-04] Function arguments, the SELECT list, GROUP BY and ORDER BY are collected on parser-owned scratch stacks and copied once into exact-size slices. A FunctionCall (<= 3 args) and a SelectStatement (+ From[1], + Columns <= 4) share one allocation with their small slices. aggregate allocs/op 18 -> 13, complex 50 -> 44; time neutral. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @ajitpratap0's task in 47s —— View job Review of PR #931: co-allocate small slices and size lists exactlyVerdict: looks good. I found no blocking issues. I read the diff only. I did not run the tests or benchmarks, so I'm relying on the PR's reported results ( Correctness (checked by reading)
Minor suggestions (non-blocking)
Tests
The mutation check on the clears is a nice touch. One gap: a test that parses a very large list (more than 1024 elements) and then confirms Performance and security |
Perf gate: REGRESSION (shadow mode: report only, this check never fails)Base
Rules: time regresses at >= +3% with p < 0.01 (Mann-Whitney U), or a suite geomean >= +1.5% (per-round geomeans, same test); allocs/op must not increase; B/op must not rise more than 2%; instructions/op must not rise more than 1.5%. An A/A control with more than 10% of benchmarks moving over 3% is noisy: re-run once, then inconclusive (neutral). Gated suite (GOGC=off, GOMAXPROCS=1)
A/A control: 1 of 32 benchmarks moved more than 3%. Instructions per operation (cmd/perfdriver)
Secondary metric: default GC, GOMAXPROCS=1 (informational)
Environmentbenchstat (primary metric) |
…frames small [W4-04] The deferred drops grew the parseFunctionCall and parseSelectStatement frames (+88/+64 B on amd64) and made fresh-goroutine parses slower in the CI gate. The builders now drop explicitly on their error returns and move their lists into the node as soon as they are complete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 37s —— View job Review update for PR #931 (perf gate run on head
|
| Case | Time | Allocs/op |
|---|---|---|
ParseFresh/simple_select |
+42% | 6 → 4 |
ParseFresh/join |
+25% | 18 → 16 |
ParseFresh/complex |
+16% | 51 → 45 |
PoolGetPut/serial |
+6.7% | 0 → 0 |
The suite geomean is +2.9% (threshold +1.5%), against the PR's local figure of -1.07%.
- What the benchmark measures.
BenchmarkParseFresh(pkg/gosqlx/bench_server_test.go:76) runs each parse on a new goroutine, so it pays for stack growth from the small initial stack. Allocations and instruction counts both went down: e2e instructions/op is -0.5% to -3.4%. A time-only regression with fewer allocs and instructions points at goroutine stack growth, not extra work. - Likely cause. The co-allocation and scratch changes enlarge the stack frames of the recursive functions in
parseSelectStatement,parseFunctionCallandparseFromClause. The structs sized to size classes are large for a stack frame (a 392 BSelectStatementplus aTableReferenceplus columns). If any is built on the stack before escaping, or if the genericscratchPush[T]andscratchPop[T]instantiations inline into those functions, the frames grow. A fresh 2–8 KB goroutine stack then crosses a growth threshold earlier and does one moremorestackcopy. - Related symptoms.
- The commit "unwind scratch on error paths instead of defer to keep frames small" shows you already suspected frame size.
PoolGetPut/serialis also +6.7%. It does no parsing allocation, so this may be noise or code-layout or alignment shift. Rerun it before blaming the PR.- Warm-loop
ParseRWis +0.6% to +2.5% on the gated suite despite 8–19% fewer allocs, which is also consistent with larger frames or extra per-element copying.
- Suggested diagnosis.
- Compare frame sizes with
go build -gcflags=-S ./pkg/sql/parser 2>&1 | grep -E 'TEXT.*(parseSelectStatement|parseFunctionCall|parseFromClause|parseExpression)', base vs head. The frame size is the number after$. - Run
-gcflags=-mand look at thenewSelectStatementandnewFunctionCallblock structs. Check that they are heap-allocated in an out-of-line//go:noinlinehelper and not constructed in the caller's frame. - Add
//go:noinlineto the coldn <= 2/n <= 4branches, or split them into per-size helpers, so the caller only holds a pointer. - Confirm with
go test -bench ParseFresh -count=10locally, and compareGODEBUG=...stack-growth counts if needed.
- Compare frame sizes with
Merge guidance. The gate is in shadow mode, so this does not block the merge mechanically. But the PR's own goal is performance, and goroutine-per-request is the server shape the project benchmarks on purpose. A +16% to +42% regression on that shape deserves a fix, or at least an explicit tradeoff note in the PR body. The body currently says "time neutral to better" and defers to the CI gate, which does not agree.
Earlier review points that still apply (non-blocking):
- Add an
unsafe.Sizeoftest pinning the 576 B size-class assumption. - Check whether
parseSelectColumnListstill has callers. - Add a test that parses more than 1024 elements and checks that
Resetdrops the oversized backing array. - The cap in
maxPooledScratchis counted in elements, not bytes.OrderByExpressionis larger thanast.Expression, so the order stack retains more bytes at the cap. - Note in the doc comments that a co-allocated block stays alive while any part of it is referenced.
Correctness. I found nothing new on the scratch-stack logic. scratchPop and scratchDrop zero what they release, and resetScratchSlice bounds the capacity.
· branch perf/w4-04-coalloc-small-slices
…elds in place [W4-04] Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 37s —— View job Review of PR #931 (head
|
…docs [W4-04] Adds TestScratchErrorPathsDropOwnRegion, which runs one builder directly with a sentinel below it on each stack and fails at the builder's own level, so an enclosing region cannot mask a missing drop. Covers all 18 drop sites including Oracle CURSOR, SQL Server OPENROWSET, JSON_OBJECT, XMLFOREST, MySQL JSON_TABLE, PostgreSQL NULLS/USING and ClickHouse WITH FILL; removing any one drop now fails. Rewrites the scratch.go header and pushSelectColumns doc to the explicit-drop contract (no defer in recursive builders). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 39s —— View job Review of PR #931 (head
|
…ut of line [W4-04] PoolGetPut/serial showed +3.76% on amd64: resetScratch was inlined into Reset with its generic slow path, growing Reset on the pool Get/Put path. Move the work to a noinline resetScratchSlow and fold the four checks into two. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 13s —— View job Review of PR #931 (head
|
…all-slices Brings in #926 (tokens built in place) and #930 (O(1) ReleaseAST). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
…e column-list split [W4-04] TestCoallocBlockSizes pins unsafe.Sizeof of the six co-allocation blocks and checks each still fits the malloc size class it was shaped for, so a field added to SelectStatement, TableReference or FunctionCall fails a test instead of silently moving a block up a class. The SELECT block comment now gives the real sizes (568 B in the 576 B class, not "exactly 576") and both block comments note the lifetime trade-off: keeping only Columns, From or Arguments pins the whole block. parseSelectColumnList says why parseSelectStatement calls pushSelectColumns directly. Comments and tests only: the linux/amd64 machine code of every GoSQLX function is byte-identical to the previous commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 16s —— View job Review of PR #931 (head
|
…-04] The entry claimed "no time regression". Five amd64 gate runs of the merged head show simple_select and subquery faster everywhere, other SELECT shapes mixed by runner, and INSERT/DELETE (same instructions as before) up to 5% slower on AMD Zen 5 runners only. Say so, and say why: zero-instruction controls move single benchmarks by similar amounts on the same gate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 15s —— View job Review of PR #931 (head
|
|
Holding this PR until the perf gate accounts for code-layout noise (the control runs 41543555385, 41545141354 and 41545145216 showed REGRESSION verdicts on changes that execute no new code). Gate fix is being done separately; this PR will be re-gated with the fixed gate before merge. |
Summary
PLAN W4-04 (pulled forward by C0 for aggregate; decision D10 option b).
pkg/sql/parser/scratch.go) and copied once into exact-size slices.FunctionCall(up to 3 args) shares one allocation with its argument array.SelectStatementshares one allocation withFrom[1]and, for up to 4 columns, itsColumnsarray. The blocks are sized against Go size classes (stmt + From[1] + Columns[2] is 568 B in the 576 B class);TestCoallocBlockSizespins all six block sizes.Reset.TestScratchErrorPathsDropOwnRegionfails if any of the 18 drop sites is removed.Columns,Fromor a call'sArgumentspins the whole block (up to 600 B).pkg/gosqlx/alloc_budget_test.golowered to the new counts.main(perf(tokenizer): build tokens in place instead of copying them by value [W2-10] #926, perf(ast): O(1) ReleaseAST for parser-built trees with no pooled draws [W4-01] #930); no conflicts.Behaviour: trees are identical (behaviour goldens unchanged). The only observable difference:
Columns,GroupBy,OrderByandFunctionCall.Argumentsnow havelen == capinstead of spare capacity, so anappendby a caller reallocates. Listed in CHANGELOG.Perf gate (amd64): what it shows
Wins that hold on every runner: allocs/op down on every SELECT shape (aggregate 18 -> 13, complex 50 -> 44, ParseRW 8-19% fewer), B/op down, instructions/op down 1-5% on every SELECT shape (cachegrind). Time:
simple_select-3.9..-5.4%,subquery-3.1..-6.2%,TransformParse/simple_select-2.4..-7.7% in all five runs.Not met: PLAN 5.3 strict rule. The PR-run sticky (head 156c47c, merge ref 96f3554, run 41538950257) is REGRESSION:
ParseRW/sqlite/medium+5.06% (p<0.001). Five gate runs of the same head (base fd7eb53, GOGC=off, -cpu 1, 10 rounds):ParseE2E/windowParseE2E/complexParseE2E/insertParseE2E/deleteParseRW/sqlite/mediumTransformParse/joinWhy (evidence, not a hypothesis):
Resetis instruction-identical to base on linux/amd64,Resetadds about 8 instructions, and cachegrind shows e2e/delete -0.08..+0.33% and insert +0.03..+0.06% across the five runs. allocs/op and B/op are unchanged.Reset(ff32af8's is smaller); every other function is byte-identical, shifted by 96 or -224 bytes.Parserby 48 B and nothing else gaveParseE2E/cte+3.14% (p=0.007) and delete -4.45% on Zen 5 (run 41543555385). Emitting one unused equality function, which shifts the tokenizer and most of the parser by 160 B (the shift this PR'sfunctionCall3causes), gave delete +3.26% (p<0.001) on Intel (run 41545141354) andParseRW/snowflake/medium+3.35% (p=0.005) on Zen 5 (run 41545145216). In total, 3 of 7 control runs report REGRESSION.Resetcheck cannot recover 5% from 8 instructions. A lazily allocated scratch pointer only changes the data layout.-ldflags=-funcalign=64on both arms (diagnostic only) removes the 32-byte phase lottery but not placement effects in general. ff32af8 passes (run 41539097186) and the current head still flagsParseE2E/join+3.36% on Zen 3 (run 41539093047).Test Plan
pkg/sql/parser/coalloc_test.go: scratch balance and zeroing after success and mid-list failures at several depths; Reset clears and bounds; len==cap and append independence; list lengths 0..7 across every co-allocation boundary; nested lists; pooled-parser reuse; newTestCoallocBlockSizespins the six block sizes and their size classes.task check(fmt, vet, golangci-lint, race tests incl. goldens): passtask test:cbinding: passgo test -count=1 ./pkg/compatibility/...: passgo run honnef.co/go/tools/cmd/staticcheck@latest ./pkg/sql/parser/ ./pkg/sql/ast/: cleanPerf-gate history
defer): ParseFresh +16..42%, PoolGetPut +6.7%. The defers grewparseFunctionCallandparseSelectStatementframes. Fixed by explicit unwinding and by moving the scratch fields to the end ofParser.resetScratchwas inlined intoResetwith its generic slow path. Fixed in ff32af8 with an out-of-line slow path.Breaking Changes
None to the exported API.
len == capon the slices named above (see Summary).🤖 Generated with Claude Code
https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2