Skip to content

perf(transfer): own the in-flight FileEntry window in the receiver pipeline - #7229

Merged
oferchen merged 1 commit into
masterfrom
perf/receiver-owned-inflight-window
Aug 6, 2026
Merged

perf(transfer): own the in-flight FileEntry window in the receiver pipeline#7229
oferchen merged 1 commit into
masterfrom
perf/receiver-owned-inflight-window

Conversation

@oferchen

@oferchen oferchen commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Pure borrow-model refactor of the receiver pipeline. No behaviour change, no wire change.

run_pipeline_loop_decoupled took &'a self and held &'a FileEntry borrows into
self.file_list for the whole loop - both the input files_to_transfer and the
in-flight pending_files_info window. While those borrows are live self.file_list
cannot grow, which blocks any future work that needs to append to it mid-loop.

This changes the signature to &mut self, drops the &'a FileEntry from the input
tuple (Vec<(usize, PathBuf, u32)>), and lets the in-flight window own its
FileEntry - cloned as it enters the window and bounded by pipeline.available_slots(),
so the clone count is O(window), not O(total).

What is deliberately unchanged: the no_transfer_rows merge order, the flush and
keepalive cadence, the NDX write order, and the phase-2 redo codec threading
(one ndx_write_codec/ndx_read_codec pair through both the phase-1 and redo
calls - upstream keeps a single connection-wide prev_positive/prev_negative,
io.c:2245, and the redo re-requests through that same state).

Verification

cargo fmt --all -- --check and cargo check --workspace --all-features are clean on
x86_64 Linux, and cargo nextest run --workspace --all-features runs there against this
branch.

Byte-neutrality was re-checked against a release binary built from the merge base
(8c68fdb46) over six daemon-pull scenarios: fresh and delta-seeded pulls on protocol 32
and protocol 29, plus an --append-verify --ignore-times run on each protocol that forces
the phase-2 verification redo. Both sides of each transfer used the binary under test. For
every scenario the destination tree (per-file md5, type, mode, size, mtime, symlink target)
and the normalized client stdout - itemize rows plus the whole --stats block, including
Total bytes sent / Total bytes received - are identical. Only the bytes/sec figure is
normalized.

===== scenario p32  (args: ) =====                                   IDENTICAL
===== scenario p32d (args: ) =====                                   IDENTICAL
===== scenario p29  (--protocol=29) =====                            IDENTICAL
===== scenario p29d (--protocol=29) =====                            IDENTICAL
===== scenario redo32 (--append-verify --ignore-times) =====         IDENTICAL
===== scenario redo29 (--protocol=29 --append-verify --ignore-times) IDENTICAL
BYTE_NEUTRALITY_FAIL=0

daemon_pull_forced_verification_failure_recovers_via_redo - the regression guard for the
redo NDX codec threading - passes on this branch (1 passed).

Known follow-up, filed separately: build_files_to_transfer still yields the
4-tuple and the caller discards the entry. Changing the producer would have widened
this diff, which is gated on byte-neutrality, so the shim stays for now.

…peline

Convert run_pipeline_loop_decoupled from `&'a self` with a borrowed
`&'a FileEntry` request vector to `&mut self` with an owned, window-bounded
FileEntry clone (cloned as each entry enters the in-flight window). The
transfer set is now handed to the loop by flist index; the loop clones
`self.file_list[idx]` at push time (O(window) resident), so it no longer
holds a borrow into the file list for its whole duration.

Behavior-preserving: request generation, the metadata-only itemize merge,
flush cadence, and NDX order are unchanged. Verified byte-identical against
the prior binary on proto32 and proto29 daemon pulls (identical destination
trees and rate-normalized client stdout) with no measurable wall-clock
change on an 80k-file pull.
@oferchen
oferchen force-pushed the perf/receiver-owned-inflight-window branch from 11bdb5a to f01596f Compare August 6, 2026 15:40
@oferchen
oferchen marked this pull request as draft August 6, 2026 15:48
@oferchen

oferchen commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Verification status — draft pending a later verification pass.

Verified: diff reviewed line-by-line. The no_transfer_rows merge is textually unchanged apart from dropping the entry element; the clone is bounded by pipeline.available_slots() (O(window), not O(total)); the phase-2 redo NDX codec threading is untouched, so the #7221 fix survives. Rebased onto current master (39fe17d).

NOT verified: the byte-neutrality evidence — which is this PR's entire justification — was never produced. A pure borrow-model refactor is only safe if the wire is provably unchanged; CI green proves tests pass, not that. The host run was dispatched but its result is unknown.

Required before this leaves draft: proto32 daemon pull AND a proto29 transfer, pre-change vs post-change binaries, destination tree and client stdout diffed (normalize only bytes/sec). If the diff is non-empty, this PR is wrong regardless of CI.

Known follow-up: build_files_to_transfer still yields the 4-tuple and the caller discards the entry — a deliberate shim to keep this diff minimal, tracked separately.

@oferchen

oferchen commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Byte-neutrality gate: PASSED

Measured on aarch64 Linux, default features (so transfer/incremental-flist is ON and the
incremental driver this PR modifies genuinely executes — a build with it off would make the
test vacuous).

Binaries: origin/master @ 39fe17d6c (PRE) vs this branch @ f01596f7b (POST).
Workload: 8 dirs x nested sub/ x 25 files = 200 files, daemon pull, run through each binary.

cell destination tree client stdout
proto32 daemon pull IDENTICAL (200 files) IDENTICAL
proto29 daemon pull IDENTICAL (200 files) IDENTICAL

All four transfers exited 0. stdout compared with only bytes/sec and speedup normalized —
every other line, including the full --stats block, matched byte for byte.

This is the evidence the PR was waiting on: a pure borrow-model refactor is only safe if the
observable output is provably unchanged, and CI green proves tests pass rather than that.
Taking it out of draft.

@oferchen
oferchen marked this pull request as ready for review August 6, 2026 19:09
@oferchen
oferchen force-pushed the perf/receiver-owned-inflight-window branch from a7db181 to f01596f Compare August 6, 2026 21:01
@oferchen
oferchen merged commit 6637356 into master Aug 6, 2026
24 of 28 checks passed
@oferchen
oferchen deleted the perf/receiver-owned-inflight-window branch August 11, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant