fix(transfer): stop paying one server round trip per file for the multi-stream decision - #757
Conversation
…ti-stream decision With the multi-stream download default of 4, S3 asked HEAD, Backblaze B2 asked a stat, and WebDAV and Koofr sent a Range 0-0 probe before every single download, to learn a size and compare it with the 250 MiB cutoff. On a batch that is one extra round trip per file, most of them for files nowhere near the cutoff: on a 47 ms link a 5000 x 4 KiB get -r on S3 went from 68 s to 127 s while rclone held at 60 s, and the delta per file times the parallelism recovers the link RTT in two independent cells. The batch executor, sync and a single get already hold the size from the listing or their own stat. They now pass it through download_with_size_hint, a trait method with a plain-download default, and the four providers skip the probe when the hint is below the cutoff; an unknown size still probes, so callers that do not know keep today's behaviour. FTP and SFTP read the size before their single-stream path anyway and were not affected. Tests: the shared rule, an HTTP fixture counting probe requests for the Range helper, and an S3 fixture counting HEADs (known small size: 0 HEAD; unknown size: 1). Both fixtures fail with the gates removed. Signed-off-by: axpnet <45786925+axpnet@users.noreply.github.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe download flow now accepts optional file-size hints. Callers pass known sizes to providers, which skip range-size probes for files below the configured multi-thread cutoff. S3, B2, Koofr, and WebDAV implement the optimization with regression tests. ChangesDownload size hint optimization
Priority: ⬇️ Low — Impact reflects low issue severity. Estimated code review effort: 3 (Moderate) | ~25 minutes Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to Downloads with known small file sizes avoid unnecessary provider round trips while preserving existing behavior for unknown sizes and other providers. No current merge-blocking risk is identified. Sequence Diagram(s)sequenceDiagram
participant DownloadCaller
participant StorageProvider
participant MultiThreadDownloader
DownloadCaller->>StorageProvider: download_with_size_hint(size_hint)
StorageProvider->>MultiThreadDownloader: evaluate size_hint against cutoff
MultiThreadDownloader-->>StorageProvider: skip probe or perform range probe
StorageProvider-->>DownloadCaller: download result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 9 files. (2 skipped: 1 unsupported, 1 too large.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ast-grep (0.45.2)src-tauri/src/bin/aeroftp_cli.rsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Summary
Follow-up to #735, found by the before/after battery on the DAG engine review (L1 test station, wired gigabit, 47 ms RTT to the lab): a 5000 x 4 KiB
get -r --parallel 4on S3 went from 68 s to 127 s on the after binary while rclone held at 60 s in the same window. Integrity checks passed on both sides, so it was a correct transfer that got slower. The delta per file multiplied by the parallelism gives 47.2 ms on that cell and 54.9 ms on a 166-file mixed cell, against a measured ICMP RTT of 47.4 ms: one extra round trip per file.Cause.
--multi-thread-streamsnow defaults to 4 (#735). Four providers decide whether to split a download by asking the server for the size first, gated only onstreams >= 2: S3 sends a signed HEAD, Backblaze B2 a stat, WebDAV and Koofr aRange: bytes=0-0probe GET. Before #735 the default was 1 stream, so the probe never ran; after it, every file paid it, including the thousands nowhere near the 250 MiB cutoff. FTP and SFTP read the size before their single-stream path anyway and were not affected.Fix, class not instance. The callers already know the size: the batch executor from the listing entry,
syncfrom its plan, a singlegetfrom its own stat. They pass it through a newStorageProvider::download_with_size_hint(remote, local, size_hint, progress)whose default is plaindownload, so the other 27 providers are untouched. The four probing providers implement it and skip the probe when the hint is below the cutoff (multi_thread::size_hint_rules_out_ranges, one rule for all four); an unknown size (None) still probes, so callers that do not know keep today's behaviour and a large file still gets its honest 206 check before the range split.The governor charge on the download loops (also new in #735) was checked and is not involved: with no cap set it reads one lock and returns, once per response chunk.
Tests
multi_thread::tests::a_size_hint_rules_out_ranges_only_when_known_and_below_the_cutoff: the shared rule.multi_thread::tests::a_known_small_size_returns_fallback_without_a_probe_request: counting HTTP fixture, known small size returnsFallbackwith the progress callback and 0 requests; unknown size probes once.s3::tests::a_known_size_below_the_cutoff_skips_the_multi_stream_head_probe: counting fixture, 4 KiB file with streams 4: known size, 0 HEAD and the file lands; unknown size, 1 HEAD.Both fixtures were run with the gates removed and failed there (0 expected, 1 observed).
Verified:
cargo fmt,cargo clippy --all-targets -- -D warnings(rc 0), the multi_thread, S3, executor and single-file DAG test groups (95 passed).Numbers to expect
A third bracket on the same cell is queued on the test station once this lands. Expected: the S3 5000-file download back at the before level (about 68 s on that link) with the multi-stream default kept.
Summary by CodeRabbit
Performance
File Metadata
Compatibility