Repository navigation
perf(gitread): join a git call's output once - #750
Conversation
Run kept the output of a budgeted git call in a bytes.Buffer and returned Buffer.String(), which copies it. The buffer doubles by copying as well, so one call peaked at 3 to 4 times its output (3.2 times on a 31 MB listing). limitedBuffer now keeps the output in chunks that never move, each a quarter of what is held (64 bytes to 8 MiB), and String joins them once into a string of exactly the output's size. The peak is the chunks plus the string, 2 times the output. Run still returns a string and still counts the output once against the allowance. A plain strings.Builder was measured and is no better: it grows by 1.25 times, and the copies it leaves raise the peak to 3 to 4 times. Change-Id: I5c81caba0567af251ad51b7fd1e20a582717abc9 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Change-Id: I6d63e76001856af8ac4e153749e6ac309810944d Signed-off-by: Thomas Kosiewski <tk@coder.com>
A chunk for every write, bytes held that are not counted, and a count taken from the whole write were caught only by a test that ran until its timeout (CI would hold the job for 30 minutes and print a stack) or not at all. The write loops of TestLimitedBufferKeepsWhatIsWritten and TestLimitedBufferChunksStayWithinAQuarter now check after every write that the buffer holds what was written (l.n) in at most 80 chunks. The bound follows from the growth: 4 chunks of 64 bytes, then 1.25 times per chunk from 256 bytes to 32 MiB, then chunks of 8 MiB; the tests hold 31 to 59. Each mutant now fails within about a second of test time with a message. Change-Id: Ic2da6a859f47d5d23e6f0d92ca7c36229d66433b Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
A chunk that ignores the size of its write split the first 32 KiB write into many small chunks. 1 MiB then cost 50 allocations instead of 20, and no test failed. A smallest chunk of 1 byte cost a 41-byte output written a byte at a time 22 allocations instead of 3, and no test failed. TestLimitedBufferAllocationsAtSize bounds the allocations of 1 MiB and 16 MiB written in 32 KiB pieces (limits 30 and 45, the code makes 20 and 33). TestLimitedBufferSmallOutputAllocations also writes the small output a byte at a time. The maxChunks comment names only runs that a test does. Change-Id: I34f90bb95fdb342bdebeefb2419da14cfdfc901a Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
ThomasK33 asked to fix the open coder/openstorage issues and land them on main. The review of #739 measured that every budgeted git call peaks at 3 to 5 times its output: on a real
serve, a listing of 31 MB raisedVmHWM(peak resident memory) by 100 MB or more (see Verification). This PR removes the copy that causes it. It is not a change of #739.gitread.Runkept git's output in abytes.Bufferand returnedBuffer.String(), which copies it. The buffer also grows by doubling, which copies it again and leaves the old arrays as garbage. The peak was the output 3 to 5 times.What changes
limitedBuffer(internal/gitread/budget.go) keeps the output in chunks that never move. A chunk is a quarter of what is held (64 bytes to 8 MiB, or the size of the write when that is larger), so the unused part of the last chunk stays within a quarter of the output.Stringjoins the chunks once, into a string of exactly the output's size. The peak is the chunks plus the string, 2 times the output.Runstill returns a string, still counts the output once against the allowance, and still returns the partial output with an error. A small output (a git call that prints 41 bytes) takes one more allocation of 24 bytes and 235 to 315 ns per call inBenchmarkLimitedBuffer, where the old buffer took 80 to 262 ns (8 alternating runs, load average 36 to 43, so the ranges overlap). A git process start costs 8 to 18 ms (two measurements ofgit rev-parse --git-dir).Runis called byrunGit(gitread.go),CommitsBySHA(log.go),ReadBlob(tree.go),wcScratch.git(internal/api/handlers_write_commit.go) andmergeBaseSHA(internal/api/handlers_read_query.go). Each reads the result as a string. The buffer is local toRun, andos/execwaits for its copy goroutines beforeWaitreturns, also afterWaitDelay, so nothing writes to it onceRunreads it. No non-test code outsidevendorimportsunsafe. I changed no caller.ReadBlob(its one caller reads symlink targets inarchive.go) converts the string to bytes, which is one more copy. A target can be as large as the output allowance, so a cap on its length is a separate change.strings.Builder(no copy inString) does not help: it grows by 1.25 times, and the copies it leaves raise the peak (table below). Chunks that never move do.internal/gitread/limitedbuffer_test.go): the chunks join into what was written for odd write sizes, and after every write the buffer holds what was written in at most 80 chunks (the bound follows from the growth: 4 chunks of 64 bytes, then 1.25 times per chunk from 256 bytes to 32 MiB, so 61 is the most at the 64 MiB allowance, and the tests reach 31 to 56), an overflow is refused and kills git, the allocation of 16 MiB of output is at most 2.5 times its size (3 times or more before), 1 MiB and 16 MiB written in 32 KiB pieces cost at most 30 and 45 allocations (the code makes 20 and 33), a small output costs at most 3 allocations, also when it comes a byte at a time, the unused part of the last chunk stays within a quarter and under 8 MiB, and a budgetedRunof a 16 MiB blob allocates at most 2.5 times its size and counts it once.BenchmarkLimitedBuffershows the copies as bytes and allocations per call.Verification
Peak resident memory of one budgeted
Runofgit cat-file blobon a blob of listing-like text. A child process runs it once and readsVmHWMafter resetting the peak (clear_refs). The table is the peak above the baseline of the process: the median of 7 runs, and in brackets the lowest to the highest run. The host was shared (load average 62 to 72), so read the ratios. The "before" runs move between runs (garbage collection timing), so read them as a range. "Before" is this branch's parent,da289aa2. A reviewer repeated the three sizes on this head (median of 7) and got 3.57, 4.28 and 3.26 times before and 2.06, 2.04 and 2.03 times after.Two other buffers, same harness, median of 5 runs, for 20, 31 and 45 MB: a plain
strings.Buildergave 3.16, 4.03 and 3.74 times (and 5.2 to 5.7 times the output in allocations). Abytes.Bufferreturned withunsafe.Stringgave 3.42, 2.21 and 3.02 times. A reviewer measured a realserveon the #739 listing (a directory of 500,001 entries, 31.1 MB), with a freshservefor each of 4 samples per build. The first request raisedVmHWMby 132.6 to 150.1 MB (4.3 to 4.8 times) at this branch's base,c9c8ae7e, and by 63.2 to 63.5 MB (2.04 times) with this change. An earlier measurement of 2 samples read 100.7 and 100.8 MB before (3.24 times) and 63.8 and 64.4 MB after (2.05 to 2.07 times).BenchmarkLimitedBuffer(writes of 32 KiB, then a join). 8 alternating runs of the old and the new buffer, with 400,000 iterations for 41 bytes, 300 for 1 MiB and 30 for 16 MiB, load average 36 to 43. The time is the lowest to the highest run, so the ranges overlap on this host. The bytes and allocations do not vary at the precision shown, except the old buffer's 11 to 12 allocations at 16 MiB.Round 1: Codex code and security reviews of head 98ed394 were clean. Round 2 (tests that fail fast): the reviews of head 6af97ca were clean. Round 3 (this head): a verifier found that the one mutant I had called equivalent changes the allocation counts, and that some numbers here did not reproduce. This head pins that count with a test, and the numbers above are repeated measurements.
Checks on the head:
go build ./...,go vet -tags=openstorage_viewcheckoninternal/gitread,internal/apiandcmd/...: clean.golangci-lint2.15.0 on./internal/gitread/...and./internal/api/...: 0 issues.go test -tags=openstorage_viewcheck -count=1 -race ./internal/gitread: pass on this head (122.2 s)../internal/apiand./cmd/openstoragepassed the same command on the previous head,6af97ca0, with main merged in (346.4 s and 44.9 s). This head changes onlyinternal/gitread/limitedbuffer_test.go, which neither package compiles, so I did not run them again.python3 scripts/check-doc-links.py: 0 broken links.limitedBuffer18 times, one change at a time, and ran the new tests. All 18 turned a test red. The red ones: the oldbytes.BufferwithString()(3.00 times its output), astrings.Builder(5.2 times), noGrowbefore the join, aGrowof half the output, chunks as large as what is held, no bound on a chunk, a bound of 1 KiB, an eighth of what is held for a chunk, chunks joined in reverse, a write that reports one byte less, a refused write that does not set the overflow flag, a first chunk of 1 MiB, the output counted twice, the bytes held not counted, the count taken from the whole write, a new chunk for every write, a chunk that ignores the size of the write (the write splits into small chunks, and 1 MiB costs 50 allocations, not 20), and a smallest chunk of 1 byte (a 41-byte output written a byte at a time costs 22 allocations, not 3). The first version of the tests caught a chunk for every write and the bytes not counted only by their timeout, and the count from the whole write not at all. Now the write loops check after every write that the buffer holds what was written in at most 80 chunks, and a verifier's five mutants (a chunk for every write, bytes not counted, a count from the whole write, an eighth, a bound of 1 KiB) fail with a message within 1.3 s of test time (6.5 to 8.8 s with the compile).Refs #739
Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh