Repository navigation
perf(lsp): cheaper semanticTokens [W6-11] - #948
Merged
Merged
Conversation
…ared case folding [W6-11] 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 32s —— View job Review of PR #948: cheaper semanticTokens [W6-11]I read the diff for Overall: The change looks correct and well contained. I found no blocking issues. What I checked
Suggestions (non-blocking)
Security: I found no concerns. There is no unsafe code, and the pooled buffers are cleared of document references before reuse. Tests: Coverage is good: a byte-identical golden, an LSP reference test against the old encoder, a fold test and benchmarks. |
github-actions
Bot
added
documentation
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.29%, p 0.086. Low power1 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) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 25s —— View job Review of PR #948: cheaper semanticTokens [W6-11]I read the full diff of Verdict: The change is correct and well contained. I found one minor issue and a few nits. Minor issue: stale lexemes can pin memory in the pool
What I checked and found correct
Nits (non-blocking)
Security: No concerns. There is no unsafe code, and pooled buffers are cleared before reuse, apart from the retry edge case above. Tests: Coverage is good: a byte-identical golden, an LSP reference test, a fold test and benchmarks. |
…ing [W6-11] Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 33s —— View job Review of PR #948: cheaper semanticTokens [W6-11]I read Verdict: I found no blocking issues. I checked the earlier "stale lexemes pin memory" concern from the previous review and it does not apply to the current code. Why the
Non-blocking suggestions
Security: No concerns. There is no unsafe code, and pooled buffers are cleared before reuse. Tests: Coverage is good: a byte-identical golden, an LSP reference test against the old encoder, a fold test, a concurrency test and benchmarks. |
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 #948: cheaper semanticTokens [W6-11]I read the diff for Verdict: I found no blocking issues. The change is correct and well contained. Checked and correct
Non-blocking suggestions
Performance
Security: No concerns. There is no unsafe code, and pooled buffers are cleared of document references before reuse. Tests: Coverage is good: a byte-identical golden, an LSP reference test against the old encoder, a fold test, a concurrency test and benchmarks. |
…4 figures in CHANGELOG [W6-11] Merge origin/main; split resetScratch from putScratch so tests never touch a pooled object; add a driven case that fails if resolve stores out[:0]. 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 #948: cheaper semanticTokens [W6-11]This is a static review of the current head ( Verdict: No blocking issues. The change is correct and contained. Checked and correct
Findings (minor)
Performance
Security: No concerns. There is no unsafe code, and pooled buffers are cleared of document references before reuse. Tests: Coverage is good: a byte-identical golden, an LSP reference test against the old encoder, a fold test, a concurrency test and benchmarks. |
…erential [W6-11] 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 #948: cheaper semanticTokens [W6-11]This is a static review of the current head. I read Verdict: No blocking issues. The change is correct and contained. Checked and correct
Minor findings
Performance
Security: No concerns. There is no unsafe code, and pooled buffers are cleared of document references before reuse. Tests: Coverage is good: a byte-identical golden, an LSP reference test and fuzz tests, a fold test, a concurrency test, and mutation checks that the PR body describes. |
|
Claude finished @ajitpratap0's task in 15s —— View job Review of PR #948: cheaper semanticTokens [W6-11]This is a static review of the current head. I read Verdict: No blocking issues. The change is correct and contained. Checked and correct
Minor findings
Performance
Security: No concerns. There is no unsafe code, and pooled buffers are cleared of document references before reuse. Tests: Coverage is good: a byte-identical golden, an LSP reference test with fuzz and a control/DEL differential, a fold test, a concurrency test, and the mutation checks in the PR body. |
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
W6-11 (LSP-2):
highlight.Spanskeeps lexeme/comment/line-start buffers in a pooled scratch and case-folds each lexeme once through a shared buffer; semantic-token encoding is leaner. No public API or behaviour change; ahighlightgolden plus an LSP reference test pin output byte-identical. Follow-ups in this round: merged origin/main (CHANGELOG conflict with #896 resolved, both entries kept under the single### Performanceheading), renamed the misnamedhighlight_bench_test.gotoconcurrency_test.go, and documented the intentionally duplicated emit block inencodeSemanticTokens.Test Plan
task check,task test:cbinding,go test -count=1 ./pkg/compatibility/..., staticcheck on pkg/lsp and pkg/highlight: all pass. One unrelated timing flake (internal/scaletestTestAssertLinearGrowthSlowQuadraticFailsWithinBudget, race shard 2/4) passed on CI rerun.Spans: -12.15% (p<0.001), B/op -84.8%, allocs 252 -> 3LSP/semanticTokens/30KBpostgresql/mysql/sqlserver: -13.3 / -13.9 / -14.3% (p<0.001), B/op -26.4 / -26.8 / -26.0%, allocs ~2000 -> ~20LSP/semanticTokens/1KB: -24.9 / -34.4 / -37.3%;300KB: -12.4 / -17.7 / -18.1%task bench:ab BASE=origin/main ROUNDS=6 LAYOUTS=4,./pkg/lspBenchmarkLSP/semanticTokens, head ca27d16): 30KB sec/op -21.4 / -18.8 / -18.9% (pg/mysql/sqlserver, p=0.002), B/op -26.4 / -26.8 / -26.0%, allocs 2032 -> 13, 1856 -> 14, 1946 -> 14; 1KB B/op -63 to -79%; 300KB allocs -99.7%. Full output: scratchab-local.log.highlight_golden_test.go,concurrency_test.go,semantic_tokens_ref_test.go,semantic_tokens_test.goandtestdata/goldens/highlight.sha256, copied onto the unchanged merge base, all pass. The new golden therefore proves the output is byte-identical to before.clear(sc.lexemes[:cap(sc.lexemes)])from resetScratch (the real retention mutation: pooled scratch would pin document strings) -> TestScratchClearedAfterRealRun and TestPutScratchDropsReferences fail; note storingh.sc.lexemes = out[:0]is harmless (resetScratch clears to cap, same backing array) and is not a retention bug; plan mutations (c)-(i) were not re-run in this round. Also added ASCII control/DEL bytes to the 200k random differential; a mutation making DEL count 2 UTF-16 units in the in-span ASCII branch of handler.go now fails TestEncodeSemanticTokensMatchesReference.PoolGetPut/serial+5.43% (p<0.001, 0 allocs, A/A' -0.05%). This PR changes no code in pkg/gosqlx or anything PoolGetPut exercises, and the same row read -3.46% on the previous head, so this is reported as layout/runner noise rather than hidden; no other gated row moved and allocs/op and B/op did not rise anywhere.Breaking Changes
None.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
Gate record
Amd64 matrix run 41743109325: 30KB semanticTokens -13.3/-13.9/-14.3% (p<0.001), which MISSES the PLAN target of <= -15% (reported, not hidden; still a significant gain so PLAN 5.3 rule 5 does not apply). Deterministic thresholds hold: 30KB B/op -26%, allocs ~-99%, 1KB B/op -63 to -79%, Spans allocs 252->3. BenchmarkLSP is not yet in scripts/perf/gate-suite.txt, so the time verdict comes from matrix mode, not the gated suite. Layout-randomized gate on ca27d16 flagged PoolGetPut/serial +5.43%: this benchmark only touches tokenizer/parser/AST Get/Put which this PR does not change, and the row swings without cause on unrelated PRs (#954 -3.29%, #945 -3.53%); re-run on head 3307dc8 (merge ref 5703e86, base 8889402, 20 layouts/arm, AMD EPYC 9V45): Perf gate PASS, PoolGetPut/serial -0.41% (p=0.552, listed under Low power), no gated regression, allocs/op and B/op unchanged. All CI checks green, mergeState CLEAN.