Skip to content

Commit a7fed9e

Browse files
committed
Pool the dual parser in flavor.Detect; inline onlyAccept
Self-review found two issues from the plan-185 changes: 1. Detect built a fresh goldmark parser per call via NewPooledParser, running the full Extend hook chain on every Check. The previous singleton in internal/rules/markdownflavor avoided this; the move to a stateless public Detect regressed it. Add a sync.Pool inside the flavor package that hands each Detect goroutine its own parser-with-reset pair, mirroring internal/schema's contentParserPool. The pool resets the link-reference transformer before Put so idle slots do not pin document bytes. 2. fix.go used a one-line onlyAccept helper that was only ever called from one site. Inline the closure literal at the call site and drop the helper. A new BenchmarkDetectReusesPool exercises the dual-parser code path repeatedly; coverage in pkg/markdown/flavor stays at 100%. https://claude.ai/code/session_0144ZKUS2Zrg7xBft54qyoti
1 parent 0dc13f9 commit a7fed9e

3 files changed

Lines changed: 73 additions & 15 deletions

File tree

internal/rules/markdownflavor/fix.go

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,10 @@ func (r *Rule) fixByteRangeFeatures(f *lint.File) []byte {
4444

4545
if !flavor.Supports(r.Flavor, flavor.FeatureBareURLAutolinks) {
4646
doc := &markdown.Document{Body: f.Source, AST: f.AST}
47-
for _, fin := range flavor.Detect(doc, onlyAccept(flavor.FeatureBareURLAutolinks)) {
47+
acceptBareURLs := func(feat flavor.Feature) bool {
48+
return feat == flavor.FeatureBareURLAutolinks
49+
}
50+
for _, fin := range flavor.Detect(doc, acceptBareURLs) {
4851
edits = append(edits, wrapBareURL(f.Source, fin))
4952
}
5053
}
@@ -55,12 +58,6 @@ func (r *Rule) fixByteRangeFeatures(f *lint.File) []byte {
5558
return applyEdits(f.Source, edits)
5659
}
5760

58-
// onlyAccept returns an accept predicate that admits only feat. Used
59-
// by Fix when re-running detection for a single feature.
60-
func onlyAccept(feat flavor.Feature) func(flavor.Feature) bool {
61-
return func(f flavor.Feature) bool { return f == feat }
62-
}
63-
6461
// needsAnyDualFix reports whether any dual-parser fixable feature is
6562
// unsupported by the configured flavor. Skips the dual re-parse for
6663
// flavors that accept every dual feature (e.g. flavor.FlavorAny,

pkg/markdown/flavor/bench_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package flavor
2+
3+
import (
4+
"testing"
5+
6+
"github.com/jeduden/mdsmith/pkg/markdown"
7+
)
8+
9+
// BenchmarkDetectReusesPool exercises the dual-parser code path
10+
// repeatedly to confirm the sync.Pool inside Detect avoids
11+
// rebuilding the goldmark parser per call. Each iteration parses a
12+
// small document that triggers the dual pass.
13+
func BenchmarkDetectReusesPool(b *testing.B) {
14+
src := []byte("# Title {#top}\n\n" +
15+
"- [ ] task\n\n" +
16+
"| a | b |\n| - | - |\n| 1 | 2 |\n\n" +
17+
"~~old~~ text\n")
18+
doc := markdown.Parse(src)
19+
b.ResetTimer()
20+
b.ReportAllocs()
21+
for i := 0; i < b.N; i++ {
22+
_ = Detect(doc, nil)
23+
}
24+
}

pkg/markdown/flavor/detect.go

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@ import (
44
"bytes"
55
"regexp"
66
"sort"
7+
"sync"
78

89
"github.com/yuin/goldmark/ast"
910
extast "github.com/yuin/goldmark/extension/ast"
11+
"github.com/yuin/goldmark/parser"
1012
"github.com/yuin/goldmark/text"
1113

1214
"github.com/jeduden/mdsmith/pkg/markdown"
@@ -66,6 +68,26 @@ var bareURLPattern = regexp.MustCompile(
6668
"`" + `]*)?`,
6769
)
6870

71+
// detectParserPool reuses dual-parser instances across Detect calls.
72+
// Building one parser fans out goldmark.New plus every extension's
73+
// Extend hook; over a 600-file workspace check that cost shows up as
74+
// a measurable fraction of CPU and allocations. parser.Parser is
75+
// only safe to reuse sequentially within a single goroutine, so the
76+
// pool hands each Detect goroutine its own instance and clears the
77+
// link-reference transformer's pinned document bytes via the paired
78+
// reset closure before Put. Mirrors the schema content-parser pool.
79+
type pooledDetectParser struct {
80+
parser parser.Parser
81+
reset func()
82+
}
83+
84+
var detectParserPool = sync.Pool{
85+
New: func() any {
86+
p, reset := NewPooledParser()
87+
return &pooledDetectParser{parser: p, reset: reset}
88+
},
89+
}
90+
6991
// Detect runs every feature detector against doc and returns findings
7092
// in document-body order. accept is an optional predicate: when
7193
// non-nil, only features for which accept(feat) returns true are
@@ -88,14 +110,7 @@ func Detect(doc *markdown.Document, accept func(Feature) bool) []Finding {
88110
var out []Finding
89111

90112
if anyDualFeatureAccepted(keep) {
91-
dualParser, reset := NewPooledParser()
92-
defer reset()
93-
dualDoc := dualParser.Parse(text.NewReader(source))
94-
for _, fin := range detectFromDual(source, dualDoc) {
95-
if keep(fin.Feature) {
96-
out = append(out, fin)
97-
}
98-
}
113+
out = append(out, dualFindings(source, keep)...)
99114
}
100115

101116
if keep(FeatureBareURLAutolinks) {
@@ -112,6 +127,28 @@ func Detect(doc *markdown.Document, accept func(Feature) bool) []Finding {
112127
return out
113128
}
114129

130+
// dualFindings runs the dual parser via the pooled parser instance,
131+
// walks the resulting AST, and returns the keep-filtered findings.
132+
// Borrows a parser for the duration of the parse-plus-walk only, so
133+
// the parser's link-ref transformer cannot pin source bytes between
134+
// calls.
135+
func dualFindings(source []byte, keep func(Feature) bool) []Finding {
136+
pp := detectParserPool.Get().(*pooledDetectParser)
137+
defer func() {
138+
pp.reset()
139+
detectParserPool.Put(pp)
140+
}()
141+
dualDoc := pp.parser.Parse(text.NewReader(source))
142+
all := detectFromDual(source, dualDoc)
143+
out := make([]Finding, 0, len(all))
144+
for _, fin := range all {
145+
if keep(fin.Feature) {
146+
out = append(out, fin)
147+
}
148+
}
149+
return out
150+
}
151+
115152
// anyDualFeatureAccepted reports whether any feature detected by the
116153
// dual-parser pass is wanted. Lets Detect skip the goldmark re-parse
117154
// when every feature it would detect is already supported by the

0 commit comments

Comments
 (0)