Repository navigation
Stream printed diffs instead of buffering them three times - #101
Merged
Merged
Conversation
Printing a large diff used to hold several copies of it at once: PrintHunks built a growing buffer per file, PrintFileDiff copied it into a second buffer and PrintMultiFileDiff copied every file into a third, each growing by doubling. Sourcegraph's frontend prints whole repository comparisons this way, and diffs of hundreds of MiB have contributed to OOMs there. WriteMultiFileDiff and WriteFileDiff now write directly to an io.Writer, with hunk bodies passed through without copying and header lines assembled in one reused scratch slice instead of through fmt. Callers that can stream (such as an HTTP response) no longer need the printed diff in memory at all. The Print functions are built on the same code and keep their output byte-for-byte, including returning nil for empty output. They first measure the output with a counting writer and then fill a buffer of exactly that size, so the result is allocated once. For a 148 MiB diff of 200k files this cuts allocation from 816 MiB in 3.4M mallocs to 148 MiB in 5, and peak RSS growth from ~475 MiB to 148 MiB; for a single 121 MiB hunk, from 363 MiB to 121 MiB. Despite formatting twice, printing is about 10x faster. Amp-Thread-ID: https://ampcode.com/threads/T-01a15907-a942-77c9-ab83-3ff2545b0c9d Co-authored-by: Amp <amp@ampcode.com>
Member
Author
|
aah I can merge without review, will do that. Will respond to post merge feedback :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Printing a big diff used to keep about three copies of it in memory.
PrintHunksbuilt a growing buffer per file,PrintFileDiffcopied that into a second one, andPrintMultiFileDiffcopied each file into a third. Each buffer grew by doubling. Sourcegraph's frontend prints whole repo comparisons this way, and diffs of hundreds of MiB have contributed to OOMs there.This adds
WriteMultiFileDiff(w, ds)andWriteFileDiff(w, d), which write straight to anio.Writer. Hunk bodies go through as-is, and header lines are built in one reused scratch slice instead of withfmt. ThePrint*functions now use the same code. They do a quick counting pass first, then fill a buffer allocated at exactly the output size. Output is byte-for-byte the same, includingnilfor empty output. I checked that against the old implementation on all of testdata, a set of edge cases and 20k random diffs.Numbers from a 200k-file diff and a single huge hunk, each run in its own process:
io.Discardio.DiscardYep, it's also about 10× faster even though it formats twice. Most of the old time went into filling freshly allocated memory.
Callers that can stream, like an HTTP handler, should use
WriteMultiFileDiffand skip holding the diff in memory at all. That also gets rid of the extra copy fromstring(b).