Repository navigation
perf(parser): currentToken as a pointer into p.tokens with a per-parser EOF sentinel [W3-02] - #922
Open
ajitpratap0 wants to merge 5 commits into
Open
ajitpratap0 wants to merge 5 commits into
ajitpratap0 wants to merge 5 commits into
Conversation
…er EOF sentinel [W3-02]
advance copied a 72-byte pointerful TokenWithSpan into the Parser on every
token, paying a GC write barrier per pointer word and a memmove. currentToken
now points at p.tokens[currentPos], or at the parser's own zero EOF sentinel
past the end of the stream (decision D4 option b), and peekToken returns a
pointer. Every parse entry point, advance, seek, Reset and Release set the
pointer, so a zero-value &Parser{} stays usable.
Parse results, errors and positions are unchanged: the behaviour goldens pass
and a recovery-path differential over the corpus, every dialect and truncated
variants was byte-identical. New tests pin the cursor invariant, that the
sentinel is per parser and never written, and the zero-value parser.
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.
|
github-actions
Bot
added
documentation
|
Claude finished @ajitpratap0's task in 27s —— View job Review of PR #922:
|
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) |
… over the full corpus [W3-02] Require currentToken == &p.eof after Reset and Release, cover an oversized tokenBuf, the pool New init, peekToken sentinel identity and the empty ParseContextFromModelTokens stream, and drive the sentinel-unmodified check over the W0-02 corpus for every dialect through all three entry points. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
…ams in sentinel corpus test [W3-02] Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 28s —— View job Review of PR #922:
|
|
Claude finished @ajitpratap0's task in 21s —— View job Review of PR #922:
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 18s —— View job Review of PR #922:
|
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
PLAN W3-02 (PC-3 = ADV-1 = MEM-5), decision D4 option (b).
Parser.currentTokenis now*models.TokenWithSpanpointing atp.tokens[currentPos], instead of a 72-byte pointerful copy made on everyadvance/seek(each copy paid a GC write barrier per pointer word plus a memmove).eof models.TokenWithSpan, never a package variable. A stray write can therefore neither race across goroutines nor leak between parsers.peekTokenreturns a pointer (intop.tokens, or&p.eofpast the end).&Parser{}stays usable: every parse entry point (parseTokens,parseContextTokens,parseWithRecovery, including the empty-stream case),advance,seek,Reset,Release,NewParserand the poolNewpointcurrentTokenat&p.eofwhen no real token applies.nextAdjacentcomparesEnd/Startvalues instead of copying tokens;writeRawLeafpasses the pointer.savedTok := p.currentToken) now save a pointer, which is equivalent becausep.tokensis never written during a parse.[Unreleased]entry added.Behaviour: no golden diff. Beyond the W0-02 goldens, a throwaway differential (not committed) dumped, for every corpus statement x every dialect x truncated prefixes (about 618 MB), the output of
ParseFromModelTokens(SQL(), error, all positions),ParseWithRecoveryFromModelTokens(statements and errors) andParseContextFromModelTokenson origin/main and on this branch: byte-identical.Test Plan
Tests written first (
pkg/sql/parser/currenttoken_pointer_test.go):TestCurrentToken_TracksCursor:currentToken == &tokens[currentPos](or&p.eof) across advance, seek (including negative), and save/restore, for a corpus of 20 inputs incl. error and empty inputs.TestCurrentToken_PeekToken,TestCurrentToken_NextAdjacent(pins the rewritten helper).TestCurrentToken_SentinelUnmodifiedAfterParsing(the plan's sentinel-unmodified test): all three entry points, error and recovery paths, pooled parser reused across rounds andReset;p.eofmust stay the zero token.currentTokenat&p.eof(TestCurrentToken_ReleaseRepointsSentinel,TestCurrentToken_ResetDropsOversizedBuffer, oversized tokenBuf and caller-owned slice); poolNewinit andpeekTokensentinel identity asserted; emptyParseContextFromModelTokensstream covered.TestCurrentTokenSentinelUnmodifiedOverCorpus(external test) runs all three entry points over the full W0-02 corpus for every dialect, as PLAN W3-02 asks. Fix (a59315c): the dialect is re-applied after each Reset (Reset clears it), so every statement really runs under its dialect, and each stream is also fed with the trailing EOF token stripped so currentToken/peekToken actually reach &p.eof. Overlay mutation check: an unconditional and a clickhouse-gated write to p.eof both fail this test (previously the gated one passed). Mutation check: deleting the re-point at parser.go lines 70, 163, 672 or 734 now fails the suite. Merged origin/main (perf(tokenizer): single fused-hash keyword table [W2-01] #920); no golden diffs.TestCurrentToken_SentinelIsPerParser(D4),TestCurrentToken_ZeroValueParser,TestCurrentToken_EmptyStreamClearsStaleCursor,TestCurrentToken_ConcurrentParsers(-race).dialect_migration_test.go(two tests used to write throughcurrentToken, they now allocate their own token so they cannot touch a sentinel),paren_query_test.go,type_helpers_test.go,utility_cursor_test.go.Gates (AC-STD), all run on this branch:
task check(fmt, vet, lint,test:raceincluding the W0-02 goldens): passtask test:cbinding: passgo test -count=1 ./pkg/compatibility/...: passgo run honnef.co/go/tools/cmd/staticcheck@latest ./pkg/sql/parser/: cleanLocal A/B (Apple M4, shared machine, indicative only)
scripts/perf/ab.sh -g -a -r 10 -t 300msagainstorigin/main(c05c5fd), 10 interleaved rounds,-cpu 1. A/A control: 0 of 30 benchmarks moved more than 3%. allocs/op and B/op identical on every benchmark.-cpu 1, GOGC=off (gated primary)-cpu 1, default GC (secondary)-cpu 4, default GC, 10 ABBA roundsReading: the gain is the GC write-barrier and copy cost, so it is small with GC off, 3-4% end to end and 6% (geomean) in parse-only with the GC running at
-cpu 1, and not distinguishable from noise at-cpu 4. This matches the PLAN evidence (CONFIRMED at-cpu 1, not significant at-cpu 4). Plan gates:parseonly<= -5% at-cpu 1is met with default GC (geomean about -6%) but not with GOGC=off (-1.9%); E2E <= -2% at-cpu 1is met with default GC (about -3.3%) but not with GOGC=off (-0.7%). The CI gate's primary metric is GOGC=off, so see the CI verdict below; the miss against the PLAN threshold in that regime is reported, not chased.Raw outputs: scratchpad
perf/wave1/w3-02/ab1/report.md,cpu4-benchstat.txt(not committed).CI perf gate (amd64, shadow)
Run 41331968001, "A/B perf gate (shadow)": PASS, "4 benchmark(s) faster by 3% or more", no regression, allocs/op and B/op unchanged on every benchmark, A/A control 0 of 26 benchmarks over 3%. amd64 (pinned CPU 3), GOGC=off,
-cpu 1, 10 interleaved rounds. Full sticky comment above; summary:-cpu 1)ParseE2E/*(11)ParseRW/*/medium(8)ParseFresh/{simple_select,join,complex}PoolGetPut/serial,Tokenize/scratch/*Against the PLAN gates: E2E <= -2% at
-cpu 1is met with default GC (-2.8% .. -5.3%) and by half of the queries in the GOGC=off primary metric (geomean about -1.5%);parseonlyis not in the CI gate suite (local numbers above).-cpu 4is not measured by the CI gate; the local-cpu 4result is within noise, as the PLAN predicts.Note:
ParseFresh(-28% to -45%) is far larger than the local steady-state gain. Hypothesis, not verified: the smaller frames from dropping the 72 B copies (the audit measured parseExpression 384 -> 288 B, parseStatement 288 -> 256 B) mean fewermorestackstack growths on a fresh goroutine. It is consistent with the PLAN's note that frame-size effects show on fresh goroutines, but I did not profile it.Breaking Changes
None.
currentToken,eofandpeekTokenare unexported. Parse results, error codes, locations and AST positions are unchanged (goldens pass, differential identical).🤖 Generated with Claude Code
https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2