Skip to content

Commit d8e30fa

Browse files
perf(sanitize): make clean text allocation-free on the hot path
Sanitizing user-authored response fields ran multiple allocating passes over every string regardless of content: FilterInvisibleCharacters converted the whole input to []rune and back, FilterCodeFenceMetadata split and rejoined every line, and bluemonday ran unconditionally. On comment- and issue-heavy responses this dominated conversion CPU and allocation. Three changes, none of which alter output or widen what the policy allows: - FilterInvisibleCharacters scans first and copies only from the first filtered rune, skipping ASCII runs without decoding them. Invalid UTF-8 is still re-encoded to U+FFFD, matching the []rune round trip it replaces. - FilterCodeFenceMetadata walks lines in place and returns the input when no line changes. - FilterHTMLTags skips bluemonday for input that is provably a fixed point of the policy: printable ASCII, TAB and LF, with none of the five characters html.EscapeString rewrites. Sanitize also skips the second invisible/code-fence pass when HTML normalization returned its input unchanged, since both filters are fixed points there. Equivalence is pinned by a verbatim copy of the previous pipeline: the new code is diffed against it over a corpus of ~22k deterministic cases plus two fuzz targets, and the fast path is checked byte by byte against the live bluemonday policy. Benchmarks (Intel Ultra 9 185H, n=6): Sanitize/TitleASCII 5.35µs -> 114ns 1 -100% allocs Sanitize/Comment1KiB 45.3µs -> 1.27µs 1 -100% allocs Sanitize/Body64KiB 2.47ms -> 85.9µs 1 -100% allocs 30 issues x 2KiB body 3.00ms -> 88.6µs 1.55MiB -> 1.9KiB 100 comments x 1KiB 5.04ms -> 169µs 2.19MiB -> 6.3KiB Content that genuinely needs rewriting still pays for it, and non-ASCII text still goes through bluemonday by design. Fixes #3117 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 08edfa8 commit d8e30fa

4 files changed

Lines changed: 906 additions & 21 deletions

File tree

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
package github
2+
3+
import (
4+
"strings"
5+
"testing"
6+
7+
"github.com/google/go-github/v89/github"
8+
)
9+
10+
// Benchmarks for the minimal converters that sanitize user-authored prose. These
11+
// model the response shapes called out in
12+
// https://github.com/github/github-mcp-server/issues/3117: a 30-issue listing
13+
// page and a 100-comment page.
14+
15+
func benchProse(n int) string {
16+
const para = "The converter allocates a new slice for every field it touches, which shows up " +
17+
"as GC pressure once the response contains a few hundred comments. Rework the hot path so " +
18+
"clean text is returned as-is. See the linked issue for measurements and the plan.\n\n" +
19+
"- item one\n- item two\n- item three\n\n"
20+
21+
var b strings.Builder
22+
b.Grow(n + len(para))
23+
for b.Len() < n {
24+
b.WriteString(para)
25+
}
26+
return b.String()[:n]
27+
}
28+
29+
func benchIssuePage(count, bodySize int) []*github.Issue {
30+
page := make([]*github.Issue, count)
31+
for i := range page {
32+
page[i] = &github.Issue{
33+
Number: github.Ptr(i + 1),
34+
Title: github.Ptr("Converter allocates on every sanitized field for large listing responses"),
35+
Body: github.Ptr(benchProse(bodySize + i)),
36+
State: github.Ptr("open"),
37+
User: &github.User{Login: github.Ptr("octocat")},
38+
}
39+
}
40+
return page
41+
}
42+
43+
func benchCommentPage(count, bodySize int) []*github.IssueComment {
44+
page := make([]*github.IssueComment, count)
45+
for i := range page {
46+
page[i] = &github.IssueComment{
47+
ID: github.Ptr(int64(i + 1)),
48+
Body: github.Ptr(benchProse(bodySize + i)),
49+
User: &github.User{Login: github.Ptr("octocat")},
50+
}
51+
}
52+
return page
53+
}
54+
55+
// BenchmarkConvertToMinimalIssuePage measures a 30-issue page with 2 KiB bodies.
56+
func BenchmarkConvertToMinimalIssuePage(b *testing.B) {
57+
page := benchIssuePage(30, 2048)
58+
b.ReportAllocs()
59+
for b.Loop() {
60+
for _, issue := range page {
61+
sinkIssue = convertToMinimalIssue(issue)
62+
}
63+
}
64+
}
65+
66+
// BenchmarkConvertToMinimalCommentPage measures a 100-comment page with 1 KiB bodies.
67+
func BenchmarkConvertToMinimalCommentPage(b *testing.B) {
68+
page := benchCommentPage(100, 1024)
69+
b.ReportAllocs()
70+
for b.Loop() {
71+
for _, comment := range page {
72+
sinkComment = convertToMinimalIssueComment(comment)
73+
}
74+
}
75+
}
76+
77+
var (
78+
sinkIssue MinimalIssue
79+
sinkComment MinimalIssueComment
80+
)

pkg/sanitize/bench_test.go

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
package sanitize
2+
3+
import (
4+
"strings"
5+
"testing"
6+
)
7+
8+
// benchCorpus holds the response shapes that dominate high-throughput
9+
// conversion: short titles, comment-sized bodies, large issue bodies, and the
10+
// adversarial content the sanitizer exists to neutralise.
11+
var benchCorpus = []struct {
12+
name string
13+
input string
14+
}{
15+
{"TitleASCII", benchTitleASCII},
16+
{"TitleUnicode", benchTitleUnicode},
17+
{"Comment1KiB", benchComment1KiB},
18+
{"Comment1KiBUnicode", benchComment1KiBUnicode},
19+
{"Body64KiB", benchBody64KiB},
20+
{"Body64KiBUnicode", benchBody64KiBUnicode},
21+
{"CodeFenceBody", benchCodeFenceBody},
22+
{"AdversarialHTML", benchAdversarialHTML},
23+
{"AdversarialUnicode", benchAdversarialUnicode},
24+
{"AdversarialMixed", benchAdversarialMixed},
25+
}
26+
27+
var (
28+
benchTitleASCII = "Fix flaky converter test for issue comments on large pages"
29+
benchTitleUnicode = "Fix flaky ✈️ converter test — 世界 for issue comments"
30+
31+
benchComment1KiB = buildClean(1024)
32+
benchComment1KiBUnicode = buildUnicode(1024)
33+
benchBody64KiB = buildClean(64 * 1024)
34+
benchBody64KiBUnicode = buildUnicode(64 * 1024)
35+
36+
benchCodeFenceBody = buildFenced(4096)
37+
38+
benchAdversarialHTML = strings.Repeat(
39+
"<script>alert(1)</script>Hello <b>bold</b> &#8203; <a href=\"https://example.com\" onclick=\"x\">link</a>\n",
40+
16,
41+
)
42+
benchAdversarialUnicode = strings.Repeat(
43+
"Hidden\u200B\u200C\u202Epayload\u202C\u2066here\u2069\uFE0F\U000E0101\U000E0102 \U0001F600\uFE0F ok\n",
44+
16,
45+
)
46+
benchAdversarialMixed = benchAdversarialHTML + benchAdversarialUnicode + benchCodeFenceBody
47+
)
48+
49+
// buildClean produces deterministic plain markdown prose of at least n bytes,
50+
// representative of an ordinary comment or issue body.
51+
func buildClean(n int) string {
52+
const para = "The converter allocates a new slice for every field it touches, which shows up " +
53+
"as GC pressure once the response contains a few hundred comments. Rework the hot path so " +
54+
"clean text is returned as-is. See the linked issue for measurements and the plan.\n\n" +
55+
"- item one\n- item two\n- item three\n\n"
56+
57+
var b strings.Builder
58+
b.Grow(n + len(para))
59+
for b.Len() < n {
60+
b.WriteString(para)
61+
}
62+
return b.String()[:n]
63+
}
64+
65+
// buildUnicode produces deterministic prose of at least n bytes containing
66+
// legitimate non-ASCII text (accents, CJK, emoji with variation selectors) that
67+
// the sanitizer must preserve untouched.
68+
func buildUnicode(n int) string {
69+
const para = "Der Konverter reserviert für jedes Feld einen neuen Puffer — 世界 — was sich als " +
70+
"GC-Druck zeigt. Ship it \U0001F600\uFE0F and \u2708\uFE0F today. 葛\U000E0100城 is a registered sequence.\n\n"
71+
72+
var b strings.Builder
73+
b.Grow(n + len(para))
74+
for b.Len() < n {
75+
b.WriteString(para)
76+
}
77+
// Trim on a rune boundary so the corpus stays valid UTF-8.
78+
s := b.String()
79+
for n > 0 && n < len(s) && s[n]&0xC0 == 0x80 {
80+
n--
81+
}
82+
return s[:n]
83+
}
84+
85+
// buildFenced produces deterministic prose of at least n bytes built from fenced
86+
// code blocks, exercising the code-fence filter's line splitting.
87+
func buildFenced(n int) string {
88+
const block = "Consider this snippet:\n\n```go\nfmt.Println(\"hi\")\nreturn nil\n```\n\nand this one:\n\n" +
89+
"```\nplain text block\n```\n\n"
90+
91+
var b strings.Builder
92+
b.Grow(n + len(block))
93+
for b.Len() < n {
94+
b.WriteString(block)
95+
}
96+
return b.String()
97+
}
98+
99+
func BenchmarkSanitize(b *testing.B) {
100+
for _, tc := range benchCorpus {
101+
b.Run(tc.name, func(b *testing.B) {
102+
b.SetBytes(int64(len(tc.input)))
103+
b.ReportAllocs()
104+
for b.Loop() {
105+
sinkString = Sanitize(tc.input)
106+
}
107+
})
108+
}
109+
}
110+
111+
func BenchmarkFilterInvisibleCharacters(b *testing.B) {
112+
for _, tc := range benchCorpus {
113+
b.Run(tc.name, func(b *testing.B) {
114+
b.SetBytes(int64(len(tc.input)))
115+
b.ReportAllocs()
116+
for b.Loop() {
117+
sinkString = FilterInvisibleCharacters(tc.input)
118+
}
119+
})
120+
}
121+
}
122+
123+
func BenchmarkFilterHTMLTags(b *testing.B) {
124+
for _, tc := range benchCorpus {
125+
b.Run(tc.name, func(b *testing.B) {
126+
b.SetBytes(int64(len(tc.input)))
127+
b.ReportAllocs()
128+
for b.Loop() {
129+
sinkString = FilterHTMLTags(tc.input)
130+
}
131+
})
132+
}
133+
}
134+
135+
func BenchmarkFilterCodeFenceMetadata(b *testing.B) {
136+
for _, tc := range benchCorpus {
137+
b.Run(tc.name, func(b *testing.B) {
138+
b.SetBytes(int64(len(tc.input)))
139+
b.ReportAllocs()
140+
for b.Loop() {
141+
sinkString = FilterCodeFenceMetadata(tc.input)
142+
}
143+
})
144+
}
145+
}
146+
147+
// BenchmarkSanitizeIssuePage models a 30-issue listing response: each issue
148+
// contributes a title and a 2 KiB body.
149+
func BenchmarkSanitizeIssuePage(b *testing.B) {
150+
bodies := makePage(30, 2048, buildClean)
151+
b.SetBytes(int64(pageBytes(bodies) + 30*len(benchTitleASCII)))
152+
b.ReportAllocs()
153+
for b.Loop() {
154+
for _, body := range bodies {
155+
sinkLen += len(Sanitize(benchTitleASCII)) + len(Sanitize(body))
156+
}
157+
}
158+
}
159+
160+
// BenchmarkSanitizeCommentPage models a 100-comment listing response with 1 KiB
161+
// bodies, the shape called out as the worst case in issue #3117.
162+
func BenchmarkSanitizeCommentPage(b *testing.B) {
163+
bodies := makePage(100, 1024, buildClean)
164+
b.SetBytes(int64(pageBytes(bodies)))
165+
b.ReportAllocs()
166+
for b.Loop() {
167+
for _, body := range bodies {
168+
sinkLen += len(Sanitize(body))
169+
}
170+
}
171+
}
172+
173+
func makePage(count, size int, build func(int) string) []string {
174+
page := make([]string, count)
175+
for i := range page {
176+
// Vary the offset so entries are not identical strings.
177+
page[i] = build(size + i)
178+
}
179+
return page
180+
}
181+
182+
func pageBytes(page []string) int {
183+
total := 0
184+
for _, s := range page {
185+
total += len(s)
186+
}
187+
return total
188+
}
189+
190+
var (
191+
sinkString string
192+
sinkLen int
193+
)

0 commit comments

Comments
 (0)