Skip to content

perf(tokenizer): read the clock only when metrics are enabled [W1-02] - #923

Merged
ajitpratap0 merged 3 commits into
mainfrom
perf/w1-02-clock-gate
Nov 1, 2026
Merged

perf(tokenizer): read the clock only when metrics are enabled [W1-02]#923
ajitpratap0 merged 3 commits into
mainfrom
perf/w1-02-clock-gate

Conversation

@ajitpratap0

@ajitpratap0 ajitpratap0 commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

Tokenizer.tokenize called time.Now() and time.Since() on every call, even with metrics disabled (the default). The clock is now read only when metrics.IsEnabled(). If metrics are enabled mid-call the call is not recorded (PLAN said "duration 0"; not recording is equivalent for disabled-at-entry and avoids a bogus zero-duration sample). No API, token-output or error-text change. PLAN W1-02 (TOK-6 / MEM-2 time part).

Test Plan

  • New pkg/sql/tokenizer/metrics_gate_test.go: counters recorded for success, tokenize error and oversize error when enabled; nothing recorded when disabled; token output identical either way.
  • task check, task test:cbinding, go test -count=1 ./pkg/compatibility/..., staticcheck on ./pkg/sql/tokenizer/: pass. Goldens: no diffs.
  • Local scripts/perf/ab.sh -r 10 -t 200ms vs origin/main (GOGC=off, -cpu 1, M4, shared machine so indicative only):
    • Tokenize/scratch geomean 635.8n -> 583.5n, -8.23% (all 11 queries p<0.001, from -3.0% to -16.1%); plan threshold <= -4%: met.
    • ParseE2E/simple_select 582.6n -> 541.4n, -7.07% (p<0.001); plan threshold <= -3%: met. ParseE2E geomean -3.78%.
    • B/op and allocs/op unchanged everywhere.
  • CI perf gate (amd64, shadow, run 41331999297, base 69a5a72 head 1a02ee1): PASS. Gated geomean -1.87% (p<0.001), A/A control clean (0/26 moved >3%), no allocs/op or B/op change, instructions/op down on every e2e case (-0.15% to -1.59%).
    • ParseE2E/simple_select -3.37%, select_where -3.42%, aggregate -3.39%, subquery -3.89%, window -3.16%, insert -4.39%, update -5.40%, delete -8.50% (all significant); join -1.93%, cte -2.60%; complex -0.25% (n.s.).
    • Tokenize/scratch (amd64 gate subset): insert -4.38%, join -0.50%, complex +1.09% (p<0.001, below the 3% regress line; instructions/op for e2e/complex is -0.15%).
    • ParseRW medium: all within +-1.0%, not significant. Plan expected -0.5% to -2.5% on amd64 for short statements; the PLAN 4%/3% thresholds (measured on M4) are met locally but only partly on amd64 (simple_select -3.4%, meets -3%).

Breaking Changes

None.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

@vercel

vercel Bot commented Nov 1, 2026 •
edited
Loading

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 1, 2026 6:39pm UTC

@github-actions github-actions Bot added documentation Improvements or additions to documentation tokenizer Tokenizer / lexer changes labels Nov 1, 2026
@github-actions

github-actions Bot commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown

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

Review of PR #923: gate time.Now() on metrics.IsEnabled()

Verdict: looks good. I found one stale comment and a few small test nits. Nothing blocks merging.

I reviewed the diff only. I did not run the tests or benchmarks in this session.

Code (pkg/sql/tokenizer/tokenizer.go)

  • The change is correct and minimal. metricsOn is read once at entry. Every time.Since call is behind the same flag, so startTime is never read as the zero time.Time.
  • Stale comment (around line 608): it says "the duration is recorded as 0" if metrics are toggled on mid-call. The code doesn't do that. metricsOn is false, so nothing is recorded. The PR description and CHANGELOG both say "not recorded", so the code comment is the odd one out. Suggested wording: "If metrics are toggled on mid-call, that call is not recorded."
  • Enabled mid-call, then disabled: if metrics are on at entry and turned off before the end, RecordTokenization still runs. I don't know whether it re-checks IsEnabled internally. It would be a harmless sample either way, since it's the same race as before.
  • Cost of the check: metrics.IsEnabled() is probably a single atomic load, which is cheap. This is consistent with the measured gains.

Tests (metrics_gate_test.go)

  • The gate test covers success, tokenize error and oversize error, with metrics on and off. That's good coverage of the three RecordTokenization sites.
  • Global state: these tests change process-wide metrics state with Enable/Disable/Reset. They restore the enabled flag in t.Cleanup, but Reset() still wipes counters other tests might rely on. They also aren't safe under t.Parallel(). Nothing here is parallel, so this is fine today. Other tests in the package that read metrics could become flaky if someone adds parallelism later.
  • Weak assertion: TestTokenize_MetricsDoNotChangeTokens compares only token counts. Comparing the token types and values with reflect.DeepEqual or a loop would match the "token output identical" claim better. The tokens aren't safe to hold after PutTokenizer, so copy or compare them before the deferred release.
  • Missing case: there's no test for the tokscratch path (scratch == true). The gate is shared, so the risk is low.
  • Oversize allocation: strings.Repeat("x", MaxInputSize+1) allocates the full oversize buffer on each run() call, so twice per test. That's fine if MaxInputSize is a few MB. If it's much larger, consider building the slice once.

Performance, security, docs

  • Performance: the claimed gains are plausible. The PR itself says the amd64 gain may be only 0.5–2.5%, since tsc-based clocks are cheap. The pending CI gate result is the number that matters.
  • Security: nothing to flag. The size check still runs before the copy.
  • CHANGELOG: the entry is accurate, and its "not recorded" wording matches the behavior.

Suggested fixes

  1. Correct the stale "recorded as 0" comment.
  2. Optionally compare token content, not just the count, in the second test.
    · branch perf/w1-02-clock-gate

@github-actions

github-actions Bot commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown

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

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

  • 11 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 -3.69% <0.001 +0.19% +0.00% 5 → 5 faster
ParseE2E/select_where -2.78% <0.001 +0.05% +0.00% 18 → 18 ok
ParseE2E/join -1.94% <0.001 +0.60% +0.00% 17 → 17 ok
ParseE2E/aggregate -3.48% <0.001 +0.25% +0.00% 18 → 18 faster
ParseE2E/subquery -3.98% 0.003 +0.47% +0.00% 14 → 14 faster
ParseE2E/cte -3.16% <0.001 +0.09% +0.00% 21 → 21 faster
ParseE2E/window -4.19% <0.001 +0.10% +0.00% 11 → 11 faster
ParseE2E/complex -0.38% 0.165 +0.04% +0.00% 50 → 50 ok
ParseE2E/insert -4.34% <0.001 -0.41% +0.00% 17 → 17 faster
ParseE2E/update -5.20% <0.001 -0.10% +0.00% 13 → 13 faster
ParseE2E/delete -8.62% <0.001 +0.23% +0.00% 5 → 5 faster
ParseRW/postgresql/medium -0.35% 0.043 -0.15% +0.00% 646 → 646 ok
ParseRW/mysql/medium -1.01% 0.035 -0.63% +0.00% 365 → 365 ok
ParseRW/mariadb/medium +0.11% 0.796 -0.70% -0.00% 523 → 523 ok
ParseRW/sqlserver/medium +0.98% 0.010 -0.18% +0.00% 936 → 936 ok
ParseRW/oracle/medium -0.72% 0.143 -0.39% -0.00% 1093 → 1093 ok
ParseRW/sqlite/medium -0.92% 0.015 -0.11% +0.00% 346 → 346 ok
ParseRW/snowflake/medium -0.29% 0.353 -0.09% +0.00% 777 → 777 ok
ParseRW/clickhouse/medium -0.18% 0.393 -0.32% +0.00% 384 → 384 ok
ParseFresh/simple_select -0.80% 0.015 -0.09% +0.00% 6 → 6 ok
ParseFresh/join -1.38% <0.001 -0.41% +0.00% 18 → 18 ok
ParseFresh/complex -0.40% 0.089 +0.07% +0.00% 51 → 51 ok
PoolGetPut/serial -0.17% 0.226 +0.07% +0.00% 0 → 0 ok
Tokenize/scratch/join -0.08% 0.426 +0.06% +0.00% 0 → 0 ok
Tokenize/scratch/complex +1.78% <0.001 +0.01% +0.00% 0 → 0 ok
Tokenize/scratch/insert -4.50% <0.001 -0.19% +0.00% 0 → 0 faster
Normalize/simple_select -3.26% <0.001 +0.03% +0.00% 10 → 10 faster
Normalize/join -1.59% <0.001 -0.08% +0.00% 40 → 40 ok
Normalize/complex -0.58% 0.023 -0.12% +0.00% 110 → 110 ok
TransformParse/simple_select -6.29% 0.003 -0.22% +0.00% 5 → 5 faster
TransformParse/join -2.99% 0.003 +0.33% +0.00% 17 → 17 ok
TransformParse/complex -0.87% 0.011 +0.22% +0.00% 50 → 50 ok
geomean -2.07% <0.001 -0.04%

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

Instructions per operation (cmd/perfdriver)

case instructions/op status
e2e/aggregate -0.50% ok
e2e/complex -0.11% ok
e2e/cte -0.49% ok
e2e/delete -1.54% ok
e2e/insert -0.70% ok
e2e/join -0.39% ok
e2e/select_where -0.54% ok
e2e/simple_select -1.55% ok
e2e/subquery -0.60% ok
e2e/update -0.95% ok
e2e/window -0.70% ok
rw/postgresql/medium -0.20% ok
Secondary metric: default GC, GOMAXPROCS=1 (informational)
benchmark time p
ParseE2E/simple_select -3.32% <0.001
ParseE2E/select_where -0.99% 0.005
ParseE2E/join -2.00% 0.015
ParseE2E/aggregate -2.40% 0.002
ParseE2E/subquery -3.33% <0.001
ParseE2E/cte -3.71% <0.001
ParseE2E/window -3.07% <0.001
ParseE2E/complex -0.57% 0.002
ParseE2E/insert -0.60% 0.064
ParseE2E/update -1.28% 0.007
ParseE2E/delete -5.92% <0.001
ParseRW/postgresql/medium -0.08% 0.796
ParseRW/mysql/medium +0.37% 0.739
ParseRW/mariadb/medium +0.11% 0.529
ParseRW/sqlserver/medium +0.03% 0.684
ParseRW/oracle/medium -0.23% 1.000
ParseRW/sqlite/medium +0.35% 0.280
ParseRW/snowflake/medium +0.55% 0.218
ParseRW/clickhouse/medium +0.40% 0.105
ParseFresh/simple_select -0.94% <0.001
ParseFresh/join -1.06% <0.001
ParseFresh/complex +0.22% 0.579
PoolGetPut/serial -0.28% 0.082
Tokenize/scratch/join -0.30% 0.053
Tokenize/scratch/complex +1.55% <0.001
Tokenize/scratch/insert -4.32% <0.001
Normalize/simple_select -2.88% <0.001
Normalize/join -1.61% 0.023
Normalize/complex +0.52% 0.684
TransformParse/simple_select -5.32% <0.001
TransformParse/join -1.47% 0.009
TransformParse/complex -0.54% 0.151
geomean -1.33%
Environment 
date:        2026-11-01T18:45:36Z
base:        160c41726db7d601c5cb7478d417316df9d8d2fe
head:        working tree of db46c7417016f3fdea00fbc4175021051c320aeb
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.96, 0.67, 0.24
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.916µ ± 1%   2.821µ ± 0%  -3.26% (p=0.000 n=10)
Normalize/join            10.64µ ± 3%   10.47µ ± 1%  -1.59% (p=0.000 n=10)
Normalize/complex         26.65µ ± 1%   26.49µ ± 1%       ~ (p=0.023 n=10)
geomean                   9.384µ        9.214µ       -1.82%

                        │     base     │                 head                  │
                        │     B/op     │     B/op      vs base                 │
Normalize/simple_select   1.016Ki ± 0%   1.016Ki ± 0%       ~ (p=1.000 n=10) ¹
Normalize/join            3.234Ki ± 0%   3.234Ki ± 0%       ~ (p=1.000 n=10) ¹
Normalize/complex         8.945Ki ± 0%   8.945Ki ± 0%       ~ (p=1.000 n=10) ¹
geomean                   3.086Ki        3.086Ki       +0.00%
¹ all samples are equal

                        │    base    │                head                 │
                        │ allocs/op  │ allocs/op   vs base                 │
Normalize/simple_select   10.00 ± 0%   10.00 ± 0%       ~ (p=1.000 n=10) ¹
Normalize/join            40.00 ± 0%   40.00 ± 0%       ~ (p=1.000 n=10) ¹
Normalize/complex         110.0 ± 0%   110.0 ± 0%       ~ (p=1.000 n=10) ¹
geomean                   35.30        35.30       +0.00%
¹ all samples are equal

pkg: github.com/ajitpratap0/GoSQLX/pkg/gosqlx
                          │    base     │                head                │
                          │   sec/op    │   sec/op     vs base               │
ParseE2E/simple_select      1.842µ ± 1%   1.774µ ± 1%  -3.69% (p=0.000 n=10)
ParseE2E/select_where       3.918µ ± 1%   3.810µ ± 1%  -2.78% (p=0.000 n=10)
ParseE2E/join               5.863µ ± 0%   5.749µ ± 1%  -1.94% (p=0.000 n=10)
ParseE2E/aggregate          4.321µ ± 0%   4.171µ ± 1%  -3.48% (p=0.000 n=10)
ParseE2E/subquery           3.741µ ± 0%   3.592µ ± 1%  -3.98% (p=0.001 n=10)
ParseE2E/cte                5.161µ ± 0%   4.998µ ± 0%  -3.16% (p=0.000 n=10)
ParseE2E/window             3.404µ ± 0%   3.262µ ± 1%  -4.19% (p=0.000 n=10)
ParseE2E/complex            15.57µ ± 1%   15.51µ ± 1%       ~ (p=0.165 n=10)
ParseE2E/insert             2.675µ ± 1%   2.559µ ± 0%  -4.34% (p=0.000 n=10)
ParseE2E/update             2.433µ ± 0%   2.306µ ± 0%  -5.20% (p=0.000 n=10)
ParseE2E/delete             1.537µ ± 1%   1.405µ ± 3%  -8.62% (p=0.000 n=10)
ParseRW/postgresql/medium   196.8µ ± 1%   196.1µ ± 1%       ~ (p=0.043 n=10)
ParseRW/mysql/medium        109.3µ ± 1%   108.2µ ± 1%       ~ (p=0.035 n=10)
ParseRW/mariadb/medium      159.6µ ± 2%   159.8µ ± 0%       ~ (p=0.796 n=10)
ParseRW/sqlserver/medium    332.6µ ± 1%   335.8µ ± 1%  +0.98% (p=0.007 n=10)
ParseRW/oracle/medium       394.3µ ± 1%   391.5µ ± 1%       ~ (p=0.143 n=10)
ParseRW/sqlite/medium       92.75µ ± 1%   91.89µ ± 1%       ~ (p=0.015 n=10)
ParseRW/snowflake/medium    230.0µ ± 1%   229.3µ ± 1%       ~ (p=0.353 n=10)
ParseRW/clickhouse/medium   105.6µ ± 1%   105.4µ ± 1%       ~ (p=0.393 n=10)
ParseFresh/simple_select    10.12µ ± 0%   10.04µ ± 1%       ~ (p=0.015 n=10)
ParseFresh/join             23.62µ ± 1%   23.30µ ± 0%  -1.38% (p=0.000 n=10)
ParseFresh/complex          35.99µ ± 0%   35.84µ ± 1%       ~ (p=0.089 n=10)
PoolGetPut/serial           66.06n ± 0%   65.95n ± 1%       ~ (p=0.224 n=10)
geomean                     14.97µ        14.66µ       -2.06%

                          │     base     │                head                 │
                          │     B/s      │     B/s       vs base               │
ParseE2E/simple_select      13.47Mi ± 0%   13.99Mi ± 1%  +3.86% (p=0.000 n=10)
ParseE2E/select_where       25.07Mi ± 1%   25.79Mi ± 1%  +2.87% (p=0.000 n=10)
ParseE2E/join               20.49Mi ± 0%   20.90Mi ± 1%  +1.98% (p=0.000 n=10)
ParseE2E/aggregate          18.54Mi ± 0%   19.21Mi ± 1%  +3.63% (p=0.000 n=10)
ParseE2E/subquery           20.90Mi ± 0%   21.77Mi ± 1%  +4.15% (p=0.001 n=10)
ParseE2E/cte                22.91Mi ± 0%   23.66Mi ± 0%  +3.25% (p=0.000 n=10)
ParseE2E/window             26.34Mi ± 0%   27.49Mi ± 1%  +4.38% (p=0.000 n=10)
ParseE2E/complex            23.16Mi ± 1%   23.25Mi ± 1%       ~ (p=0.190 n=10)
ParseE2E/insert             27.09Mi ± 1%   28.31Mi ± 0%  +4.52% (p=0.000 n=10)
ParseE2E/update             21.56Mi ± 0%   22.74Mi ± 0%  +5.49% (p=0.000 n=10)
ParseE2E/delete             32.27Mi ± 1%   35.30Mi ± 3%  +9.38% (p=0.000 n=10)
ParseRW/postgresql/medium   18.23Mi ± 1%   18.30Mi ± 1%       ~ (p=0.060 n=10)
ParseRW/mysql/medium        24.77Mi ± 1%   25.03Mi ± 1%       ~ (p=0.035 n=10)
ParseRW/mariadb/medium      22.21Mi ± 2%   22.19Mi ± 0%       ~ (p=0.796 n=10)
ParseRW/sqlserver/medium    20.01Mi ± 1%   19.81Mi ± 1%  -0.98% (p=0.006 n=10)
ParseRW/oracle/medium       21.26Mi ± 1%   21.41Mi ± 1%       ~ (p=0.138 n=10)
ParseRW/sqlite/medium       22.27Mi ± 1%   22.48Mi ± 1%       ~ (p=0.016 n=10)
ParseRW/snowflake/medium    22.11Mi ± 1%   22.17Mi ± 1%       ~ (p=0.287 n=10)
ParseRW/clickhouse/medium   21.35Mi ± 1%   21.38Mi ± 1%       ~ (p=0.343 n=10)
geomean                     21.98Mi        22.51Mi       +2.41%

                          │      base      │                 head                  │
                          │      B/op      │     B/op      vs base                 │
ParseE2E/simple_select        816.0 ± 0%       816.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/select_where       1.523Ki ± 0%     1.523Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/join               2.305Ki ± 0%     2.305Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/aggregate          1.812Ki ± 0%     1.812Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/subquery           1.828Ki ± 0%     1.828Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/cte                2.336Ki ± 0%     2.336Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/window             1.281Ki ± 0%     1.281Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/complex            6.399Ki ± 0%     6.399Ki ± 0%       ~ (p=1.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.52Ki ± 0%     70.52Ki ± 0%       ~ (p=0.116 n=10)
ParseRW/mysql/medium        43.73Ki ± 0%     43.73Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/mariadb/medium      64.85Ki ± 0%     64.85Ki ± 0%       ~ (p=0.237 n=10)
ParseRW/sqlserver/medium    90.61Ki ± 0%     90.61Ki ± 0%       ~ (p=0.097 n=10)
ParseRW/oracle/medium       113.8Ki ± 0%     113.8Ki ± 0%       ~ (p=0.170 n=10)
ParseRW/sqlite/medium       39.89Ki ± 0%     39.89Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/snowflake/medium    83.38Ki ± 0%     83.38Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/clickhouse/medium   37.13Ki ± 0%     37.13Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/simple_select      896.0 ± 0%       896.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/join             2.383Ki ± 0%     2.383Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/complex          6.477Ki ± 0%     6.477Ki ± 0%       ~ (p=1.000 n=10) ¹
PoolGetPut/serial             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                 │
ParseE2E/simple_select       5.000 ± 0%      5.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/select_where        18.00 ± 0%      18.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/join                17.00 ± 0%      17.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/aggregate           18.00 ± 0%      18.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/subquery            14.00 ± 0%      14.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/cte                 21.00 ± 0%      21.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/window              11.00 ± 0%      11.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/complex             50.00 ± 0%      50.00 ± 0%       ~ (p=1.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%      646.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/mysql/medium         365.0 ± 0%      365.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/mariadb/medium       523.0 ± 0%      523.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/sqlserver/medium     936.0 ± 0%      936.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/oracle/medium       1.093k ± 0%     1.093k ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/sqlite/medium        346.0 ± 0%      346.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/snowflake/medium     777.0 ± 0%      777.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/clickhouse/medium    384.0 ± 0%      384.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/simple_select     6.000 ± 0%      6.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/join              18.00 ± 0%      18.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/complex           51.00 ± 0%      51.00 ± 0%       ~ (p=1.000 n=10) ¹
PoolGetPut/serial            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                │
                          │  sec/stmt   │  sec/stmt    vs base               │
ParseRW/postgresql/medium   16.40µ ± 1%   16.34µ ± 1%       ~ (p=0.042 n=10)
ParseRW/mysql/medium        13.66µ ± 1%   13.52µ ± 1%       ~ (p=0.034 n=10)
ParseRW/mariadb/medium      17.74µ ± 2%   17.76µ ± 0%       ~ (p=0.796 n=10)
ParseRW/sqlserver/medium    15.84µ ± 1%   15.99µ ± 1%  +0.98% (p=0.007 n=10)
ParseRW/oracle/medium       16.43µ ± 1%   16.31µ ± 1%       ~ (p=0.148 n=10)
ParseRW/sqlite/medium       15.46µ ± 1%   15.31µ ± 1%       ~ (p=0.015 n=10)
ParseRW/snowflake/medium    17.69µ ± 1%   17.64µ ± 1%       ~ (p=0.353 n=10)
ParseRW/clickhouse/medium   15.08µ ± 1%   15.06µ ± 1%       ~ (p=0.363 n=10)
geomean                     15.99µ        15.94µ       -0.30%

pkg: github.com/ajitpratap0/GoSQLX/pkg/sql/tokenizer
                         │    base     │                head                │
                         │   sec/op    │   sec/op     vs base               │
Tokenize/scratch/join      2.636µ ± 0%   2.634µ ± 0%       ~ (p=0.424 n=10)
Tokenize/scratch/complex   7.021µ ± 0%   7.146µ ± 0%  +1.78% (p=0.000 n=10)
Tokenize/scratch/insert    1.321µ ± 3%   1.262µ ± 1%  -4.50% (p=0.000 n=10)
geomean                    2.902µ        2.874µ       -0.97%

                         │     base     │                head                 │
                         │     B/s      │     B/s       vs base               │
Tokenize/scratch/join      45.60Mi ± 0%   45.63Mi ± 0%       ~ (p=0.362 n=10)
Tokenize/scratch/complex   51.35Mi ± 1%   50.44Mi ± 0%  -1.76% (p=0.000 n=10)
Tokenize/scratch/insert    54.88Mi ± 3%   57.45Mi ± 1%  +4.68% (p=0.000 n=10)
geomean                    50.46Mi        50.95Mi       +0.97%

                         │     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.852µ ± 0%   1.736µ ± 1%  -6.29% (p=0.001 n=10)
TransformParse/join            6.331µ ± 0%   6.142µ ± 1%  -2.99% (p=0.001 n=10)
TransformParse/complex         16.52µ ± 1%   16.37µ ± 1%       ~ (p=0.011 n=10)
geomean                        5.785µ        5.588µ       -3.41%

                             │     base     │                 head                  │
                             │     B/op     │     B/op      vs base                 │
TransformParse/simple_select     816.0 ± 0%     816.0 ± 0%       ~ (p=1.000 n=10) ¹
TransformParse/join            2.305Ki ± 0%   2.305Ki ± 0%       ~ (p=1.000 n=10) ¹
TransformParse/complex         6.398Ki ± 0%   6.398Ki ± 0%       ~ (p=1.000 n=10) ¹
geomean                        2.273Ki        2.273Ki       +0.00%
¹ all samples are equal

                             │    base    │                head                 │
                             │ allocs/op  │ allocs/op   vs base                 │
TransformParse/simple_select   5.000 ± 0%   5.000 ± 0%       ~ (p=1.000 n=10) ¹
TransformParse/join            17.00 ± 0%   17.00 ± 0%       ~ (p=1.000 n=10) ¹
TransformParse/complex         50.00 ± 0%   50.00 ± 0%       ~ (p=1.000 n=10) ¹
geomean                        16.20        16.20       +0.00%
¹ all samples are equal

Run, raw data and benchstat

@ajitpratap0 @claude
fix(tokenizer): correct metrics-gate comment and strengthen token-ide…
f0ca0dc 
…ntity test [W1-02]

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
@ajitpratap0

ajitpratap0 commented Nov 1, 2026

Copy link
Copy Markdown
Owner Author

Pushed f0ca0dc: fixed the tokenize() comment (a call that starts with metrics off and has them enabled mid-call is not recorded, matching the code, CHANGELOG and this PR body; accepted over the PLAN's 'duration 0' to avoid fake zero-duration samples) and made TestTokenize_MetricsDoNotChangeTokens compare each token's Type and Value. Comment and test only, no production code change; the amd64 perf-gate numbers above still apply (gated geomean -1.87%, ParseE2E/simple_select -3.37%, allocs unchanged).

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown

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

Review of PR #923: gate time.Now() on metrics.IsEnabled()

Verdict: looks good to merge. I read the diff against origin/main but did not run the tests or benchmarks. The CI perf gate result in the PR comments is the only performance data I relied on.

Code (pkg/sql/tokenizer/tokenizer.go)

  • metricsOn is read once at entry. All three RecordTokenization sites (oversize, tokenize error, success) sit behind it, so time.Since is never called on a zero startTime.
  • The code comment now says "that call is not recorded", which matches the code, the PR description and the CHANGELOG. The stale "recorded as 0" wording flagged in the earlier review is gone from the diff.
  • If metrics are on at entry and turned off mid-call, the sample is still recorded. That is the same race as before this change and is harmless.
  • metrics.IsEnabled() is cheap relative to a clock read. The measured gains fit that.

Tests (metrics_gate_test.go)

  • TestTokenize_MetricsGate checks the enabled and disabled cases. When enabled it expects 3 operations and 2 errors, which covers the success, tokenize-error and oversize paths. When disabled it expects zero of both.
  • TestTokenize_MetricsDoNotChangeTokens compares token type and value, not just the count. It copies the values before the deferred PutTokenizer, so there is no use-after-release.
  • Nits, none blocking:
    • Both tests call Enable, Disable and Reset on process-wide metrics state. That is fine while no test in the package uses t.Parallel(). It would become flaky if someone adds parallel tests that read metrics.
    • Neither test exercises the scratch == true path. The gate is shared, so the risk is low.
    • strings.Repeat("x", MaxInputSize+1) allocates the oversize buffer on each run() call. Building it once is cheap if MaxInputSize is large.

Performance

  • The perf gate passed. The ParseE2E short statements are 2–8% faster, and allocs/op and B/op are unchanged.
  • Tokenize/scratch/complex is +1.09% (p<0.001), which is below the 3% regress line. Instructions/op is down on every e2e case, so I think this is layout or noise.
  • On amd64, ParseRW medium is flat. That is expected, because the clock read is a small share of those longer inputs.

Security

Nothing to flag. The size check still runs before the input copy.
· branch perf/w1-02-clock-gate

@github-actions

github-actions Bot commented Nov 1, 2026 •
edited
Loading

Copy link
Copy Markdown

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

Review of PR #923: gate time.Now() on metrics.IsEnabled()

Verdict: looks good to merge. I read the diff against origin/main. I did not run tests or benchmarks. For performance I relied on the CI perf-gate report.

Code (pkg/sql/tokenizer/tokenizer.go)

  • metricsOn is read once at entry. All three RecordTokenization sites (oversize, tokenize error, success) sit behind it, so time.Since is never called on a zero startTime.
  • The comment now says an enabled-mid-call invocation "is not recorded". That matches the code, the CHANGELOG and the PR body.
  • If metrics are on at entry and turned off mid-call, the sample is still recorded. This is the same race as before and is harmless.
  • The size check still runs before the input copy, so oversized input is still rejected before it is copied.

Tests (metrics_gate_test.go)

  • TestTokenize_MetricsGate covers the success, tokenize-error and oversize paths, both enabled and disabled.
  • TestTokenize_MetricsDoNotChangeTokens compares token type and value, not just the count.
  • Nits, none blocking:
    • The tests change process-wide metrics state with Enable, Disable and Reset. That is fine while the package has no t.Parallel() tests that read metrics.
    • The scratch == true path isn't exercised. It shares the same gate, so the risk is low.
    • strings.Repeat("x", MaxInputSize+1) is rebuilt on each run() call. Building it once would be cheap if MaxInputSize is large.

Performance

  • The perf gate passed in shadow mode: ParseE2E short statements are 1–8% faster. allocs/op and B/op are unchanged, and instructions/op is down on every e2e case.
  • The ParseRW medium cases are flat, which is expected because the clock read is a small share of longer inputs.

Security

Nothing to flag.

CHANGELOG

The entry is accurate and sits under Performance.
· branch perf/w1-02-clock-gate

@ajitpratap0
ajitpratap0 merged commit d29c644 into main Nov 1, 2026
48 checks passed
@ajitpratap0
ajitpratap0 deleted the perf/w1-02-clock-gate branch November 1, 2026 18:56
This was referenced Nov 1, 2026
Open
Open

This branch was successfully deployed

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

Reviewers

No reviews

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation tokenizer Tokenizer / lexer changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ajitpratap0