Repository navigation
perf(keywords): allocation-free case-insensitive keyword lookups [W2-07] - #944
Open
ajitpratap0 wants to merge 2 commits into
Open
ajitpratap0 wants to merge 2 commits into
ajitpratap0 wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
lookupFold upper-cases pure-ASCII words up to 64 bytes into a stack buffer; other input falls back to strings.ToUpper. No behaviour change. 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 20s —— View job Review of PR #944: allocation-free keyword lookupsVerdict: looks good, with a few nits. I found no blocking issues. I read the production diff in full. I did not run the tests or benchmarks. The numbers below are from your PR description. Correctness
Performance
Tests
Nits
No security concerns. The change is pure lookup logic with no new inputs or unsafe code. |
Perf gate: PASS (shadow mode: report only, this check never fails)Base Runner CPU:
Rules: each arm runs 20 randomized code layouts ( Gated suite (GOGC=off, GOMAXPROCS=1)
A/A' control (base revision, disjoint layout seeds): 0 of 52 benchmarks moved more than 3%; 0 would have been flagged by the time rule; suite geomean +0.04%, p 0.738. Low power3 benchmark(s) have under 80% estimated power to be flagged on their own for a +5% slowdown (their spread across layouts is too wide for 20 layouts at the Benjamini-Hochberg level alpha/m = 0.000192). A slowdown below the last column usually goes unflagged there; instructions/op, allocs/op and the suite geomean still guard them.
Power is estimated with the normal approximation for the U test (Noether): power = Phi(ln(1+shift)/s * sqrt(K/2) * sqrt(3/pi) - z(1 - alpha/(2m))), with s the spread and K the layouts per arm. Instructions per operation (cmd/perfdriver)
Secondary metric: default GC, GOMAXPROCS=1 (informational)
Environmentbenchstat (primary metric, samples pooled across layouts: informational, the verdict uses the per-layout test) |
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
lookupFoldhelper (pkg/sql/keywords/lookup_fold.go): upper-cases pure-ASCII words up to 64 bytes into a stack buffer and looks up from there; any other input falls back tostrings.ToUpper. Used byIsKeyword,GetKeywordType,IsReserved,GetKeyword,GetTokenType,IsCompoundKeywordStartandcontainsKeyword. Results are identical for every input (Unicode folds such asſelect, invalid UTF-8, long words, case-sensitive instances).BenchmarkKeywordLookup(commit 1 = baseline) and tests: differential oracle vs frozen copies of the old bodies, fuzz target, built-in-keys-fit-fast-path check, zero-alloc test (fails on the old code; 7 mutations M1-M7 each caught).Test Plan
task test:cbinding,go test ./pkg/compatibility/..., staticcheck on./pkg/sql/keywords/...: pass.go test -raceon keywords, formatter, highlight, mcp: pass.task checkhit 120s package timeouts (formatter, transpiler) at machine load average 50-75; both pass when run alone with a larger timeout.can inline lookupFold[...],string(buf[:len(s)]) does not escape.task bench:ab BASE=<commit1>(20 layouts, indicative, shared M4): geomean -54.5%; lower/mixed/miss -63% to -76% for all six methods, allocs 1 -> 0, B/op -> 0; upper -6% to -29%; long (65 B) ~0 except IsKeyword/long +4.75% (p=0.007, 3% spread, low power; allocs unchanged, code path unchanged for len>64 so I read it as noise).Breaking Changes
None.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2