Skip to content

Commit 637dfa7

Browse files
committed
perf(tools): derive the changed-line span from bytes, not from per-line slices
Reported by @jatmn. changedLineSpan split both file versions into []string, so it allocated one string header per line for EACH version — work that scales with the size of the file rather than the size of the edit. countLines then split a third time just to take a length. All of it ran before RecordEdit had checked whether there was a tracked observation to update at all. He measured 32,036,640 bytes for two 2 MB versions of a million short lines, and extrapolated roughly 1.6 GB of headers for a 100 MB file before counting the content, the updated bytes and the hashes already live beside them. The memory was not the sharp end. edit_file writes the updated bytes BEFORE calling RecordEdit, so an out-of-memory kill lands after the user's file has changed and before the tracker baseline catches up: the file and the record of it disagree, and nothing says so. His instruction was to fix the allocation source rather than add a size guard, and that a guard — if used at all — would have to reject before os.WriteFile rather than during post-write re-baselining. Taken as written: there is no new limit, and the same edits are accepted as before. Two identical byte prefixes share every line that ends inside them, so counting newlines in the common prefix counts the unchanged leading lines directly. Same for the suffix, with one rule that the line-array version got for free: the line a shared tail STARTS in is only shared when the tail begins on a line boundary in both versions, or its opening bytes differ and it is a changed line that merely ends the same way. Measured on his own scenario, two 2 MB versions: changedLineSpan 32,036,640 bytes -> 0 allocations countLines per-line slice -> 0 allocations ## The equivalence is tested, not asserted A rewrite of a diff primitive is only safe if it is behaviour-preserving, so the replaced implementation is kept in the test file as an oracle and both are asked the same questions. That caught 206 mismatches in my first attempt at the byte scan — empty versions, and trailing lines that are only partially shared. Neither appears in the hand-written cases, and I would have shipped it. Two regressions: the oracle comparison, and an absolute allocation ceiling rather than a ratio, so a future rewrite that reintroduces per-line slices fails rather than merely getting slower. Mutation: restoring strings.Split in either function allocates 3.2 MB and 3.6 MB against a 4 KB bound. splitLinesForTracking lost its last caller and is removed — the same obsolete- helper lint failure that broke Windows CI on #911, caught here by running make lint-static before pushing rather than after. 0 issues. Rebased onto ad34dc8, 0 behind. go test -race ./internal/tools/: clean. Pre-existing here and on main: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-c962d7 | Claude Code | 17 prompts Origin-Snapshot: a599377c09e0
1 parent 6d59074 commit 637dfa7

2 files changed

Lines changed: 189 additions & 26 deletions

File tree

‎internal/tools/file_tracker.go‎

Lines changed: 106 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package tools
22

33
import (
4+
"bytes"
45
"crypto/sha256"
56
"encoding/hex"
67
"errors"
@@ -264,21 +265,88 @@ func (tracker *FileTracker) RecordEdit(absPath string, before, after []byte, inf
264265
// replacement spans: one edit_file call with replace_all can rewrite many
265266
// scattered occurrences, and the span between the outermost two is the only
266267
// region that is honestly unknown afterwards.
268+
// The span is derived by scanning BYTES, not by materialising lines.
269+
//
270+
// The first version split both versions into []string. That allocates one string
271+
// header per line for each version — work that scales with the SIZE OF THE FILE
272+
// rather than the size of the edit, and it ran before RecordEdit had even checked
273+
// whether there was a tracked observation to update. @jatmn measured 32,036,640
274+
// bytes for two 2 MB versions of a million short lines, and extrapolated roughly
275+
// 1.6 GB of headers for a 100 MB file before counting the content and hashes
276+
// already live alongside them.
277+
//
278+
// That mattered beyond memory pressure: edit_file writes the updated bytes BEFORE
279+
// calling RecordEdit, so an out-of-memory kill lands after the user's file has
280+
// changed and before the tracker baseline catches up — the file and the record of
281+
// it disagree, and nothing says so.
282+
//
283+
// Bytes give the same answer in bounded memory. Two identical byte prefixes share
284+
// every line that ends inside them, so counting newlines in the common prefix
285+
// counts the unchanged leading lines directly.
267286
func changedLineSpan(before, after string) (firstChanged, lastChangedBefore, delta int) {
268-
beforeLines := splitLinesForTracking(before)
269-
afterLines := splitLinesForTracking(after)
270-
prefix := 0
271-
for prefix < len(beforeLines) && prefix < len(afterLines) && beforeLines[prefix] == afterLines[prefix] {
272-
prefix++
273-
}
274-
suffix := 0
275-
for suffix < len(beforeLines)-prefix && suffix < len(afterLines)-prefix &&
276-
beforeLines[len(beforeLines)-1-suffix] == afterLines[len(afterLines)-1-suffix] {
277-
suffix++
278-
}
279-
return prefix + 1, len(beforeLines) - suffix, len(afterLines) - len(beforeLines)
287+
beforeCount := countTrackedLines(before)
288+
afterCount := countTrackedLines(after)
289+
290+
prefix := commonLinePrefix(before, after)
291+
// Never more leading lines than the shorter version has. The byte scan can
292+
// otherwise claim a line that only one side possesses — two empty versions
293+
// share a whole "line" that neither actually contains.
294+
prefix = min(prefix, min(beforeCount, afterCount))
295+
// The suffix may not reach back past the lines the prefix already claimed,
296+
// exactly as the line-array loop bounded itself.
297+
suffix := commonLineSuffix(before, after, min(beforeCount-prefix, afterCount-prefix))
298+
return prefix + 1, beforeCount - suffix, afterCount - beforeCount
299+
}
300+
301+
// commonLinePrefix counts the leading lines that are byte-identical in both.
302+
func commonLinePrefix(before, after string) int {
303+
limit := min(len(before), len(after))
304+
at := 0
305+
for at < limit && before[at] == after[at] {
306+
at++
307+
}
308+
lines := strings.Count(before[:at], "\n")
309+
// A LINE THAT ENDS EXACTLY WHERE THE SCAN STOPPED IS STILL SHARED. Scanning
310+
// halts where the bytes diverge OR where the shorter version runs out, and in
311+
// the second case the line containing that point can be complete and equal on
312+
// both sides — "a\nb" against "a\nb\nc" diverges only because the first ended,
313+
// and "b" is a whole matching line in each.
314+
if lineBoundary(before, at) && lineBoundary(after, at) {
315+
lines++
316+
}
317+
return lines
318+
}
319+
320+
// commonLineSuffix counts the trailing lines that are byte-identical in both, up
321+
// to the ceiling the prefix leaves.
322+
func commonLineSuffix(before, after string, ceiling int) int {
323+
if ceiling <= 0 {
324+
return 0
325+
}
326+
limit := min(len(before), len(after))
327+
back := 0
328+
for back < limit && before[len(before)-1-back] == after[len(after)-1-back] {
329+
back++
330+
}
331+
beforeAt, afterAt := len(before)-back, len(after)-back
332+
// Each newline inside the shared tail closes a line that lies wholly within
333+
// it. The line the tail STARTS in is shared only when the tail begins on a
334+
// line boundary in both versions — otherwise its opening bytes differ and it
335+
// is a changed line that merely ends the same way.
336+
lines := strings.Count(before[beforeAt:], "\n")
337+
if lineStart(before, beforeAt) && lineStart(after, afterAt) {
338+
lines++
339+
}
340+
return min(lines, ceiling)
280341
}
281342

343+
// lineBoundary reports whether at is the end of a line: the end of the text, or
344+
// the position of the newline that closes it.
345+
func lineBoundary(text string, at int) bool { return at == len(text) || text[at] == '\n' }
346+
347+
// lineStart reports whether at begins a line.
348+
func lineStart(text string, at int) bool { return at == 0 || text[at-1] == '\n' }
349+
282350
// changedByteSpan is changedLineSpan in bytes: the first differing offset, the
283351
// end offset of the changed region in BEFORE, and the size delta.
284352
func changedByteSpan(before, after []byte) (firstChanged, lastChangedBefore, delta int) {
@@ -294,13 +362,6 @@ func changedByteSpan(before, after []byte) (firstChanged, lastChangedBefore, del
294362
return prefix, len(before) - suffix, len(after) - len(before)
295363
}
296364

297-
func splitLinesForTracking(text string) []string {
298-
if text == "" {
299-
return nil
300-
}
301-
return strings.Split(text, "\n")
302-
}
303-
304365
// countLines reports the line count a READER would give the content, which is
305366
// what observation.total is compared against.
306367
//
@@ -311,15 +372,34 @@ func splitLinesForTracking(text string) []string {
311372
// every text file ends in a newline, so this was the common case rather than an
312373
// edge one.
313374
//
314-
// splitLinesForTracking keeps its own behaviour: changedLineSpan diffs the two
315-
// slices against each other, where the trailing element is harmless because both
316-
// sides carry it.
375+
// countTrackedLines keeps the OTHER rule deliberately: the span arithmetic has to
376+
// match the indices strings.Split produced, so it counts the empty element after
377+
// a trailing newline where this one does not. Two rules, two names, both stated —
378+
// they were one function with an if, and that is how they were confused.
317379
func countLines(content []byte) int {
318-
lines := splitLinesForTracking(string(content))
319-
if n := len(lines); n > 0 && lines[n-1] == "" {
320-
return n - 1
380+
// Counted, not split. This only ever needed a number, and building a slice of
381+
// every line to take its length was the third full per-line allocation in the
382+
// same call path.
383+
if len(content) == 0 {
384+
return 0
385+
}
386+
lines := bytes.Count(content, []byte{'\n'})
387+
if content[len(content)-1] != '\n' {
388+
lines++
389+
}
390+
return lines
391+
}
392+
393+
// countTrackedLines is countLines' rule for the string form: the number of
394+
// elements strings.Split would produce, which counts the empty element after a
395+
// trailing newline. The two differ deliberately — observation.total compares
396+
// against what a READER reports, while the span arithmetic has to match the
397+
// indices the split produced.
398+
func countTrackedLines(text string) int {
399+
if text == "" {
400+
return 0
321401
}
322-
return len(lines)
402+
return strings.Count(text, "\n") + 1
323403
}
324404

325405
// coversFully reports whether ranges together cover every line in [start, end].

‎internal/tools/file_tracker_test.go‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package tools
33
import (
44
"os"
55
"path/filepath"
6+
"strings"
67
"testing"
78
)
89

@@ -153,3 +154,85 @@ func TestNilFileTrackerCreatedFilesIsANoop(t *testing.T) {
153154
t.Fatalf("CreatedFiles() on nil tracker = %v, want nil", got)
154155
}
155156
}
157+
158+
// THE SPAN IS DERIVED IN BOUNDED MEMORY, whatever the file size.
159+
//
160+
// changedLineSpan used to split both versions into []string, so it allocated one
161+
// string header per line for each — work scaling with the SIZE OF THE FILE rather
162+
// than the size of the edit, and it ran before RecordEdit had even checked whether
163+
// there was an observation to update. @jatmn measured 32,036,640 bytes for two
164+
// 2 MB versions of a million short lines.
165+
//
166+
// The cost was not only memory. edit_file writes the updated bytes BEFORE calling
167+
// RecordEdit, so an out-of-memory kill lands after the user's file has changed and
168+
// before the tracker baseline catches up — file and record disagree, silently.
169+
//
170+
// This asserts an absolute ceiling rather than a ratio, so a future rewrite that
171+
// reintroduces per-line slices fails here rather than merely getting slower.
172+
func TestTheChangedSpanDoesNotAllocatePerLine(t *testing.T) {
173+
before := strings.Repeat("x\n", 200_000)
174+
after := before[:len(before)-2] + "y\n"
175+
176+
spanBytes := testing.Benchmark(func(b *testing.B) {
177+
b.ReportAllocs()
178+
for i := 0; i < b.N; i++ {
179+
changedLineSpan(before, after)
180+
}
181+
}).AllocedBytesPerOp()
182+
if spanBytes > 4096 {
183+
t.Errorf("changedLineSpan allocated %d bytes for a %d-line pair; per-line slices are back", spanBytes, 200_000)
184+
}
185+
186+
content := []byte(before)
187+
countBytes := testing.Benchmark(func(b *testing.B) {
188+
b.ReportAllocs()
189+
for i := 0; i < b.N; i++ {
190+
countLines(content)
191+
}
192+
}).AllocedBytesPerOp()
193+
if countBytes > 4096 {
194+
t.Errorf("countLines allocated %d bytes to return a number", countBytes)
195+
}
196+
}
197+
198+
// AND IT GIVES THE SAME ANSWERS THE LINE-ARRAY VERSION DID. The rewrite is only
199+
// safe if it is behaviour-preserving, so the replaced implementation is kept here
200+
// as an oracle and both are asked the same questions. This caught 206 mismatches
201+
// in the first attempt at the byte scan — empty versions and partially shared
202+
// trailing lines, neither of which the hand-written cases below would have found.
203+
func TestTheByteScanAgreesWithTheLineSplit(t *testing.T) {
204+
oracle := func(before, after string) (int, int, int) {
205+
split := func(text string) []string {
206+
if text == "" {
207+
return nil
208+
}
209+
return strings.Split(text, "\n")
210+
}
211+
beforeLines, afterLines := split(before), split(after)
212+
prefix := 0
213+
for prefix < len(beforeLines) && prefix < len(afterLines) && beforeLines[prefix] == afterLines[prefix] {
214+
prefix++
215+
}
216+
suffix := 0
217+
for suffix < len(beforeLines)-prefix && suffix < len(afterLines)-prefix &&
218+
beforeLines[len(beforeLines)-1-suffix] == afterLines[len(afterLines)-1-suffix] {
219+
suffix++
220+
}
221+
return prefix + 1, len(beforeLines) - suffix, len(afterLines) - len(beforeLines)
222+
}
223+
224+
versions := []string{
225+
"", "a", "\n", "a\n", "a\nb", "a\nb\n", "a\nb\nc", "a\nb\nc\n",
226+
"\n\n", "x\n\ny", "a\nb\nc\nd\ne", "same\nsame\nsame\n", "ab\nc", "b\nc",
227+
}
228+
for _, before := range versions {
229+
for _, after := range versions {
230+
wantFirst, wantLast, wantDelta := oracle(before, after)
231+
gotFirst, gotLast, gotDelta := changedLineSpan(before, after)
232+
if wantFirst != gotFirst || wantLast != gotLast || wantDelta != gotDelta {
233+
t.Errorf("changedLineSpan(%q, %q) = (%d, %d, %d), want (%d, %d, %d)",
234+
before, after, gotFirst, gotLast, gotDelta, wantFirst, wantLast, wantDelta)
235+
}
236+
}
237+
}
238+
}

0 commit comments

Comments
 (0)