Repository navigation
perf(fingerprint,formatter,transform): tokenize into the pooled scratch buffer [W5-01] - #903
Merged
Merged
Conversation
…ch buffer [W5-01] Normalize/Fingerprint, Formatter.Format and transform's parse helpers called the public TokenizeString, which copies every token into a fresh slice that the parser immediately copies again. They now use internal/tokscratch, the path gosqlx.Parse already uses: tokens live in the pooled tokenizer's buffer for the duration of the call. Each caller finishes with the tokens before the deferred PutTokenizer, the parser copies what the AST needs, and token strings are substrings of the input, so nothing borrowed outlives the call. Outputs are byte-identical. Tests: differential tests against the public tokenizer path over every statement in the real-world corpora; concurrency tests (8+ goroutines, -race) checked against a sequential run; pool-reuse tests that returned strings, ASTs, conditions and joins stay correct after later calls reuse the buffer. M4, GOGC=off -cpu 1, 10 interleaved rounds, the 11 compare queries: Normalize B/op -42% to -70%, Fingerprint -40% to -65%, TransformParse -49% to -78% (geomean -54% / -63%), allocs -5% to -29%. Time: no regression; the shared machine was too noisy for a time claim. 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 19s —— View job Review of PR #903: tokenize into the pooled scratch bufferVerdict: looks good. I have no blocking issues. I read the five production call-site changes and the Safety of the swap
Suggestions (non-blocking)
Tests: The differential tests against the old path over the realworld and corpus data, the The change is small and mechanical. The risk is limited to the lifetime rule above, and the call sites follow it. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
ajitpratap0
marked this pull request as ready for review
November 1, 2026 16:30
|
Claude finished @ajitpratap0's task in 23s —— View job Review of PR #903: tokenize into the pooled scratch bufferVerdict: looks good. I found no blocking issues. I read the diff for the three production files and the Production changes
Suggestions (non-blocking)
The differential, |
github-actions
Bot
added
the
documentation
Perf gate: PASS (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: 0 of 32 benchmarks moved more than 3%. Instructions per operation (cmd/perfdriver)
Secondary metric: default GC, GOMAXPROCS=1 (informational)
Environmentbenchstat (primary metric) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 14s —— View job Review of PR #903: tokenize into the pooled scratch bufferVerdict: looks good. I found no blocking issues. I read the diff for the three production files ( Safety of the swap
Suggestions (non-blocking)
The differential, |
…race test [W5-01] Add fingerprint.Normalize and transform ParseSQL cases to the perf gate suite, add a parallel gosqlx/formatter/fingerprint/transform race test on the shared tokenizer pool, and fix the CHANGELOG wording. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 17s —— View job Review of PR #903: tokenize into the pooled scratch bufferVerdict: looks good. I found no blocking issues. I read the diff of the production files ( Production changes
Gate suite change
Suggestions (non-blocking)
The differential, |
|
Claude finished @ajitpratap0's task in 16s —— View job Review of PR #903: tokenize into the pooled scratch bufferVerdict: looks good. I found no blocking issues. I read the diff of the production files ( Production changes
Perf gate (amd64, shadow mode)
Suggestions (non-blocking)
The differential, |
|
Claude finished @ajitpratap0's task in 17s —— View job Review of PR #903: tokenize into the pooled scratch bufferVerdict: looks good. I found no blocking issues. I read the diff of the five production call sites, Production changes
Perf gate (amd64, shadow mode)
Suggestions (non-blocking)
The differential, |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wave-1 task W5-01 from the 2026-11 performance audit (TOK-SCRATCH). The prototype was confirmed by an independent verifier. Codex ported it; Opus reviewed it and ran the full gate.
Five call sites used the public
TokenizeString, which copies every token into a fresh slice that the parser then copies again:fingerprint.Normalize(also used byFingerprint)Formatter.Formattransform.parseSQL,transform.ParseSQLandtransform.ParseSQLWithDialectThey now call
internal/tokscratch.String, the pathgosqlx.Parsealready uses. The tokens stay in the pooled tokenizer's buffer for the duration of the call.Why this is safe. The
tokscratchcontract says the slice and itsWordvalues belong to the tokenizer until it is reused, while token strings, being substrings of the input, stay valid. Each caller follows that contract:PutTokenizer. InFormat, that includesformatASTCheckedWithTokens.Outputs are byte-identical, and no exported API changes.
Status: ready for review. The wave-0 behaviour goldens (W0-02) and the perf gate (W0-09) are on
main; this branch has merged origin/main (cda05b2).Test Plan
testdata/realworld/*andtestdata/corpus/*.-race, with results checked against a sequential run.go run honnef.co/go/tools/cmd/staticcheck@lateston the three packagestask checktask test:cbindinggo test -count=1 ./pkg/compatibility/...A/B setup (re-measured after merging wave 0, 2026-11-01):
task bench:ab-equivalent (scripts/perf/ab.sh -r 10 -t 200ms) against origin/main 962b458, Apple M4,GOGC=off,-cpu 1, 10 interleaved ABBA rounds, wave-0 benchmark names, benchstat alpha 0.01. Shared machine (load ~5-7), so indicative only; the amd64 CI gate below is authoritative.Normalize/*(11)TransformParse/*(11)Render/*(control, untouched)Plan targets:
NormalizeB/op <= -40% (met),TransformParseB/op <= -45% (met). Time is not worse anywhere.Merge notes. Merged origin/main into the branch. One conflict:
pkg/fingerprint/fingerprint_bench_test.go, resolved by taking main's wave-0 benchmark matrix (BenchmarkNormalize,BenchmarkFingerprint,BenchmarkNormalizeScaleover the shared benchcorpus). The PR's ownpkg/transform/tokscratch_bench_test.goduplicated wave-0'sBenchmarkTransformParseand was removed. No golden changed. Added the CHANGELOG entry under## [Unreleased]/### Performance(moved there from the released 1.17.0 section after review).Gate on the merged head:
task check,task test:cbinding,go test -count=1 ./pkg/compatibility/...and staticcheck on the changed packages all pass.CI perf gate (amd64, shadow mode): PENDING on head d36d27d. This push adds
NormalizeandTransformParse(simple_select, join, complex) toscripts/perf/gate-suite.txt(ab.sh reads the suite from the head checkout), so the gate now measures this PR's targets. The gate table and verdict will be pasted here once the run completes. The earlier PASS (merge ref 743f682) did not cover these benchmarks.Review-fix additions (d36d27d): gate-suite lines for the targets;
TestCrossConsumerSharedTokenizerPool(gosqlx.Parse, Formatter.Format, fingerprint.Normalize, transform.ParseSQL in parallel on the shared tokenizer pool,-race); CHANGELOG wording. Re-ran on this head:task check,task test:cbinding,go test -count=1 ./pkg/compatibility/..., staticcheck on fingerprint/formatter/transform: all pass.Breaking Changes
None.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2