Merge pull request #361 from walles/johan/pre-allocated-getlines

Improve Search Cache Performance

Author: walles

internal/filteringReader.go

@@ -174,6 +174,16 @@ func (f *FilteringReader) GetLines(firstLine linemetadata.Index, wantedLineCount
 	}
 }
 
+func (f *FilteringReader) GetLinesPreallocated(firstLine linemetadata.Index, resultLines *[]reader.NumberedLine) string {
+	if f.shouldPassThrough() {
+		return f.BackingReader.GetLinesPreallocated(firstLine, resultLines)
+	}
+
+	lines := f.GetLines(firstLine, cap(*resultLines))
+	*resultLines = lines.Lines
+	return lines.StatusText
+}
+
 // In the general case, this will return a text like this:
 // "Filtered: 1234/5678 lines  22%"
 func (f *FilteringReader) createStatus(lastLine *linemetadata.Index) string {

internal/reader/reader.go

@@ -62,6 +62,13 @@ type Reader interface {
 	// that the returned first line may be different from the requested one.
 	GetLines(firstLine linemetadata.Index, wantedLineCount int) InputLines
 
+	// GetLines gets the indicated lines from the input. The lines will be stored
+	// in the provided preallocated slice to avoid allocations. The line count is
+	// determined by the capacity of the provided slice.
+	//
+	// The return value is the status text for the returned lines.
+	GetLinesPreallocated(firstLine linemetadata.Index, resultLines *[]NumberedLine) string
+
 	// False when paused. Showing the paused line count is confusing, because
 	// the user might think that the number is the total line count, even though
 	// we are not done yet.
@@ -944,7 +951,6 @@ func clipRangeToLength(start linemetadata.Index, wantedCount int, maxIndex int)
 // GetLines gets the indicated lines from the input
 func (reader *ReaderImpl) GetLines(firstLine linemetadata.Index, wantedLineCount int) InputLines {
 	reader.RLock()
-
 	if len(reader.lines) == 0 || wantedLineCount == 0 {
 		statusText := reader.createStatusUnlocked(firstLine)
 		reader.RUnlock()
@@ -953,29 +959,54 @@ func (reader *ReaderImpl) GetLines(firstLine linemetadata.Index, wantedLineCount
 			StatusText: statusText,
 		}
 	}
+	reader.RUnlock()
 
-	// Prevent reading past the end of the available lines
 	firstLineIndex, lastLineIndex := clipRangeToLength(firstLine, wantedLineCount, len(reader.lines)-1)
+	wantedLineCount = lastLineIndex - firstLineIndex + 1
+
+	resultLines := make([]NumberedLine, 0, wantedLineCount)
+	statusText := reader.GetLinesPreallocated(linemetadata.IndexFromZeroBased(firstLineIndex), &resultLines)
+
+	return InputLines{
+		Lines:      resultLines,
+		StatusText: statusText,
+	}
+}
+
+// GetLines gets the indicated lines from the input. The lines will be stored
+// in the provided preallocated slice to avoid allocations. The line count is
+// determined by the capacity of the provided slice.
+//
+// The return value is the status text for the returned lines.
+func (reader *ReaderImpl) GetLinesPreallocated(firstLine linemetadata.Index, resultLines *[]NumberedLine) string {
+	// Clear the result slice
+	*resultLines = (*resultLines)[:0]
+
+	reader.RLock()
+
+	if len(reader.lines) == 0 || cap(*resultLines) == 0 {
+		statusText := reader.createStatusUnlocked(firstLine)
+		reader.RUnlock()
+
+		return statusText
+	}
+
+	// Prevent reading past the end of the available lines
+	firstLineIndex, lastLineIndex := clipRangeToLength(firstLine, cap(*resultLines), len(reader.lines)-1)
 
 	statusText := reader.createStatusUnlocked(linemetadata.IndexFromZeroBased(lastLineIndex))
 
-	returnLines := make([]NumberedLine, 0, lastLineIndex-firstLineIndex+1)
 	for loopIndex, returnLine := range reader.lines[firstLineIndex : lastLineIndex+1] {
-		lineIndex := linemetadata.IndexFromZeroBased(firstLineIndex + loopIndex)
-		returnLines = append(returnLines, NumberedLine{
-			Index:  lineIndex,
+		*resultLines = append(*resultLines, NumberedLine{
+			Index:  linemetadata.IndexFromZeroBased(firstLineIndex + loopIndex),
 			Number: linemetadata.NumberFromZeroBased(firstLineIndex + loopIndex),
 			Line:   returnLine,
 		})
 	}
 
-	// Scary parts done, no lock needed anymore
 	reader.RUnlock()
 
-	return InputLines{
-		Lines:      returnLines,
-		StatusText: statusText,
-	}
+	return statusText
 }
 
 func (reader *ReaderImpl) PumpToStdout() {

internal/search-line-cache.go

@@ -10,22 +10,17 @@ import (
 // performance. But a larger cache without a performance increase has no value.
 // To evaluate:
 //
-//	go test -run='^$' -bench 'Search' ./internal
+//  go test -run='^$' -bench 'Warm' ./internal
 //
 // Results from Johan's laptop. The numbers are the test iteration counts for
-// BenchmarkHighlightedSearch and BenchmarkPlainTextSearch. The optimization has
-// been done to improve the sum of these two benchmarks.
+// BenchmarkHighlightedWarmSearch and BenchmarkPlainTextWarmSearch. The
+// optimization has been done to improve the sum of these two benchmarks.
 //
-// After introducing the RWMutex in reader.go these numbers became quite
-// volatile, they change a bunch from run to run so it's hard to draw obvious
-// conclusions. I ended up just picking one number.
-//
-//	 200: 387+368=755
-//	1000: 400+392=792
-//	2000: 382+394=776
-//	3000: 373+402=775
-//	4000: 393+412=805
-//	5000: 374+366=740
+//    100: 741+726=1467
+//    500: 832+882=1714 <-- Best
+//   1000: 754+810=1564
+//   5000: 637+658=1295
+
 const searchLineCacheSize = 500
 
 type searchLineCache struct {
@@ -48,20 +43,19 @@ func (c *searchLineCache) GetLine(reader reader.Reader, index linemetadata.Index
 		firstIndexToRequest = index.NonWrappingAdd(-searchLineCacheSize + 1)
 	}
 
-	lines := reader.GetLines(firstIndexToRequest, searchLineCacheSize)
-	if len(lines.Lines) == 0 {
-		// No lines at all
-		return nil
-	}
-
-	c.lines = lines.Lines
+	reader.GetLinesPreallocated(firstIndexToRequest, &c.lines)
 
 	// Get the line from the cache
 	return c.getLineFromCache(index)
 }
 
 // Or nil if that line isn't in the cache
 func (c *searchLineCache) getLineFromCache(index linemetadata.Index) *reader.NumberedLine {
+	if cap(c.lines) != searchLineCacheSize {
+		// Initialize cache
+		c.lines = make([]reader.NumberedLine, 0, searchLineCacheSize)
+	}
+
 	if len(c.lines) == 0 {
 		return nil
 	}

internal/search-linescanner_test.go

@@ -76,7 +76,10 @@ func benchmarkSearch(b *testing.B, highlighted bool, warm bool) {
 	assert.NilError(b, err)
 	fileContents := string(sourceBytes)
 
-	// Read one copy of the example input
+	// Repeat input enough times to get to some target size, before highlighting
+	// to get the same amount of text in either case
+	replications := 5_000_000 / len(fileContents)
+
 	if highlighted {
 		highlightedSourceCode, err := reader.Highlight(fileContents, *styles.Get("native"), formatters.TTY16m, lexers.Get("go"))
 		assert.NilError(b, err)
@@ -86,9 +89,11 @@ func benchmarkSearch(b *testing.B, highlighted bool, warm bool) {
 		fileContents = *highlightedSourceCode
 	}
 
-	// Repeat input enough times to get to some target size, before highlighting
-	// to get the same amount of text in either case
-	replications := 5_000_000 / len(fileContents)
+	if !warm {
+		// This makes the ns/op benchmark numbers more comparable between plain
+		// and highlighted in the cold case.
+		replications = 5_000_000 / len(fileContents)
+	}
 
 	// Create some input to search. Use a Builder to avoid quadratic string concatenation time.
 	var builder strings.Builder