Plan 197 PoC: goldmark link-ref BlockReader reuse, +plan 198 by jeduden · Pull Request #369 · jeduden/mdsmith

jeduden · 2026-05-22T18:45:16Z

Summary

Completes plan 197 — a proof-of-concept to reduce goldmark parser allocations. The PoC measured a 12.8 % reduction in allocs/op (634k → 553k) by reusing a single text.BlockReader across all paragraphs in the link-reference transformer, instead of allocating fresh per paragraph. Wall time improved 6.4 % (264ms → 247ms p95). Results fell within 6 % of the predicted 13.6 % saving, passing the acceptance gate.

Also introduces plan 198 — the next phase to tackle the remaining four structural allocators (NewTextSegment, NewParagraph, Segments.Append backing arrays, FindClosure) via a per-parse arena, targeting ~41 % combined ceiling.

Key Changes

Plan 197 completion: Marked status ✅, filled review matrix with lifecycle/reuse-barrier/category/risk for all five hot allocators, documented cross-cutting findings, ranked by estimated saving, and recorded PoC benchmark results.
LinkRefParagraph transformer (internal/goldmark/linkrefparagraph/): Vendored goldmark's link-reference definition parser into a new package. The transformer now owns a reusable text.BlockReader that is Reset per paragraph instead of allocated fresh. Includes upstream license attribution.
Parser integration (pkg/markdown/parser.go): Wired the new transformer into the default parser configuration, replacing goldmark's singleton with a per-parser instance (safe under mdsmith's parserPool).
Plan 198 scaffolding: Added full plan document outlining the arena-based approach for the remaining structural allocators, with acceptance criteria and risk mitigation (AST lifetime contract, equivalence harness, build-tag A/B).

Implementation Details

The BlockReader reuse is safe because the type has a Reset(*Segments) method and holds only per-paragraph state (source, segments, pos, line, head, last, lineOffset).
The transformer is not a global singleton; each parser instance gets its own via New(), making it goroutine-safe under mdsmith's parserPool (one parser per goroutine).
Cross-Parse source changes trigger a fresh BlockReader allocation (text.BlockReader has no SetSource), but within a single Parse, Reset is reused across all paragraphs.
Benchmark methodology: BenchmarkCheckCorpusLarge -benchtime=10x -count=3 -benchmem on the same machine, same minute, three runs each.

The PoC unblocks plan 198, which will tackle the remaining ~41 % of allocations via a per-parse arena over the four structural allocators.

https://claude.ai/code/session_01H3V2ym6D1BSPRPgRgbcTGo

codecov · 2026-05-22T18:46:56Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.46%. Comparing base (c9ec4b3) to head (ee75a8f).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

Components	Coverage Δ
Go	`96.43% <100.00%> (+0.14%)`	⬆️
TypeScript	`99.39% <ø> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR completes plan 197’s proof-of-concept to reduce goldmark parsing allocations by reusing a text.BlockReader in the link-reference paragraph transformer, wires that transformer into mdsmith’s canonical parser, and adds plan 198 outlining a deeper goldmark fork using a per-parse arena for remaining structural allocators.

Changes:

Added a vendored internal/goldmark/linkrefparagraph transformer that reuses a text.BlockReader via Reset across paragraphs.
Integrated the per-parser transformer instance into pkg/markdown.NewParser() (replacing goldmark’s singleton transformer).
Updated planning docs: marked plan 197 complete, added plan 198, and updated PLAN.md catalog.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
plan/198_goldmark-arena-fork.md	Introduces the next-phase plan to fork goldmark with an arena allocator and equivalence gates.
plan/197_fork-goldmark-for-allocs.md	Marks plan 197 complete and records the review matrix + benchmark results.
PLAN.md	Updates the plan catalog for plan 197 status and adds plan 198.
pkg/markdown/parser.go	Installs the new per-parser link-ref paragraph transformer into the canonical parser configuration.
internal/goldmark/linkrefparagraph/UPSTREAM_LICENSE	Adds upstream MIT license attribution for the vendored goldmark code.
internal/goldmark/linkrefparagraph/transformer.go	Implements the reusable-BlockReader transformer logic and source-change handling.
internal/goldmark/linkrefparagraph/parser.go	Vendors goldmark link-ref parsing helpers used by the transformer.
internal/goldmark/linkrefparagraph/doc.go	Documents the forked transformer and its rationale.

Copilot review on plan-197 PoC flagged four items: 1. NewParser registered only the custom link-ref transformer instead of starting from parser.DefaultParagraphTransformers() and substituting. Future goldmark versions may add defaults that would then silently disappear. Walk the default slice, replace the LinkReferenceParagraphTransformer entry at its original priority, pass the rest through unchanged. 2. doc.go referenced NewTransformer() and *text.BlockReader; the actual API is New() and the field is the interface text.BlockReader. Doc updated. 3. The Transformer keeps the last-parsed source []byte and the underlying BlockReader for cross-paragraph reuse. While the parent parser sits in sync.Pool, that pins the document buffer. Add Transformer.Reset() that nils block + source, and call it from ParseContext via a small pooledParser wrapper before each Put. 4. Transformer doc said "one parser per goroutine" — tightened to accurately describe sync.Pool's per-Get exclusive-access semantics, and corrected *text.BlockReader -> text.BlockReader (interface). Add a dedicated transformer_test.go: 9 AST-equivalence cases against upstream goldmark (bare, three quote variants, angle destinations, indented-3, no-def, multi-def), plus tests for the reuse path, the Reset path, and the cross-source reallocation path. Bumps package self-coverage to 74.1 % and gives codecov's patch check the line-level coverage it wanted. Re-run BenchmarkCheckCorpusLarge confirms wrapper overhead is negligible: allocs/op still ~553k (matches the original PoC, the plan-197 PASS gate is unchanged).

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Goldmark allocates a fresh text.BlockReader per paragraph at parser/link_ref.go:18, even though the type has Reset() and parser.go:902 already runs ONE shared blockReader across the entire inline pass via Reset. Vendor a 200-line subset of parseLinkReferenceDefinition, parseLinkDestination, linkFindClosureOptions, and newASTReference into internal/goldmark/linkrefparagraph/. Replace goldmark's LinkReferenceParagraphTransformer with a per-parser Transformer that owns one BlockReader and Resets it for every paragraph. goroutine-safety: mdsmith's parserPool hands one parser per goroutine, so each parser's transformer is goroutine-local. BenchmarkCheckCorpusLarge -benchtime=10x -count=3: - allocs/op: 634,459 -> 553,143 (-12.8 %) - p95 wall : 264 ms -> 247 ms (-6.4 %) - bytes/op : 201 MB -> 192 MB (-4.5 %) - go test ./... and -race: green The 12.8 % saving is within 6 % of the 13.6 % review prediction, clearing the plan-197 pass gate (within 10 %, wall time <= baseline). Plan 198 picks up the remaining ~41 % structural ceiling (NewTextSegment + NewParagraph + Segments.Append) via a per-parse arena.

Copilot review on plan-197 PoC flagged four items: 1. NewParser registered only the custom link-ref transformer instead of starting from parser.DefaultParagraphTransformers() and substituting. Future goldmark versions may add defaults that would then silently disappear. Walk the default slice, replace the LinkReferenceParagraphTransformer entry at its original priority, pass the rest through unchanged. 2. doc.go referenced NewTransformer() and *text.BlockReader; the actual API is New() and the field is the interface text.BlockReader. Doc updated. 3. The Transformer keeps the last-parsed source []byte and the underlying BlockReader for cross-paragraph reuse. While the parent parser sits in sync.Pool, that pins the document buffer. Add Transformer.Reset() that nils block + source, and call it from ParseContext via a small pooledParser wrapper before each Put. 4. Transformer doc said "one parser per goroutine" — tightened to accurately describe sync.Pool's per-Get exclusive-access semantics, and corrected *text.BlockReader -> text.BlockReader (interface). Add a dedicated transformer_test.go: 9 AST-equivalence cases against upstream goldmark (bare, three quote variants, angle destinations, indented-3, no-def, multi-def), plus tests for the reuse path, the Reset path, and the cross-source reallocation path. Bumps package self-coverage to 74.1 % and gives codecov's patch check the line-level coverage it wanted. Re-run BenchmarkCheckCorpusLarge confirms wrapper overhead is negligible: allocs/op still ~553k (matches the original PoC, the plan-197 PASS gate is unchanged).

The Copilot-round commit (1f13b01) covered Transformer and the trivial astReference methods but left parseLinkReferenceDefinition at 66.7 % and parseLinkDestination at 71.9 %; codecov/patch tripped on the result. Expand the equivalence cases against upstream goldmark from 9 to 26: - multi-line label, title on next line, multi-line title - balanced and escaped destination parens, escaped angle-dest - negative paths: indent >= 4, no opener, unclosed label, blank label, no colon, missing dest, trailing content after dest/title, glued title, unclosed quote/angle Add direct unit tests for astReference.String() (via the parser.Reference interface returned by ctx.Reference) and an internal_test.go for sameSlice covering alias, distinct-backing, length-mismatch, nil, and empty-vs-nil paths. Coverage in this package: 73.9 % -> 90.5 %.

The b299bef equivalence test pack lifted package coverage to 90.5 %, but codecov's patch gate is target:auto — patch must be >= the 96.27 % project baseline. The PR was 88.65 % patch (22 lines missing). Add 9 more equivalence fixtures aimed at the remaining gaps: - dest-bad-rparen for parseLinkDestination's opened<0 break - empty-paragraph-link-ref and three-refs-paragraph for the Transform paragraph-fully-consumed paths - title-on-newline variants (title-newline-trail, unclosed-title, title-then-content-after-newline) for parseLinkReferenceDefinition's isNewLine + title-trail branches - tab/one-space continuation for indent-width parsing paths Package coverage: 90.5 % -> 97.5 %. The four still-uncovered ranges (parser.go width>3, width!=0, spaces==0; transformer.go lines.Len()==0 inside the removes loop) are vendored defensive branches that goldmark itself does not exercise from its block parser; their dead-ness is documented in the matching upstream branches.

Patch coverage rose 88.65 -> 95.36 with the previous round but still sat under the 96.31 project baseline. Two specific holes remained: - pkg/markdown/parser.go: substituteLinkRef's pass-through branch (out[i] = pv) was never executed because goldmark's DefaultParagraphTransformers ships only the link-ref entry. - internal/goldmark/linkrefparagraph/parser.go: the spaces==0 + opener-is-quote branch was the only failure path the equivalence fixtures hadn't reached. Add a direct TestSubstituteLinkRef_PreservesUnknownEntries unit test that builds a 3-entry defaults list (fake transformer at 200, LinkReferenceParagraphTransformer at 100, fake at 50) and asserts the result preserves all three slots and only the link-ref entry swaps. Add an "angle-then-title" equivalence case (`[a]: <foo>"title"`) that exercises parseLinkReferenceDefinition's spaces==0 path: the angle-destination consumer stops exactly at the `"` with zero intervening spaces. Package self-coverage: 97.5 -> 98.1. Three remaining uncovered ranges are vendored defensive guards (width > 3, width != 0, lines.Len() == 0 inside removes) that goldmark's block parser strips before the transformer sees them — verified by the sequential-3sp-indent / sequential-tab-indent fixtures that still hit the (width == 0) branch.

The plan-197 standalone linkrefparagraph fork is folded back into the vendored parser/ — it was scaffolding for a full fork that this commit ships. The vendor uses go.mod replace so every existing goldmark/* import resolves to the in-tree copy, no consumer changes required. Changes vs upstream: - parser/link_ref.go — linkReferenceParagraphTransformer now carries a reusable text.BlockReader (Reset per paragraph) plus a Reset() hook for pool consumers to drop the pinned source bytes before Put. The singleton var stays as a deprecated backwards-compat shim; DefaultParagraphTransformers returns a fresh transformer instance per call so each parser owns its own. - pkg/markdown/parser.go — pooledParser wrapper assertion-finds the linkRefResetter in DefaultParagraphTransformers and calls Reset() before parserPool.Put. BenchmarkCheckCorpusLarge stays at the plan-197 baseline: 553k allocs/op, p95 234ms — same lever, same savings, now living inside a self-contained fork instead of a side package. Plan 198 (per-parse arena absorbing NewTextSegment, NewParagraph, Segments backing arrays, FindClosure NewSegments — combined ~41% ceiling) lands as a subsequent commit on this branch.

Copilot

Pull request overview

Copilot reviewed 76 out of 77 changed files in this pull request and generated 3 comments.

CodeQL fires "incorrect-integer-conversion" on four sites that cast strconv.ParseUint(..., 16, 32) / (..., 10, 32) result (uint64) to rune (int32) for &#xNNN; and &#NNN; entity decoding: - internal/goldmark/util/util.go:618, 630 (UnescapeEntities) - internal/goldmark/renderer/html/html.go:888, 899 (escape path) The upstream code is safe in practice because the digit windows (i-start < 7 for hex, i-start < 8 for decimal) cap v far below int32 max, but the analyser cannot see that flow. Add an explicit `if v > 0x10FFFF { v = 0xFFFD }` guard at each site, which is a no-op in the supported range and downstream ToValidRune already maps the invalid case to 0xFFFD.

Copilot