[feat] - tmp file diffs #2306

ahrav · 2024-01-16T15:19:57Z

Description:

This PR aims to reduce memory usage when scanning large repositories containing diffs that exceed 10MB. Diffs now write to temporary disk files instead of allocating in-memory buffers.

The 10MB threshold is configurable and could be adjusted based on gathering metrics on average diff size. The bytes.Buffer could also be pre-allocated to the average diff length to optimize memory allocation.

By streaming large diffs to disk rather than buffering fully in memory, we can alleviate memory pressure and OutOfMemory errors that occur when scanning extremely large repositories.

Checklist:

Tests passing (make test-community)?
Lint passing (make lint this requires golangci-lint)?

pkg/gitparse/gitparse.go

mcastorina · 2024-01-16T18:30:26Z

pkg/gitparse/gitparse.go

+// write handles writing diff data to either an in-memory buffer or a file, depending on the size.
+func (d *Diff) write(ctx context.Context, p []byte) error {


Nit (optional): This is more of an architectural suggestion -- I think it would be nice to implement the in-memory / file switch buffer as it's own package and use it in the Diff struct, similar to how DiskBufferReader is its own import. We could also add tests to show the switching works without the "business logic" of diff parsing.

Moved it to its own pkg.

pkg/gitparse/gitparse.go

ahrav · 2024-01-17T23:46:06Z

pkg/gitparse/gitparse.go

+}
+
+// Len returns the length of the storage.
+func (d *Diff) Len() int { return d.contentWriter.Len() }


These methods probably can be removed, but I was trying to avoid reaching into the contentWriter directly. This is also exported since we need access to this in the git source. Open to changing it if we feel these are providing any additional encapsulation.

Do you want to guard them behind the state like ReadCloser()? I find it a bit surprising that some of the read methods are guarded and some aren't.

I like that idea. Will guard them.

ahrav · 2024-01-18T14:35:53Z

pkg/gitparse/gitparse.go

 )

 const (
 	// defaultDateFormat is the standard date format for git.
 	defaultDateFormat = "Mon Jan 02 15:04:05 2006 -0700"

 	// defaultMaxDiffSize is the maximum size for a diff. Larger diffs will be cut off.
-	defaultMaxDiffSize = 1 * 1024 * 1024 * 1024 // 1GB
+	defaultMaxDiffSize = 2 * 1024 * 1024 * 1024 // 1GB


increased these since we now write to disk and shouldn't run into issues with large diffs or commits.

Nit: The comment should be updated too

ahrav · 2024-01-18T15:36:59Z

pkg/sources/git/git.go

@@ -1048,9 +1089,6 @@ func (s *Git) handleBinary(ctx context.Context, gitDir string, reporter sources.
 		return err
 	}
 	defer func() {
-		if err := fileReader.Close(); err != nil {


removed, since StdoutPipe clsoes the pipe after seeing the command exit:

// StdoutPipe returns a pipe that will be connected to the command's // standard output when the command starts. // // Wait will close the pipe after seeing the command exit....

mcastorina

I have a general question about the bufferedfilewriter package. What is the intended API for using it?

For example, is it meant to be written to all at once and then read from all at once? Can the contents be peeked at before reading? I think answering how it will be used (or needs to be used) will help shape the interface.

mcastorina · 2024-01-18T20:38:38Z

pkg/writers/buffered_file_writer/bufferedfilewriter.go

+// Len returns the number of bytes in the buffer.
+func (w *BufferedFileWriter) Len() int { return w.buf.Len() }
+
+// String returns the contents of the buffer as a string.
+func (w *BufferedFileWriter) String() string { return w.buf.String() }


These methods aren't exactly accurate, right? The contents might be on disk instead of in the buffer.

Oh yea good catch. This should be the size, since that's what we will use to determine if we want to chunk the diff in the git source.

ahrav · 2024-01-18T21:37:17Z

I have a general question about the bufferedfilewriter package. What is the intended API for using it?

For example, is it meant to be written to all at once and then read from all at once? Can the contents be peeked at before reading? I think answering how it will be used (or needs to be used) will help shape the interface.

Main use case is for our git handling of diff/commits. So multiple writes, but only a single read for now. No peek, no extra fancy stuff for now.

I purposely scoped this to be very specific for the time being. I don't currently forsee any imminent uses of this pkg, so I think the reduced interface is a decent starting point. If in the future we plan on using it elsewhere we can always adjust it to make it a bit more broad. (ie regular reading, reading in chunks, peeking, encrypting data, etc)

rosecodym

I have one concrete question about stringification, but taking a step back, I'm pretty concerned about big-banging this into the codebase. It's a pretty sophisticated I/O structure, used in many of our most popular sources, that interacts with the temp file system - something we've historically (and currently!) had nasty trouble with. It's also probably got a pretty complicated performance profile in which a bunch of variables we don't entirely control (disk latency, diff size, flush-to-disk threshold) combine to create what's most likely a non-monotonic performance curve. (I'm guessing that it's good for small diffs and good for large diffs but pretty bad for diffs right around the cutoff.)

Is there any way we can roll this out gradually? I know that it would make the transitional code itself kind of gnarly but this thing is big, and deep, and complicated, and touching risky things. This might be a good topic for a synchronous chat, if you want.

pkg/gitparse/gitparse.go

ahrav · 2024-01-23T04:16:02Z

failing tests look to be due to account lockout with Gitlab.

mcastorina · 2024-01-23T17:27:12Z

pkg/gitparse/gitparse.go

 )

 const (
 	// defaultDateFormat is the standard date format for git.
 	defaultDateFormat = "Mon Jan 02 15:04:05 2006 -0700"

 	// defaultMaxDiffSize is the maximum size for a diff. Larger diffs will be cut off.
-	defaultMaxDiffSize = 1 * 1024 * 1024 * 1024 // 1GB
+	defaultMaxDiffSize = 2 * 1024 * 1024 * 1024 // 1GB


Nit: The comment should be updated too

pkg/gitparse/gitparse.go

rosecodym

sorry, surprise plumber arrival, but i wanted to get my one comment in!

pkg/gitparse/gitparse.go

rosecodym · 2024-01-29T15:38:51Z

pkg/sources/git/git.go

+				reader, err := diff.ReadCloser()
+				if err != nil {
+					ctx.Logger().Error(err, "error creating reader for commits", "filename", fileName, "commit", hash, "file", diff.PathB)
+					return nil


Why don't we want to propagate this error up?

I think the idea is to prevent stopping a scan for an error for a single diff. Do we think this should warrant stopping the scan?

…flehog into feat-tmp-file-diffs

ahrav force-pushed the feat-tmp-file-diffs branch 2 times, most recently from 2119161 to a2897d9 Compare January 16, 2024 17:01

Write large diffs to tmp files

f2f13b2

ahrav force-pushed the feat-tmp-file-diffs branch from a2897d9 to f2f13b2 Compare January 16, 2024 17:18

mcastorina reviewed Jan 16, 2024

View reviewed changes

ahrav added 3 commits January 16, 2024 11:04

address comments

7d2452a

Move bufferedfilewriter to own pkg

0802a62

update test

0365023

ahrav commented Jan 17, 2024

View reviewed changes

ahrav and others added 2 commits January 17, 2024 15:48

swallow write err

57e5d6a

Merge branch 'main' into feat-tmp-file-diffs

dad92c5

ahrav requested a review from mcastorina January 17, 2024 23:49

ahrav marked this pull request as ready for review January 18, 2024 00:25

ahrav requested review from a team as code owners January 18, 2024 00:25

ahrav commented Jan 18, 2024

View reviewed changes

use buffer pool

fd41709

ahrav force-pushed the feat-tmp-file-diffs branch from f4cc7f1 to fd41709 Compare January 18, 2024 15:35

ahrav commented Jan 18, 2024

View reviewed changes

mcastorina reviewed Jan 18, 2024

View reviewed changes

use size vs len

da2ff88

ahrav requested review from mcastorina and a team January 18, 2024 21:37

rosecodym reviewed Jan 19, 2024

View reviewed changes

pkg/gitparse/gitparse.go Outdated Show resolved Hide resolved

pkg/gitparse/gitparse.go Outdated Show resolved Hide resolved

ahrav added 4 commits January 19, 2024 10:13

Merge branch 'main' into feat-tmp-file-diffs

b3260ff

use interface

73903e7

fix test

994c268

update comments

f8e87e9

ahrav reopened this Jan 23, 2024

ahrav mentioned this pull request Jan 23, 2024

[feat] - tmp file diffs #2330

Closed

2 tasks

update

cbd4ddf

mcastorina reviewed Jan 23, 2024

View reviewed changes

address comments

7aef649

ahrav requested a review from mcastorina January 23, 2024 21:40

ahrav and others added 3 commits January 24, 2024 07:06

remove bufferedfile writer

47544ad

Merge branch 'main' into feat-tmp-file-diffs

3c7a1d5

Merge branch 'main' into feat-tmp-file-diffs

a9ee71a

ahrav requested review from a team January 26, 2024 15:30

rosecodym reviewed Jan 29, 2024

View reviewed changes

ahrav and others added 4 commits January 29, 2024 08:25

address comments

419107e

adjust interface

1bd8b27

Merge branch 'main' into feat-tmp-file-diffs

c259b84

fix finalize

1485c9a

ahrav requested review from rosecodym and a team January 29, 2024 19:47

address comments

73a766f

rosecodym approved these changes Jan 30, 2024

View reviewed changes

ahrav and others added 6 commits January 30, 2024 10:43

lint

1699097

remove guard

3ac95f5

Merge branch 'main' into feat-tmp-file-diffs

e442142

fix

bff34a4

Merge branch 'feat-tmp-file-diffs' of github.com:trufflesecurity/truf…

fd5181e

…flehog into feat-tmp-file-diffs

add TODO

1c577db

ahrav merged commit 7c59ff9 into main Jan 30, 2024
10 checks passed

ahrav deleted the feat-tmp-file-diffs branch January 30, 2024 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] - tmp file diffs #2306

[feat] - tmp file diffs #2306

ahrav commented Jan 16, 2024 •

edited

Loading

mcastorina Jan 16, 2024

ahrav Jan 17, 2024

ahrav Jan 17, 2024

rosecodym Jan 30, 2024

ahrav Jan 30, 2024

ahrav Jan 18, 2024

mcastorina Jan 23, 2024

ahrav Jan 18, 2024

mcastorina left a comment

mcastorina Jan 18, 2024

ahrav Jan 18, 2024

ahrav commented Jan 18, 2024 •

edited

Loading

rosecodym left a comment

ahrav commented Jan 23, 2024

mcastorina Jan 23, 2024

rosecodym left a comment

rosecodym Jan 29, 2024

ahrav Jan 29, 2024

		// write handles writing diff data to either an in-memory buffer or a file, depending on the size.
		func (d *Diff) write(ctx context.Context, p []byte) error {

[feat] - tmp file diffs #2306

[feat] - tmp file diffs #2306

Conversation

ahrav commented Jan 16, 2024 • edited Loading

Description:

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcastorina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrav commented Jan 18, 2024 • edited Loading

rosecodym left a comment

Choose a reason for hiding this comment

ahrav commented Jan 23, 2024

Choose a reason for hiding this comment

rosecodym left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrav commented Jan 16, 2024 •

edited

Loading

ahrav commented Jan 18, 2024 •

edited

Loading