Avoid quadratic-time parsing of large multiline values - #25
Conversation
Multiline continuation lines were appended to the option value with string concatenation (value += ...). Because Go strings are immutable, each continuation line reallocated and copied the whole accumulated value, making ParseReader run in O(n^2) time in the number of continuation lines belonging to a single option. A config with a single large multiline value is enough to trigger this: a ~600KB input took roughly 25 seconds to parse, and growth is quadratic, so a modestly larger file hangs the parser for minutes. Accumulate the value in a strings.Builder instead, which grows amortized O(1), so parsing is linear. Parsing behaviour and output are unchanged. Added a regression test that parses a large multiline value and fails on quadratic timing.
bigkevmcd
left a comment
bigkevmcd
left a comment
There was a problem hiding this comment.
Thank you for this!
My natural inclination is to convert TestParseLargeMultilineValue to a Benchmark test.
But otherwise the actual improvement is great.
| // the whole accumulated value on every line, making parsing O(n^2) in the number | ||
| // of continuation lines. A ~600KB input took tens of seconds to parse. | ||
| func TestParseLargeMultilineValue(t *testing.T) { | ||
| const n = 300000 |
There was a problem hiding this comment.
Wow...yes...this is very old code and string.Builder was added in 1.10 IIRC
Thank you!
Converting this test into a benchmark shows how much of a difference this makes:
BenchmarkParseLargeMultilineValue-8 39 32801434 ns/op
vs
BenchmarkParseLargeMultilineValue-8 1 25768411357 ns/op
|
|
||
| var b bytes.Buffer | ||
| b.WriteString("[section]\nkey = first\n") | ||
| for i := 0; i < n; i++ { |
There was a problem hiding this comment.
If we bump go.mod to require Go 1.22 this could be simplified to:
| for i := 0; i < n; i++ { | |
| for range n { |
|
Good call, turned it into a benchmark. Thanks! |
There was a problem hiding this comment.
🟡 Changes recommended
The performance regression check must be automatically executed and enforced.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Improves multiline-value parsing performance by replacing repeated string concatenation with strings.Builder.
Changes:
- Makes multiline accumulation amortized-linear.
- Adds a large-value parsing benchmark.
File summaries
| File | Changes |
|---|---|
configparser.go |
Uses strings.Builder for multiline values. |
configparser_multiline_test.go |
Adds a large multiline parsing benchmark; the performance check is not automatically enforced in CI. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // multiline value. Each continuation line used to be appended with string | ||
| // concatenation, which reallocates and copies the whole accumulated value on | ||
| // every line, making parsing O(n^2) in the number of continuation lines. | ||
| func BenchmarkParseLargeMultilineValue(b *testing.B) { |
While parsing config files with large multiline values, I noticed
ParseReadergets very slow well out of proportion to the input size.Cause
Each multiline continuation line is appended to the current option value with string concatenation:
Go strings are immutable, so every
+=allocates a new string and copies the entire accumulated value. For an option whose value spans n continuation lines this makes parsing O(n^2) in the number of lines, even though the file is read once.Impact
A single option with a large multiline value is enough to trigger it. On my machine:
Growth is quadratic, so a slightly larger file ties the parser up for minutes on a small input. Since
ParseReaderis the main entry point (also reached viaParse/NewConfigParserFromFile) and config data can come from untrusted sources, this is an easy denial-of-service.Fix
Accumulate the value in a
strings.Builder, which grows amortized O(1), so parsing is linear. Parsed values are byte-for-byte identical, so behaviour is unchanged. After the change the 200k case above drops from ~25 s to ~15 ms.Added a regression test that parses a large multiline value and fails if timing is quadratic (with a generous bound so it only trips on super-linear behaviour). Existing tests,
go vet, andgofmtall pass.