Skip to content

perf(daemon): drop the per-log-line String reallocation in the log task - #3102

Open
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-tkzcup-log-line-alloc
Open

perf(daemon): drop the per-log-line String reallocation in the log task#3102
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-tkzcup-log-line-alloc

Conversation

@phil-opp

@phil-opp phil-opp commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Issue

The daemon's per-node file-logging task ran this for every log line of every spawned node (binaries/daemon/src/spawn/prepared.rs):

let formatted = content.lines().fold(String::default(), |mut output, line| {
    output.push_str(line);
    output
});

content is an owned String moved out of the LogLine and not used afterward. It holds a single logical line — read_capped_line stops at the first \n — so this fold does nothing but strip the trailing newline, yet it heap-allocates a brand-new String and copies the entire line byte-for-byte. On a node logging at high frequency that's an allocation + full copy per line, on the log hot path, for no benefit.

Fix

Introduce a small strip_trailing_newline(String) -> String helper that pops a trailing \n (and a preceding \r) from the already-owned buffer in place, and use it instead of the fold. No allocation, no copy.

For the single-line inputs the reader produces, the result is byte-for-byte identical to content.lines().next().

Validation

  • Added strip_trailing_newline_matches_lines_for_single_line, which diffs the helper against the old .lines().fold(...) implementation across \n, \r\n, bare \r, empty, mid-line \r, trailing-spaces, and truncated-marker cases.
  • cargo +1.97.1 fmt -p dora-daemon -- --check — clean.
  • cargo +1.97.1 clippy -p dora-daemon -- -D warnings — clean.
  • cargo +1.97.1 test -p dora-daemon strip_trailing — passes.

⚠️ This is a machine-generated pull request authored by Claude (Claude Code). A human should review before merging.

🤖 Generated with Claude Code


Generated by Claude Code

For every log line of every spawned node, the file-logging task rebuilt
the line with `content.lines().fold(String::new(), ..)`, heap-allocating
a fresh String and copying the whole line byte-for-byte. `content` is
already an owned single logical line (the reader stops at the first
`\n`), so the fold was only stripping the trailing line terminator.

Replace it with an in-place `strip_trailing_newline` helper that pops a
trailing `\n`/`\r\n` from the owned buffer, avoiding the allocation and
copy on the log hot path. Behavior is byte-for-byte identical for the
single-line inputs the reader produces, guarded by a unit test that
diffs the helper against the old fold over newline/CRLF/empty/truncated
cases.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1Fmp8pELuTiPGTStomkZj
@trunk-io

trunk-io Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude Code — fully automated review, not vetted by a human.

No issues found.

I verified the optimization against the code. read_capped_line reads with read_until(b'\n', raw) (binaries/daemon/src/spawn/prepared.rs), so content is a single logical line holding at most one trailing \n (and possibly a preceding \r), with no embedded newlines. Under that invariant the old content.lines().fold(...) and the new strip_trailing_newline are byte-for-byte identical:

  • The fold only concatenated lines and dropped terminators; with a single line it merely stripped the trailing \n/\r\n.
  • A lone \r (truncated line) and no-newline input are preserved by both.
  • Empty, "\n", and "\r\n" all collapse to "" in both.

content is moved out of LogLine at the use site and not read afterward, so consuming it in place is safe. The differential unit test (strip_trailing_newline_matches_lines_for_single_line) checks the helper against the old fold across representative cases, which is a meaningful guard rather than a tautology.


Generated by Claude Code

@phil-opp
phil-opp marked this pull request as ready for review August 11, 2026 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants