Skip to content

Add performance benchmarks and review report#10

Merged
rjrodger merged 2 commits into
mainfrom
claude/optimize-yaml-parsing-IItaC
Apr 30, 2026
Merged

Add performance benchmarks and review report#10
rjrodger merged 2 commits into
mainfrom
claude/optimize-yaml-parsing-IItaC

Conversation

@rjrodger
Copy link
Copy Markdown
Contributor

Introduces a reproducible benchmark and profiling harness for both the TS and Go parsers, plus a markdown report (bench/REPORT.md) documenting the hotspots found by review and the measurements that confirm them.

Layout:
bench/fixtures/generate.mjs synthetic fixture generator (11 shapes)
bench/ts/bench.mjs per-fixture sweep + optional cpuprofile
bench/ts/scaling.mjs per-byte scaling sweep (N = 100..4000)
go/bench_test.go one Benchmark per fixture
go/scaling_test.go scaling sweep under go test -run
bench/README.md how to run / reproduce
bench/REPORT.md findings, profiles, recommendations

Key findings measured (see REPORT.md for detail):

  • O(N^2) flow-context rescan in the plain-scalar handler (src/yaml.ts:922, go/yaml.go:1342): cache missing in Go, only partial in TS.
  • JSON-based deep copy for alias resolution in both languages.
  • regexp.MustCompile inside the Go lexer hot path.
  • Character-by-character string concatenation in block scalar folding and continuation-line scanning.
  • 700k+ allocations per 52KB parse in Go, driven by unsized maps and per-token boxing.

Generated artifacts (fixtures, .cpuprof, .cpuprofile, ts-*.json, *.log) are gitignored — regenerate with:
node bench/fixtures/generate.mjs
node bench/ts/bench.mjs
(cd go && go test -bench=. -benchmem -run=^$ .)

claude and others added 2 commits April 21, 2026 15:53
Introduces a reproducible benchmark and profiling harness for both the
TS and Go parsers, plus a markdown report (bench/REPORT.md) documenting
the hotspots found by review and the measurements that confirm them.

Layout:
  bench/fixtures/generate.mjs   synthetic fixture generator (11 shapes)
  bench/ts/bench.mjs            per-fixture sweep + optional cpuprofile
  bench/ts/scaling.mjs          per-byte scaling sweep (N = 100..4000)
  go/bench_test.go              one Benchmark per fixture
  go/scaling_test.go            scaling sweep under `go test -run`
  bench/README.md               how to run / reproduce
  bench/REPORT.md               findings, profiles, recommendations

Key findings measured (see REPORT.md for detail):
  - O(N^2) flow-context rescan in the plain-scalar handler
    (src/yaml.ts:922, go/yaml.go:1342): cache missing in Go,
    only partial in TS.
  - JSON-based deep copy for alias resolution in both languages.
  - regexp.MustCompile inside the Go lexer hot path.
  - Character-by-character string concatenation in block scalar
    folding and continuation-line scanning.
  - 700k+ allocations per 52KB parse in Go, driven by unsized
    maps and per-token boxing.

Generated artifacts (fixtures, .cpuprof, .cpuprofile, ts-*.json, *.log)
are gitignored — regenerate with:
  node bench/fixtures/generate.mjs
  node bench/ts/bench.mjs
  (cd go && go test -bench=. -benchmem -run=^\$ .)
@rjrodger rjrodger merged commit f0f67db into main Apr 30, 2026
3 of 6 checks passed
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bbd10e1031

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread bench/ts/scaling.mjs
for (let i = 0; i < iters; i++) {
const parser = makeParser()
const t0 = performance.now()
try { parser(src) } catch { /* fall through; still report wall time */ }
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fail scaling runs on parse errors instead of timing them

The scaling harness currently swallows parser exceptions and still records elapsed time, so a broken parse at any size/shape is reported as a valid performance datapoint. In practice this can silently corrupt the benchmark output (including slope calculations) by mixing failure paths with successful parses, which undermines the report’s conclusions. Please surface the error (or at least mark that row as failed and exclude it from metrics) rather than continuing as if the parse succeeded.

Useful? React with 👍 / 👎.

Comment thread go/scaling_test.go
Comment on lines +80 to +82
// TestScaling is a "benchmark" that runs under go test so we can see the
// growth curve for each input shape. Not a pass/fail test.
func TestScaling(t *testing.T) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep scaling benchmark out of default go test execution

Declaring the scaling sweep as TestScaling makes it run on every go test ./... invocation, even though this function is intentionally non-assertive and exists only for timing output. Since the repo workflow runs go test -v ./... in CI, this introduces repeated performance benchmarking work into normal correctness runs, increasing job time and flakiness risk without improving test signal. This should be moved to a benchmark (Benchmark...) or gated behind an explicit opt-in flag/env var.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants