[THOG-315] Replace bytes.buffer with strings.builder.

[ahrav]

What?

Replace the use of bytes.buffer with strings.Builder.

Why?

strings.Builder is a decent bit more efficient. It uses less memory and performs better overall.

How?

Replace usage directly.

Testing?

After running some profiling tests on some of the larger repos/orgs I was able to determine during the gitScan as we were adding git diffs to be scanned the memory allocation for bytest.buffer was ballooning. After changing it to use strings.builder that huge memory allocation was gone.

Screenshots (optional)

Benchmark of the two.

Anything Else?

[dustin-decker]

What about the fact that we're now converting a string back into bytes? I suspect the benchmarks did not capture the full picture.

[dustin-decker]

Before it was just copying bytes around I think

[ahrav]

Yea actually the performance benefit here might not be worth the change.

I was more curious about this portion:

trufflehog/pkg/sources/git/git.go

Lines 334 to 351 in 9a4d139

	for _, frag := range file.TextFragments {
	var sb strings.Builder
	newLineNumber := frag.NewPosition
	for _, line := range frag.Lines {
	if line.Op == gitdiff.OpAdd {
	sb.WriteString(line.Line)
	}
	}
	log.WithField("fragment", sb.String()).Trace("detecting fragment")
	metadata := s.sourceMetadataFunc(fileName, email, hash, when, urlMetadata, newLineNumber)
	chunksChan <- &sources.Chunk{
	SourceName: s.sourceName,
	SourceID: s.sourceID,
	SourceType: s.sourceType,
	SourceMetadata: metadata,
	Data: []byte(sb.String()),
	Verify: s.verify,
	}

Because when i was profiling with large repos the WriteString was eating a lot of memory in comparison to strings.Builder.

[ahrav]

Here even with the additional conversion to []byte(sb.String()) benchmarks show it being significantly faster.
This converts the strings.Builder back into []bytes as part of the function.

[ahrav]

[mcastorina]

It looks like some profiling files got committed by accident: pkg/engine/profile.out and pkg/engine/memprofile.out.

[ahrav]

It looks like some profiling files got committed by accident: pkg/engine/profile.out and pkg/engine/memprofile.out.

THanks! Not sure how those slipped in.

[dustin-decker]

I could imagine the the git source change being a small improvement because it's handling strings multiple times, but probably not the detector changes, because those are pure byte operations.
Hard to be certain without seeing the benchmarking code.
The detector code path is only hit when the keyword match passes anyway, so not worth much effort to shave a few nano seconds off - we have much bigger things to address first. Optimizing the git scan path is far more payoff than that.

Also, engine.test binary file slipped in. We should add the afformentioned files and *.test to gitignore.

[bill-rich]

Thats interesting that strings.Builder is so much better on memory. Do you know what its doing differently from bytes.Buffer?

[ahrav]

Thats interesting that strings.Builder is so much better on memory. Do you know what its doing differently from bytes.Buffer?

I believe one of the major changes is a copy check mechanism which saves a good deal of space.
https://github.com/golang/go/blob/3058d38632aea679c96cd41156b2751c97578a2d/src/strings/builder.go#L20-L26

It also prevents it's internal byte slice from escaping such that it can only grow and reset making it immutable.

[dustin-decker]

Makes me wonder if we'd benefit from reusing the buffer and calling Reset().
Right now there are allocations for each fragment.

https://pkg.go.dev/bytes#Buffer.Reset

[ahrav]

Makes me wonder if we'd benefit from reusing the buffer and calling Reset(). Right now there are allocations for each fragment.

https://pkg.go.dev/bytes#Buffer.Reset

Ohhhh indeed. I think that actually might even be better. Let me submit this and test that as well.