executor: filter command output lines instead of buffering all of them - #365
Merged
Merged
Conversation
RunOpts gains KeepLine, a predicate deciding which stdout lines Run returns; a nil predicate keeps every line. Every line still reaches the log in full, so this bounds only what the caller retains. Previously, Local.Run and Remote.sshRun kept every stdout byte in a bytes.Buffer and handed the whole thing to splitOutputLines, which copied it again, so a command printing a large log cost roughly two copies of its output even where the result was discarded. Script now keeps only the setvar lines it parses, Echo only the lines it reports, and the sites that ignore the result keep nothing. Verbose is dropped from RunOpts, dead since 6c872a3: all three executors took it as _ and verbosity comes from MakeLogs. Note KeepLine makes RunOpts non-comparable, which nothing depends on today. Resolves umputun#363
Previously, maskSecrets compiled a regexp per secret on every call, and it is called for every line of every command, so on a noisy command with secrets configured it was the dominant cost of the output path. Patterns are now compiled once in MakeLogs and shared by every writer. On a typical log line with four secrets configured, masking drops from 6031 to 2190 ns/op.
A line spanning several writes is staged in a buffer that was truncated for reuse, keeping its backing array reachable until the command ended. One long line therefore stayed pinned for the rest of the run, which is the retention this filtering removes elsewhere. Buffers up to 64KiB are still reused to spare an allocation per line, larger ones are released.
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.
Previously,
Local.RunandRemote.sshRunstreamed stdout to the log and at the same time retained every byte in abytes.Buffer, then handed the whole buffer tosplitOutputLines, which copied it again. A command printing a large log therefore cost roughly two copies of its output, including at the ten call sites that discard the result.After this change,
RunOptscarriesKeepLine func(line string) bool, a predicate deciding which linesRunreturns; a nil predicate keeps every line. Every line still reaches the log in full, so this bounds only what the caller retains.Scriptpasses the samesetvartest its parse loop uses, so the two cannot drift;Echokeeps the non-empty lines it reports; the sites that ignore the result share adiscardOutputvalue.The capture is now a streaming writer in place of the buffer, so output is never fully retained. It reproduces the previous
splitOutputLines(buffer.String())exactly, which the tests pin across five write chunk sizes so that a line split across writes, a\r\npair split down the middle and a run of newlines all give the same answer as the batch split. A line of any length still arrives whole, and an unterminated final line is returned like a terminated one.Measured with the reproduction from #363, three runs each:
The issue's control run, with the same output sent to
/dev/null, sat at 21.5 to 22.4 MB.Verboseis dropped fromRunOpts, dead since 6c872a3: all three executors took it as_and verbosity comes fromMakeLogs. As noted in the issue,KeepLinemakesRunOptsnon-comparable, which nothing in the tree depends on.A line spanning several writes has to be staged, and the staging buffer is released once it grows past 64KiB rather than being kept for reuse. Holding it would pin the longest line for the rest of the command, which is the retention this change removes elsewhere.
The last commit is the
maskSecretspoint raised at the end of the issue: it compiled a regexp per secret on every call, and it is called for every line of every command. Patterns are now compiled once inMakeLogsand shared by every writer. On a typical log line with four secrets configured, masking drops from 6031 to 2190 ns/op. It sits on its own commit and can be split out if you would rather keep it separate.Resolves #363.