Skip to content

perf(parser): co-allocate small slices and size lists exactly [W4-04] - #931

Open
ajitpratap0 wants to merge 8 commits into
mainfrom
perf/w4-04-coalloc-small-slices
Open

ajitpratap0 wants to merge 8 commits into
mainfrom
perf/w4-04-coalloc-small-slices

Conversation

@ajitpratap0

@ajitpratap0 ajitpratap0 commented Nov 2, 2026 •

Copy link
Copy Markdown
Owner

Summary

PLAN W4-04 (pulled forward by C0 for aggregate; decision D10 option b).

  • Function arguments, the SELECT list, GROUP BY and ORDER BY are collected on parser-owned scratch stacks (pkg/sql/parser/scratch.go) and copied once into exact-size slices.
  • FunctionCall (up to 3 args) shares one allocation with its argument array. SelectStatement shares one allocation with From[1] and, for up to 4 columns, its Columns array. The blocks are sized against Go size classes (stmt + From[1] + Columns[2] is 568 B in the 576 B class); TestCoallocBlockSizes pins all six block sizes.
  • Scratch stacks are balanced by explicit drops before every error return (no defer in the recursive builders, to keep frames small and ParseFresh flat), zeroed on pop, and cleared and bounded (1024 elements) in Reset. TestScratchErrorPathsDropOwnRegion fails if any of the 18 drop sites is removed.
  • Lifetime trade-off (documented on the block types): a co-allocated block is one heap object, so keeping only Columns, From or a call's Arguments pins the whole block (up to 600 B).
  • Alloc budgets in pkg/gosqlx/alloc_budget_test.go lowered to the new counts.
  • Merged 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, OrderBy and FunctionCall.Arguments now have len == cap instead of spare capacity, so an append by 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):

benchmark 41538950257 Zen 5 (9V45) 41541295924 Zen 5 41541305262 Zen 5 41541300632 Zen 3 (7763) 41541309510 Intel 8573C
ParseE2E/window +2.93% (0.059) +0.76% (0.096) +4.47% (0.003) -0.63% (0.082) +0.66% (0.325)
ParseE2E/complex -0.78% (0.280) +1.53% (0.075) +2.22% (0.035) -0.86% (<0.001) +0.61% (0.315)
ParseE2E/insert +3.75% (0.052) +5.18% (0.001) +5.23% (0.002) -1.43% (<0.001) +1.69% (0.006)
ParseE2E/delete +0.83% (0.449) +3.76% (0.017) +3.90% (0.004) +1.50% (<0.001) +1.66% (0.035)
ParseRW/sqlite/medium +5.06% (<0.001) +4.57% (0.001) +3.11% (0.011) -0.43% (0.165) +1.14% (<0.001)
TransformParse/join +1.09% (0.043) -1.03% (1.000) +0.09% (0.677) +4.15% (<0.001) +1.12% (0.345)
geomean -0.17% +0.22% +0.29% -1.23% -0.99%

Why (evidence, not a hypothesis):

  • INSERT/UPDATE/DELETE execute the same machine code: every function on the delete path except Reset is instruction-identical to base on linux/amd64, Reset adds 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.
  • The original ff32af8 failure (window +4.47%, complex +3.16%, delete +6.15% on a Zen 5 runner, run 41512780007) does not reproduce: the same comparison (base 533148d, head ff32af8) re-run on a Zen 4 runner passes with window -0.21%, complex -3.19%, delete -0.45%, geomean -1.56% (run 41539101039). 5bd8130 and ff32af8 differ only in Reset (ff32af8's is smaller); every other function is byte-identical, shifted by 96 or -224 bytes.
  • Zero-instruction-change controls against the same base fail the same gate. Padding Parser by 48 B and nothing else gave ParseE2E/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's functionCall3 causes), gave delete +3.26% (p<0.001) on Intel (run 41545141354) and ParseRW/snowflake/medium +3.35% (p=0.005) on Zen 5 (run 41545145216). In total, 3 of 7 control runs report REGRESSION.
  • So the flagged rows are code and data placement effects. The gate's A/A cannot see them, because both of its arms are the same binary. GitHub runners land on four CPU models (EPYC 7763, 9V74, 9V45 and Xeon 8573C), and the flagged rows change from run to run. The one consistent pattern is INSERT and the sqlite corpus at about +5% on Zen 5. That is real for this binary on that CPU, but it comes from where unchanged code landed, not from added work.
  • The suggested data-side fixes are ruled out by the controls. The Parser-padding control did not reproduce insert/sqlite on Zen 5 (+0.27%/-0.21% and -1.42%/+0.28%), so cache lines or size class do not explain it. A cheaper Reset check cannot recover 5% from 8 instructions. A lazily allocated scratch pointer only changes the data layout.
  • -ldflags=-funcalign=64 on 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 flags ParseE2E/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; new TestCoallocBlockSizes pins the six block sizes and their size classes.
  • task check (fmt, vet, golangci-lint, race tests incl. goldens): pass
  • task test:cbinding: pass
  • go test -count=1 ./pkg/compatibility/...: pass
  • go run honnef.co/go/tools/cmd/staticcheck@latest ./pkg/sql/parser/ ./pkg/sql/ast/: clean
  • The comment/test commit 156c47c leaves the linux/amd64 machine code of every GoSQLX function byte-identical, and the CHANGELOG commit touches no code. The gate runs above therefore measure the final library code.

Perf-gate history

  • 9019d00 (scratch drops via defer): ParseFresh +16..42%, PoolGetPut +6.7%. The defers grew parseFunctionCall and parseSelectStatement frames. Fixed by explicit unwinding and by moving the scratch fields to the end of Parser.
  • 5bd8130: PoolGetPut/serial +3.76%. resetScratch was inlined into Reset with its generic slow path. Fixed in ff32af8 with an out-of-line slow path.
  • ff32af8: window/complex/delete +3..6% on a Zen 5 runner. That result does not reproduce (see above).

Breaking Changes

None to the exported API. len == cap on the slices named above (see Summary).

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

…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
@vercel

vercel Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
gosqlx-website Ready Ready Preview Nov 3, 2026 3:57am UTC

@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Claude finished @ajitpratap0's task in 47s —— View job


Review of PR #931: co-allocate small slices and size lists exactly

Verdict: 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 (task check, cbinding, compatibility, staticcheck).

Correctness (checked by reading)

  • Scratch stack discipline. Every user takes a mark and defers a drop, so error returns leave the stack balanced. scratchPop and scratchDrop zero what they release, and Reset clears and bounds the stacks. This keeps the [len:cap] zeroed invariant, so a pooled parser doesn't keep old ASTs or query text alive.
  • Slice aliasing across reallocation. p.exprScratch[argBase:argBase+argCount] and p.exprScratch[colBase:colEnd] are both re-sliced after the nested parsing. A nested push that grows the backing array can't leave them pointing at stale memory.
  • Nested lists. A nested list pushes above colEnd or argBase+argCount and pops back to its own mark. The parent's region is therefore intact when newFunctionCall or newSelectStatement copies it.
  • Function-call paths. The special-form path (SUBSTRING ... FROM, TRIM, etc.) keeps argCount = -1 and uses the arguments slice as before. The scratch path applies only to plain arguments. The empty-args case still gives Arguments == nil.
  • FROM buffer. buf[:0:1] capped at 1 means a second table append reallocates instead of writing into the neighbouring cols array in the co-allocated block. The n <= 2/n <= 4 blocks use b.cols[:n:n] for the same reason.
  • hasFrom guess. newSelectStatement takes hasFrom from p.isType(TokenTypeFrom) before parseFromClause runs. The worst case, where the guess disagrees with the parse, is an unused 144 B slot. A table list is always written through tables = buf[:0] plus append, so correctness doesn't depend on the guess.

Minor suggestions (non-blocking)

  1. Size-class assumption. The comment says SelectStatement is 392 B and TableReference is 144 B, giving exactly 576 B. Struct growth will silently shift the size classes. A cheap unsafe.Sizeof-based test could assert Sizeof(selectStmt2) == 576 so a future field addition shows up as a test failure rather than a quiet allocation regression. The alloc-budget test would catch some of this, but only for the specific queries it covers.
  2. parseSelectColumnList may be dead code. It is now a thin wrapper around pushSelectColumns. If it has no remaining callers, remove it. If it does, a one-line pointer to why parseSelectStatement doesn't use it would help.
  3. Behaviour change (documented). len == cap on Columns, GroupBy, OrderBy and FunctionCall.Arguments is listed in the CHANGELOG, which is good. Callers that did append(stmt.Columns, x) and relied on spare capacity to avoid copying now reallocate, but the result is still correct. Appending to one of these slices can no longer write into a neighbouring co-allocated field, which is the safe behaviour.
  4. Scratch cap of 1024. maxPooledScratch is measured in elements. OrderByExpression is larger than Expression (16 B), so the order stack can hold more bytes at the cap. This is probably fine, but a per-type byte cap would be more precise.
  5. Co-allocation lifetime. A co-allocated block stays alive as long as any part of it is referenced. Holding only stmt.Columns pins the whole 576 B statement, and holding only a FunctionCall's Arguments pins the call. This is a modest trade-off in return for fewer allocations. It's worth noting in the doc comments.

Tests
coalloc_test.go covers the right risk areas:

  • scratch balance after success and after mid-list failures at several nesting depths;
  • zeroing, and Reset bounding the stacks;
  • len == cap and append independence;
  • list lengths 0..7 across every co-allocation boundary;
  • reuse of a pooled parser not aliasing earlier trees.

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 Reset drops the oversized backing array. The PR says "Reset clears and bounds", so this may already be covered.

Performance and security
The reported allocation and bytes-per-op reductions are plausible given the design, and time is neutral to better. I see no security concerns: nothing is exposed, and zeroing on pop avoids retaining data across pooled parses.
· branch perf/w4-04-coalloc-small-slices

@github-actions github-actions Bot added documentation Improvements or additions to documentation parser labels Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Perf gate: REGRESSION (shadow mode: report only, this check never fails)

Base fd7eb538c2ef, head b71301a26212. 10 interleaved rounds, benchtime 200ms, -cpu 1, pinned to CPU 3 (SMT siblings 2-3) with taskset.

  • 1 benchmark(s) slower by 3% or more at p < 0.01
  • 3 benchmark(s) faster by 3% or more

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)

benchmark time p A/A B/op allocs/op status
ParseE2E/simple_select -4.38% <0.001 +0.19% -13.73% 5 → 3 faster, -allocs
ParseE2E/select_where -0.61% 0.028 +0.56% -3.08% 18 → 16 -allocs
ParseE2E/join +0.39% 0.393 +0.20% -2.03% 17 → 15 -allocs
ParseE2E/aggregate -2.93% 0.002 +0.28% -5.17% 18 → 13 -allocs
ParseE2E/subquery -6.80% <0.001 +0.20% -11.97% 14 → 10 faster, -allocs
ParseE2E/cte -4.96% 0.003 -0.82% -11.37% 21 → 16 faster, -allocs
ParseE2E/window -1.55% 0.002 +0.12% -3.66% 11 → 9 -allocs
ParseE2E/complex -1.17% <0.001 +0.03% -2.20% 50 → 44 -allocs
ParseE2E/insert -1.74% <0.001 -0.05% +0.00% 17 → 17 ok
ParseE2E/update -1.32% <0.001 -0.36% +0.00% 13 → 13 ok
ParseE2E/delete +0.70% 0.037 -0.21% +0.00% 5 → 5 ok
ParseRW/postgresql/medium +0.39% 0.684 -0.22% -2.10% 646 → 583 -allocs
ParseRW/mysql/medium -2.26% <0.001 -0.50% -3.83% 365 → 331 -allocs
ParseRW/mariadb/medium -0.54% 0.280 -0.37% -3.21% 523 → 465 -allocs
ParseRW/sqlserver/medium -0.42% 0.393 -0.30% -2.68% 928 → 854 -allocs
ParseRW/oracle/medium -0.83% 0.123 -0.23% -3.41% 1093 → 965 -allocs
ParseRW/sqlite/medium +0.60% 0.436 -0.10% -2.08% 346 → 317 -allocs
ParseRW/snowflake/medium -1.16% 0.043 -0.70% -5.21% 777 → 631 -allocs
ParseRW/clickhouse/medium -1.50% 0.054 -0.76% -4.92% 384 → 337 -allocs
ParseFresh/simple_select -1.40% 0.011 +0.02% -12.50% 6 → 4 -allocs
ParseFresh/join -0.73% 0.364 -0.79% -1.97% 18 → 16 -allocs
ParseFresh/complex +4.00% 0.571 +3.73% -2.17% 51 → 45 -allocs
PoolGetPut/serial -1.31% 0.075 +0.27% +0.00% 0 → 0 ok
Tokenize/scratch/join +0.17% 0.160 +0.24% +0.00% 0 → 0 ok
Tokenize/scratch/complex -0.13% 0.307 +0.14% +0.00% 0 → 0 ok
Tokenize/scratch/insert -0.21% 0.344 -0.11% +0.00% 0 → 0 ok
Normalize/simple_select -2.81% <0.001 -0.39% -12.50% 7 → 5 -allocs
Normalize/join +0.94% 0.003 -0.57% -1.75% 23 → 21 -allocs
Normalize/complex -1.11% 0.004 -0.32% -1.88% 69 → 63 -allocs
TransformParse/simple_select -2.32% <0.001 -0.13% -13.73% 5 → 3 -allocs
TransformParse/join +3.10% <0.001 -0.10% -2.03% 17 → 15 slower, -allocs
TransformParse/complex -0.49% 0.112 -0.20% -2.20% 50 → 44 -allocs
geomean -1.03% <0.001 -0.04%

A/A control: 1 of 32 benchmarks moved more than 3%.

Instructions per operation (cmd/perfdriver)

case instructions/op status
e2e/aggregate -3.52% ok
e2e/complex -1.16% ok
e2e/cte -3.31% ok
e2e/delete +0.19% ok
e2e/insert +0.04% ok
e2e/join -1.00% ok
e2e/select_where -1.45% ok
e2e/simple_select -5.32% ok
e2e/subquery -4.41% ok
e2e/update +0.09% ok
e2e/window -2.17% ok
rw/postgresql/medium -1.59% ok
Secondary metric: default GC, GOMAXPROCS=1 (informational)
benchmark time p
ParseE2E/simple_select -1.76% <0.001
ParseE2E/select_where +0.43% 0.910
ParseE2E/join -0.36% 0.739
ParseE2E/aggregate -0.12% 0.880
ParseE2E/subquery -1.53% 0.130
ParseE2E/cte +0.08% 0.579
ParseE2E/window -1.82% 0.082
ParseE2E/complex -0.63% 0.971
ParseE2E/insert -0.77% 0.650
ParseE2E/update +0.31% 0.579
ParseE2E/delete +1.25% 0.054
ParseRW/postgresql/medium +2.23% 0.089
ParseRW/mysql/medium +1.41% 0.247
ParseRW/mariadb/medium +1.12% 0.123
ParseRW/sqlserver/medium +0.56% 0.247
ParseRW/oracle/medium -0.52% 0.353
ParseRW/sqlite/medium +1.35% 0.089
ParseRW/snowflake/medium +2.22% 0.190
ParseRW/clickhouse/medium +0.83% 0.280
ParseFresh/simple_select -1.23% 0.015
ParseFresh/join +2.67% 0.005
ParseFresh/complex +1.78% 0.353
PoolGetPut/serial -1.28% 0.007
Tokenize/scratch/join -0.10% 0.195
Tokenize/scratch/complex -0.01% 0.850
Tokenize/scratch/insert -0.04% 0.307
Normalize/simple_select -0.92% 0.089
Normalize/join +1.04% 0.121
Normalize/complex +1.18% 0.165
TransformParse/simple_select -1.89% 0.019
TransformParse/join +0.21% 0.821
TransformParse/complex +0.78% 1.000
geomean +0.19%
Environment
date:        2026-11-03T03:58:02Z
base:        fd7eb538c2ef8b8ab62231ccac69fc4ddebf31ff
head:        working tree of b71301a262128a07d59abb68bacfe51c79f28f5b
go:          go version go1.26.1 linux/amd64
kernel:      Linux 7.0.0-1017-azure x86_64
pinning:     pinned to CPU 3 (SMT siblings 2-3) with taskset
clocksource: tsc
load:        1.55, 0.49, 0.18
instr count: cachegrind
perf probe:  Error: No supported events found. The instructions:u event is not supported. 

Architecture:                            x86_64
CPU(s):                                  4
Model name:                              AMD EPYC 7763 64-Core Processor
Thread(s) per core:                      2
Core(s) per socket:                      2
Socket(s):                               1
Hypervisor vendor:                       Microsoft
L1d cache:                               64 KiB (2 instances)
L1i cache:                               64 KiB (2 instances)
L2 cache:                                1 MiB (2 instances)
L3 cache:                                32 MiB (1 instance)
benchstat (primary metric)
goos: linux
goarch: amd64
pkg: github.com/ajitpratap0/GoSQLX/pkg/fingerprint
cpu: AMD EPYC 7763 64-Core Processor                
                        │    base     │                head                │
                        │   sec/op    │   sec/op     vs base               │
Normalize/simple_select   2.332µ ± 1%   2.267µ ± 1%  -2.81% (p=0.000 n=10)
Normalize/join            7.606µ ± 1%   7.678µ ± 1%  +0.94% (p=0.003 n=10)
Normalize/complex         18.66µ ± 1%   18.45µ ± 1%  -1.11% (p=0.004 n=10)
geomean                   6.917µ        6.848µ       -1.00%

                        │     base     │                 head                 │
                        │     B/op     │     B/op      vs base                │
Normalize/simple_select     896.0 ± 0%     784.0 ± 0%  -12.50% (p=0.000 n=10)
Normalize/join            2.680Ki ± 0%   2.633Ki ± 0%   -1.75% (p=0.000 n=10)
Normalize/complex         7.492Ki ± 0%   7.352Ki ± 0%   -1.88% (p=0.000 n=10)
geomean                   2.600Ki        2.456Ki        -5.51%

                        │    base    │                head                │
                        │ allocs/op  │ allocs/op   vs base                │
Normalize/simple_select   7.000 ± 0%   5.000 ± 0%  -28.57% (p=0.000 n=10)
Normalize/join            23.00 ± 0%   21.00 ± 0%   -8.70% (p=0.000 n=10)
Normalize/complex         69.00 ± 0%   63.00 ± 0%   -8.70% (p=0.000 n=10)
geomean                   22.31        18.77       -15.87%

pkg: github.com/ajitpratap0/GoSQLX/pkg/gosqlx
                          │    base     │                head                │
                          │   sec/op    │   sec/op     vs base               │
ParseE2E/simple_select      1.597µ ± 1%   1.527µ ± 1%  -4.38% (p=0.000 n=10)
ParseE2E/select_where       3.212µ ± 1%   3.193µ ± 1%       ~ (p=0.024 n=10)
ParseE2E/join               4.534µ ± 1%   4.551µ ± 1%       ~ (p=0.393 n=10)
ParseE2E/aggregate          3.382µ ± 0%   3.283µ ± 1%  -2.93% (p=0.001 n=10)
ParseE2E/subquery           2.947µ ± 1%   2.747µ ± 1%  -6.80% (p=0.000 n=10)
ParseE2E/cte                4.139µ ± 1%   3.934µ ± 1%  -4.96% (p=0.001 n=10)
ParseE2E/window             2.576µ ± 1%   2.536µ ± 1%  -1.55% (p=0.002 n=10)
ParseE2E/complex            11.87µ ± 1%   11.73µ ± 1%  -1.17% (p=0.000 n=10)
ParseE2E/insert             2.121µ ± 1%   2.084µ ± 1%  -1.74% (p=0.000 n=10)
ParseE2E/update             1.936µ ± 1%   1.910µ ± 1%  -1.32% (p=0.000 n=10)
ParseE2E/delete             1.213µ ± 6%   1.222µ ± 2%       ~ (p=0.034 n=10)
ParseRW/postgresql/medium   163.0µ ± 1%   163.6µ ± 1%       ~ (p=0.684 n=10)
ParseRW/mysql/medium        87.35µ ± 1%   85.38µ ± 1%  -2.26% (p=0.000 n=10)
ParseRW/mariadb/medium      124.5µ ± 1%   123.9µ ± 2%       ~ (p=0.280 n=10)
ParseRW/sqlserver/medium    267.7µ ± 1%   266.5µ ± 1%       ~ (p=0.393 n=10)
ParseRW/oracle/medium       322.0µ ± 1%   319.4µ ± 1%       ~ (p=0.123 n=10)
ParseRW/sqlite/medium       75.19µ ± 1%   75.63µ ± 1%       ~ (p=0.436 n=10)
ParseRW/snowflake/medium    178.6µ ± 1%   176.5µ ± 1%       ~ (p=0.043 n=10)
ParseRW/clickhouse/medium   84.76µ ± 1%   83.48µ ± 2%       ~ (p=0.050 n=10)
ParseFresh/simple_select    6.977µ ± 1%   6.880µ ± 2%  -1.40% (p=0.009 n=10)
ParseFresh/join             11.08µ ± 1%   11.00µ ± 1%       ~ (p=0.363 n=10)
ParseFresh/complex          19.31µ ± 5%   20.09µ ± 8%       ~ (p=0.565 n=10)
PoolGetPut/serial           67.31n ± 4%   66.42n ± 6%       ~ (p=0.072 n=10)
geomean                     11.51µ        11.36µ       -1.31%

                          │     base     │                head                 │
                          │     B/s      │     B/s       vs base               │
ParseE2E/simple_select      15.53Mi ± 1%   16.24Mi ± 1%  +4.57% (p=0.000 n=10)
ParseE2E/select_where       30.58Mi ± 1%   30.77Mi ± 1%       ~ (p=0.030 n=10)
ParseE2E/join               26.51Mi ± 1%   26.41Mi ± 1%       ~ (p=0.393 n=10)
ParseE2E/aggregate          23.68Mi ± 0%   24.40Mi ± 1%  +3.02% (p=0.001 n=10)
ParseE2E/subquery           26.54Mi ± 1%   28.47Mi ± 1%  +7.30% (p=0.000 n=10)
ParseE2E/cte                28.57Mi ± 1%   30.06Mi ± 1%  +5.24% (p=0.001 n=10)
ParseE2E/window             34.81Mi ± 1%   35.35Mi ± 1%  +1.53% (p=0.002 n=10)
ParseE2E/complex            30.36Mi ± 1%   30.73Mi ± 1%  +1.21% (p=0.000 n=10)
ParseE2E/insert             34.18Mi ± 1%   34.78Mi ± 1%  +1.76% (p=0.000 n=10)
ParseE2E/update             27.09Mi ± 1%   27.46Mi ± 0%  +1.34% (p=0.000 n=10)
ParseE2E/delete             40.88Mi ± 5%   40.60Mi ± 2%       ~ (p=0.035 n=10)
ParseRW/postgresql/medium   22.02Mi ± 1%   21.93Mi ± 1%       ~ (p=0.698 n=10)
ParseRW/mysql/medium        30.98Mi ± 1%   31.70Mi ± 1%  +2.31% (p=0.000 n=10)
ParseRW/mariadb/medium      28.47Mi ± 1%   28.62Mi ± 2%       ~ (p=0.280 n=10)
ParseRW/sqlserver/medium    24.86Mi ± 1%   24.97Mi ± 1%       ~ (p=0.382 n=10)
ParseRW/oracle/medium       26.03Mi ± 1%   26.25Mi ± 1%       ~ (p=0.123 n=10)
ParseRW/sqlite/medium       27.47Mi ± 1%   27.31Mi ± 1%       ~ (p=0.447 n=10)
ParseRW/snowflake/medium    28.47Mi ± 1%   28.81Mi ± 1%       ~ (p=0.043 n=10)
ParseRW/clickhouse/medium   26.58Mi ± 1%   26.99Mi ± 2%       ~ (p=0.050 n=10)
geomean                     27.59Mi        28.04Mi       +1.63%

                          │      base      │                  head                  │
                          │      B/op      │     B/op      vs base                  │
ParseE2E/simple_select        816.0 ± 0%       704.0 ± 0%  -13.73% (p=0.000 n=10)
ParseE2E/select_where       1.523Ki ± 0%     1.477Ki ± 0%   -3.08% (p=0.000 n=10)
ParseE2E/join               2.305Ki ± 0%     2.258Ki ± 0%   -2.03% (p=0.000 n=10)
ParseE2E/aggregate          1.812Ki ± 0%     1.719Ki ± 0%   -5.17% (p=0.000 n=10)
ParseE2E/subquery           1.828Ki ± 0%     1.609Ki ± 0%  -11.97% (p=0.000 n=10)
ParseE2E/cte                2.336Ki ± 0%     2.070Ki ± 0%  -11.37% (p=0.000 n=10)
ParseE2E/window             1.281Ki ± 0%     1.234Ki ± 0%   -3.66% (p=0.000 n=10)
ParseE2E/complex            6.399Ki ± 0%     6.259Ki ± 0%   -2.20% (p=0.000 n=10)
ParseE2E/insert               824.0 ± 0%       824.0 ± 0%        ~ (p=1.000 n=10) ¹
ParseE2E/update               736.0 ± 0%       736.0 ± 0%        ~ (p=1.000 n=10) ¹
ParseE2E/delete               400.0 ± 0%       400.0 ± 0%        ~ (p=1.000 n=10) ¹
ParseRW/postgresql/medium   70.51Ki ± 0%     69.03Ki ± 0%   -2.10% (p=0.000 n=10)
ParseRW/mysql/medium        43.73Ki ± 0%     42.05Ki ± 0%   -3.83% (p=0.000 n=10)
ParseRW/mariadb/medium      64.84Ki ± 0%     62.76Ki ± 0%   -3.21% (p=0.000 n=10)
ParseRW/sqlserver/medium    90.49Ki ± 0%     88.07Ki ± 0%   -2.68% (p=0.000 n=10)
ParseRW/oracle/medium       113.8Ki ± 0%     109.9Ki ± 0%   -3.41% (p=0.000 n=10)
ParseRW/sqlite/medium       39.89Ki ± 0%     39.06Ki ± 0%   -2.08% (p=0.000 n=10)
ParseRW/snowflake/medium    83.38Ki ± 0%     79.04Ki ± 0%   -5.21% (p=0.000 n=10)
ParseRW/clickhouse/medium   37.13Ki ± 0%     35.30Ki ± 0%   -4.92% (p=0.000 n=10)
ParseFresh/simple_select      896.0 ± 0%       784.0 ± 0%  -12.50% (p=0.000 n=10)
ParseFresh/join             2.383Ki ± 0%     2.336Ki ± 0%   -1.97% (p=0.000 n=10)
ParseFresh/complex          6.477Ki ± 0%     6.336Ki ± 0%   -2.17% (p=0.000 n=10)
PoolGetPut/serial             0.000 ± 0%       0.000 ± 0%        ~ (p=1.000 n=10) ¹
geomean                                  ²                  -4.32%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                          │     base      │                 head                 │
                          │   allocs/op   │ allocs/op   vs base                  │
ParseE2E/simple_select       5.000 ± 0%     3.000 ± 0%  -40.00% (p=0.000 n=10)
ParseE2E/select_where        18.00 ± 0%     16.00 ± 0%  -11.11% (p=0.000 n=10)
ParseE2E/join                17.00 ± 0%     15.00 ± 0%  -11.76% (p=0.000 n=10)
ParseE2E/aggregate           18.00 ± 0%     13.00 ± 0%  -27.78% (p=0.000 n=10)
ParseE2E/subquery            14.00 ± 0%     10.00 ± 0%  -28.57% (p=0.000 n=10)
ParseE2E/cte                 21.00 ± 0%     16.00 ± 0%  -23.81% (p=0.000 n=10)
ParseE2E/window             11.000 ± 0%     9.000 ± 0%  -18.18% (p=0.000 n=10)
ParseE2E/complex             50.00 ± 0%     44.00 ± 0%  -12.00% (p=0.000 n=10)
ParseE2E/insert              17.00 ± 0%     17.00 ± 0%        ~ (p=1.000 n=10) ¹
ParseE2E/update              13.00 ± 0%     13.00 ± 0%        ~ (p=1.000 n=10) ¹
ParseE2E/delete              5.000 ± 0%     5.000 ± 0%        ~ (p=1.000 n=10) ¹
ParseRW/postgresql/medium    646.0 ± 0%     583.0 ± 0%   -9.75% (p=0.000 n=10)
ParseRW/mysql/medium         365.0 ± 0%     331.0 ± 0%   -9.32% (p=0.000 n=10)
ParseRW/mariadb/medium       523.0 ± 0%     465.0 ± 0%  -11.09% (p=0.000 n=10)
ParseRW/sqlserver/medium     928.0 ± 0%     854.0 ± 0%   -7.97% (p=0.000 n=10)
ParseRW/oracle/medium       1093.0 ± 0%     965.0 ± 0%  -11.71% (p=0.000 n=10)
ParseRW/sqlite/medium        346.0 ± 0%     317.0 ± 0%   -8.38% (p=0.000 n=10)
ParseRW/snowflake/medium     777.0 ± 0%     631.0 ± 0%  -18.79% (p=0.000 n=10)
ParseRW/clickhouse/medium    384.0 ± 0%     337.0 ± 0%  -12.24% (p=0.000 n=10)
ParseFresh/simple_select     6.000 ± 0%     4.000 ± 0%  -33.33% (p=0.000 n=10)
ParseFresh/join              18.00 ± 0%     16.00 ± 0%  -11.11% (p=0.000 n=10)
ParseFresh/complex           51.00 ± 0%     45.00 ± 0%  -11.76% (p=0.000 n=10)
PoolGetPut/serial            0.000 ± 0%     0.000 ± 0%        ~ (p=1.000 n=10) ¹
geomean                                 ²               -14.55%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                          │    base     │                head                │
                          │  sec/stmt   │  sec/stmt    vs base               │
ParseRW/postgresql/medium   13.58µ ± 1%   13.63µ ± 1%       ~ (p=0.684 n=10)
ParseRW/mysql/medium        10.92µ ± 1%   10.67µ ± 1%  -2.26% (p=0.000 n=10)
ParseRW/mariadb/medium      13.84µ ± 1%   13.76µ ± 2%       ~ (p=0.271 n=10)
ParseRW/sqlserver/medium    12.75µ ± 1%   12.69µ ± 1%       ~ (p=0.393 n=10)
ParseRW/oracle/medium       13.42µ ± 1%   13.31µ ± 1%       ~ (p=0.123 n=10)
ParseRW/sqlite/medium       12.53µ ± 1%   12.61µ ± 1%       ~ (p=0.436 n=10)
ParseRW/snowflake/medium    13.74µ ± 1%   13.58µ ± 1%       ~ (p=0.043 n=10)
ParseRW/clickhouse/medium   12.11µ ± 1%   11.93µ ± 2%       ~ (p=0.050 n=10)
geomean                     12.82µ        12.73µ       -0.72%

pkg: github.com/ajitpratap0/GoSQLX/pkg/sql/tokenizer
                         │    base     │                head                │
                         │   sec/op    │   sec/op     vs base               │
Tokenize/scratch/join      1.470µ ± 0%   1.472µ ± 1%       ~ (p=0.157 n=10)
Tokenize/scratch/complex   3.930µ ± 1%   3.925µ ± 0%       ~ (p=0.305 n=10)
Tokenize/scratch/insert    736.6n ± 1%   735.1n ± 0%       ~ (p=0.343 n=10)
geomean                    1.620µ        1.619µ       -0.06%

                         │     base     │                head                 │
                         │     B/s      │     B/s       vs base               │
Tokenize/scratch/join      81.75Mi ± 0%   81.62Mi ± 1%       ~ (p=0.159 n=10)
Tokenize/scratch/complex   91.72Mi ± 1%   91.84Mi ± 0%       ~ (p=0.305 n=10)
Tokenize/scratch/insert    98.40Mi ± 1%   98.61Mi ± 0%       ~ (p=0.343 n=10)
geomean                    90.36Mi        90.42Mi       +0.06%

                         │     base     │                head                 │
                         │     B/op     │    B/op     vs base                 │
Tokenize/scratch/join      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Tokenize/scratch/complex   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Tokenize/scratch/insert    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                               ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                         │     base     │                head                 │
                         │  allocs/op   │ allocs/op   vs base                 │
Tokenize/scratch/join      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Tokenize/scratch/complex   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Tokenize/scratch/insert    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                               ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

pkg: github.com/ajitpratap0/GoSQLX/pkg/transform
                             │    base     │                head                │
                             │   sec/op    │   sec/op     vs base               │
TransformParse/simple_select   1.571µ ± 1%   1.535µ ± 1%  -2.32% (p=0.000 n=10)
TransformParse/join            4.904µ ± 1%   5.056µ ± 2%  +3.10% (p=0.000 n=10)
TransformParse/complex         13.09µ ± 1%   13.02µ ± 1%       ~ (p=0.109 n=10)
geomean                        4.654µ        4.657µ       +0.07%

                             │     base     │                 head                 │
                             │     B/op     │     B/op      vs base                │
TransformParse/simple_select     816.0 ± 0%     704.0 ± 0%  -13.73% (p=0.000 n=10)
TransformParse/join            2.305Ki ± 0%   2.258Ki ± 0%   -2.03% (p=0.000 n=10)
TransformParse/complex         6.398Ki ± 0%   6.258Ki ± 0%   -2.20% (p=0.000 n=10)
geomean                        2.273Ki        2.134Ki        -6.15%

                             │    base    │                head                │
                             │ allocs/op  │ allocs/op   vs base                │
TransformParse/simple_select   5.000 ± 0%   3.000 ± 0%  -40.00% (p=0.000 n=10)
TransformParse/join            17.00 ± 0%   15.00 ± 0%  -11.76% (p=0.000 n=10)
TransformParse/complex         50.00 ± 0%   44.00 ± 0%  -12.00% (p=0.000 n=10)
geomean                        16.20        12.56       -22.48%

Run, raw data and benchstat

…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
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Claude finished @ajitpratap0's task in 37s —— View job


Review update for PR #931 (perf gate run on head fe02ab43)

I read the perf-gate report and scratch.go. I did not run benchmarks or tests here, so the cause below is a hypothesis.

The gate flags a real regression in a case the PR description doesn't cover.

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, parseFunctionCall and parseFromClause. The structs sized to size classes are large for a stack frame (a 392 B SelectStatement plus a TableReference plus columns). If any is built on the stack before escaping, or if the generic scratchPush[T] and scratchPop[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 more morestack copy.
  • Related symptoms.
    • The commit "unwind scratch on error paths instead of defer to keep frames small" shows you already suspected frame size.
    • PoolGetPut/serial is 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 ParseRW is +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=-m and look at the newSelectStatement and newFunctionCall block structs. Check that they are heap-allocated in an out-of-line //go:noinline helper and not constructed in the caller's frame.
    • Add //go:noinline to the cold n <= 2 / n <= 4 branches, or split them into per-size helpers, so the caller only holds a pointer.
    • Confirm with go test -bench ParseFresh -count=10 locally, and compare GODEBUG=... stack-growth counts if needed.

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):

  1. Add an unsafe.Sizeof test pinning the 576 B size-class assumption.
  2. Check whether parseSelectColumnList still has callers.
  3. Add a test that parses more than 1024 elements and checks that Reset drops the oversized backing array.
  4. The cap in maxPooledScratch is counted in elements, not bytes. OrderByExpression is larger than ast.Expression, so the order stack retains more bytes at the cap.
  5. 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
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Claude finished @ajitpratap0's task in 37s —— View job


Review of PR #931 (head 3e91cf7)

I read the diff and scratch.go. I did not run tests or benchmarks. I'm relying on the PR's reported results and the CI perf-gate comment.

Verdict: no blocking issues.

Correctness

  • Scratch discipline.
    • The parseFunctionCall loop calls exprDrop(argBase) on every error return, and the success path copies out and then drops. The ORDER BY, GROUP BY and SELECT-list paths do the same.
    • Pop and drop both zero what they release, and Reset bounds the stacks.
    • The slice p.exprScratch[argBase:] is re-read after the nested parses, so growth of the backing array can't leave it stale.
  • Co-allocation.
    • From is capped at 1 (b.from[:0:1]) and Columns at n:n. An append therefore reallocates and cannot spill into a neighbouring field.
    • A wrong hasFrom guess only wastes a 144 B slot. parseTableListAndJoins does buf[:0] followed by append, so correctness doesn't depend on the guess.
  • Special-form functions. They keep the old arguments path. A call with no arguments still gets a nil Arguments.

Perf gate

  • The earlier Claude comment about a ParseFresh regression of +16% to +42% was for the older head fe02ab43. It is stale.
  • The latest gate (head 75b2f44) shows ParseFresh/simple_select -2.5%, join flat and complex flat. The frame-size follow-up commits (5008bdc, 3e91cf7) appear to have fixed it.
  • Instructions/op are down 1–5% on every query that changed.
  • The remaining "slower" flags are PoolGetPut/serial (+4.8%) and ParseE2E/delete (+2.8%), neither of which this PR changes. PoolGetPut does no parsing and delete has an identical alloc count. This is most likely code-layout noise.
  • The gate is only in shadow mode, so I'd re-run it once on the final head to confirm.
  • The PR body says "time neutral to better", which matches the latest data.

Non-blocking suggestions

  1. Pin the 576 B size-class assumption. pkg/sql/ast/struct_size_test.go already pins SelectStatement at 392 B and TableReference at 144 B. Add a one-line unsafe.Sizeof(selectStmt2{}) == 576 check in coalloc_test.go, so a struct growth that breaks the exact fit shows up as a test failure.
  2. parseSelectColumnList is not dead. It is still called from dml_common.go:421 and :454. A one-line comment saying why parseSelectStatement uses pushSelectColumns directly would help.
  3. Cap in elements, not bytes. maxPooledScratch is 1024 elements. The order stack holds a larger element type, so it can retain more bytes at the cap. This is minor.
  4. Add a test that Reset drops an oversized backing array. Parse a list of more than 1024 items, call Reset, and assert cap == 0 or cap <= 1024.
  5. Document the lifetime trade-off. A co-allocated block stays alive while any part of it is referenced. Holding only stmt.Columns pins the whole 576 B statement, and holding only a call's Arguments pins the call. A note in the doc comments would be enough.

Tests
coalloc_test.go covers the main risks: balance after errors at several nesting depths, zeroing, len == cap, list lengths 0..7 across the co-allocation boundaries, and pooled-parser reuse. The CHANGELOG entry covers the len == cap behaviour change.
· branch perf/w4-04-coalloc-small-slices

…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
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Claude finished @ajitpratap0's task in 39s —— View job


Review of PR #931 (head 5bd8130)

I read the diff and scratch.go. I did not run tests or benchmarks, so I'm relying on the PR's reported results and the CI perf-gate.

Verdict: no blocking issues.

Correctness

  • Scratch discipline.
    • Builders drop their region before every error return and move the list into the node as soon as it is complete.
    • scratchPop and scratchDrop clear what they release. Reset bounds the capacity and clears leftovers, so [len:cap] stays zeroed and a pooled parser keeps no old ASTs alive.
    • The stack slices are re-read after nested parses, so growth of the backing array can't leave them stale.
  • Co-allocation.
    • From is capped at 1 and Columns at n:n, so an append reallocates instead of writing into a neighbouring field.
    • A wrong hasFrom guess only wastes one TableReference slot.
  • Error paths.
    • The defer calls were replaced with explicit drops, which matches the stack-growth fix.
    • The one remaining deferred drop is in the cold parseSelectColumnList.
    • The "no defer" contract in the scratch.go header is documented. Any new return path in these builders must follow it, and the added guard tests cover that.

Performance

  • The CI perf-gate on the final head passes. Allocs/op and B/op are down across the board, and instructions/op are down 1–5%.
  • The suite geomean is -1.41%.
  • The earlier ParseFresh regression came from frame growth. It is fixed by the explicit unwinding and by moving the scratch fields to the end of Parser.

Non-blocking suggestions

  1. Pin the size class. Add unsafe.Sizeof(selectStmt2{}) == 576 to coalloc_test.go. pkg/sql/ast/struct_size_test.go already pins the component sizes. A pin on the combined block would turn a silent allocation regression into a test failure.
  2. Explain the parseSelectColumnList split. It is still called from dml_common.go. A one-line comment saying why parseSelectStatement calls pushSelectColumns directly would help.
  3. Cap in bytes. maxPooledScratch counts elements. OrderByExpression is larger than ast.Expression, so the order stack retains more bytes at the cap. This is minor.
  4. Oversized-array test. Parse a list of more than 1024 items and check that Reset drops the oversized backing array. coalloc_test.go:108-109 appears to cover this already, so this may be done.
  5. Document the lifetime trade-off. A co-allocated block stays alive while any part of it is referenced. Holding only stmt.Columns pins the whole ~576 B statement. A note in the doc comments would be enough.

Tests
coalloc_test.go covers balance after errors at several nesting depths, zeroing, Reset bounds, len == cap and append independence, list lengths 0..7 across the co-allocation boundaries, and pooled-parser reuse. The behaviour change (len == cap) is listed in the CHANGELOG.

Security: no concerns.
· branch perf/w4-04-coalloc-small-slices

…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
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Claude finished @ajitpratap0's task in 13s —— View job


Review of PR #931 (head ff32af8)

I read the diff and scratch.go. I did not run tests or benchmarks, so I'm relying on the PR's reported results and the CI perf gate.

Verdict: no blocking issues.

Correctness

  • Scratch discipline.
    • Builders drop their region before every error return and move the list into the node as soon as it is complete.
    • scratchPop and scratchDrop clear what they release.
    • resetScratch bounds capacity and clears leftovers, so [len:cap] stays zeroed and a pooled parser keeps no old ASTs alive.
  • resetScratch condition. The newest commit ORs the lengths and the capacities into two branches. The slow path re-checks each stack, so the OR is conservative and cannot skip a needed reset. Capacities of 0 and 1024 go down the fast path correctly.
  • Co-allocation. From is capped at 1 and Columns at n:n. An append therefore reallocates and cannot write into a neighbouring field. A wrong hasFrom guess only wastes one TableReference slot.

Performance

  • The last full gate I can see shows suite geomean -1.41%, and allocs/op and B/op are down everywhere. Instructions/op are down 1–5%.
  • The earlier ParseFresh regression was frame growth, and the explicit unwinding fixed it.
  • The PoolGetPut/serial and ParseE2E/delete flags in the shadow-mode run look like layout noise. Neither benchmark's alloc count changed.
  • The newest commit touches Reset, which is on the PoolGetPut path. It's worth confirming PoolGetPut/serial on the final head.

Non-blocking suggestions

  1. Pin the size class. Add an unsafe.Sizeof check on the combined block (576 B) in coalloc_test.go. A future field addition would then fail a test instead of quietly costing an allocation.
  2. Explain the parseSelectColumnList split. It is still used by the cold OUTPUT/RETURNING paths in dml_common.go. A one-line comment on why parseSelectStatement calls pushSelectColumns directly would help.
  3. Cap in bytes. maxPooledScratch counts elements. OrderByExpression is larger than ast.Expression, so the order stack retains more bytes at the cap. This is minor.
  4. Document the lifetime trade-off. A co-allocated block stays alive while any part of it is referenced. Holding only stmt.Columns pins the whole statement. A note in the doc comments would be enough.

Tests
coalloc_test.go covers balance after errors at several nesting depths, zeroing, Reset bounds, len == cap and append independence, list lengths 0..7 across the boundaries, and pooled-parser reuse. The guard test for the 18 drop sites is a good safeguard for the "no defer" contract. The len == cap behaviour change is in the CHANGELOG.

Security: no concerns.
· branch perf/w4-04-coalloc-small-slices

ajitpratap0 and others added 2 commits November 3, 2026 04:07
…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
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Claude finished @ajitpratap0's task in 16s —— View job


Review of PR #931 (head 156c47c)

I read the latest commit's diff. I did not run tests or benchmarks. I'm relying on the PR's reported results and the earlier perf-gate comments.

Verdict: no blocking issues.

This commit (tests and comments only)

  • TestCoallocBlockSizes pins unsafe.Sizeof for all six blocks (536, 568, 600, 200, 216, 232 bytes). It also checks that each block stays in its intended malloc size class. This covers the size-class pin suggested in earlier reviews.
  • The SELECT block comment now says 568 B in the 576 B class, not "exactly 576". That is more accurate.
  • The lifetime trade-off is documented in both the SELECT and FunctionCall block comments.
  • parseSelectColumnList now has a comment explaining why parseSelectStatement calls pushSelectColumns directly.
  • The commit message says the amd64 machine code is byte-identical to the previous commit. That fits a change that touches only comments and tests, so the earlier perf-gate results still apply.

Remaining nits (non-blocking)

  1. Hard-coded sizes. The test hard-codes 64-bit sizes, so want values will differ on 32-bit targets. If CI or users run GOARCH=386 or arm, either skip the test with if unsafe.Sizeof(uintptr(0)) != 8 { t.Skip() } or guard it with a build constraint. The -race and cbinding runs on 64-bit are unaffected.
  2. Failure message. When c.size != c.want fires, the size-class check usually fires as well. That is harmless. Consider a continue after the first error to keep the output to one line per case.
  3. Scratch cap in elements. maxPooledScratch still counts elements, not bytes, so the order stack can retain more bytes at the cap. This is minor.
  4. Perf gate on the final head. The latest shadow-mode report was for a different head, and it showed ParseE2E/window, complex and delete as slower. I can't tell from here whether that is noise. The delete benchmark has an unchanged alloc count, and it uses neither FunctionCall nor the SELECT blocks. Instructions/op are down on every query that changed. It's worth a re-run on this head. Since it's shadow mode, it doesn't block the merge.

Correctness, tests and security

  • I found nothing new on scratch-stack discipline, the len == cap behaviour change (listed in the CHANGELOG), or co-allocation aliasing.
  • Test coverage is solid: balance after errors, zeroing, Reset bounds, list lengths across every block boundary, pooled-parser reuse, and the drop-site guard test.
  • I have no security concerns.
    · branch perf/w4-04-coalloc-small-slices

…-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
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Claude finished @ajitpratap0's task in 15s —— View job


Review of PR #931 (head ef4c6a3)

I read the diff stat and the size-pin test. I did not run tests or benchmarks. I'm relying on the PR's reported results and the earlier perf-gate comments.

Verdict: no blocking issues.

Since the last review

  • The latest commits touch only the CHANGELOG, comments and tests. TestCoallocBlockSizes (pkg/sql/parser/coalloc_test.go:129-138) now pins all six block sizes and their malloc size classes. This covers the size-class pin suggested earlier.
  • Lifetime and the parseSelectColumnList split are now documented.

Remaining nits (non-blocking)

  1. 32-bit targets. TestCoallocBlockSizes hard-codes 64-bit sizes. Add if unsafe.Sizeof(uintptr(0)) != 8 { t.Skip(...) } if GOARCH=386 or arm is ever tested.
  2. Duplicate failures. A size mismatch usually also fails the size-class check. A continue after the first error would keep the output to one line per case.
  3. Scratch cap counts elements, not bytes. maxPooledScratch is 1024 elements. The order stack holds a larger element type, so it can retain more bytes at the cap. This is minor.
  4. Perf gate on the final head. The last shadow-mode report was on a different head. It showed sqlite/medium at +5% (p<0.001) and PoolGetPut and delete as neutral, all with unchanged or reduced allocs. The default-GC secondary metric is mildly positive (geomean +1.07%), though informational. The gate is shadow-only, so it doesn't block the merge. The PR notes the sticky comment on ef4c6a3 is authoritative, so confirm it shows no regression on ParseRW/sqlite.

Correctness, tests and security

  • I found nothing new on scratch discipline, co-allocation aliasing or the documented len == cap change.
  • Test coverage is good. It covers balance after errors, zeroing, Reset bounds, list lengths across the block boundaries, pooled-parser reuse, and the drop-site guard test.
  • I have no security concerns.
    · branch perf/w4-04-coalloc-small-slices

@ajitpratap0

Copy link
Copy Markdown
Owner Author

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.

This branch was successfully deployed

1 active deployment
Preview — ef4c6a38 Deployed Nov 3, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation parser

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant