Analysis: optimize peer IP list from swarm (issue #1505) — implementation rejected#1949
Conversation
…ementation rejected
…han old path) The parallel compact peer path introduced in 813f785 was benchmarked as ~2x slower than the existing Arc<Peer> path. - peers_excluding (old): 407 ns for 74 peers - peers_excluding_compact (new): 824 ns for 74 peers The root cause: Arc::clone is just an atomic refcount increment + 8-byte pointer copy, while the compact path copies 52 bytes per peer (PeerId + SocketAddr). The spec documents (ISSUE.md, pre-implementation-analysis.md), baseline performance, and post-performance report remain as a permanent record.
There was a problem hiding this comment.
Pull request overview
This PR documents (and preserves) the investigation into issue #1505 (“optimize peer IP list from swarm”) after concluding the proposed CompactPeer-based optimization is a performance regression, and updates benchmarking docs/assets accordingly.
Changes:
- Adds a full issue-spec package (pre-analysis, baseline + post benchmark reports, and an Aquatic benchmarking guide) capturing why the optimization was rejected.
- Updates
docs/benchmarking.mdto include Aquatic-based workflows and a peer-retrieval microbenchmark entrypoint. - Extends the spell-check word list and adds clarifying documentation on the tracker-client’s IPv4-only compact peer struct.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
project-words.txt |
Adds benchmark/networking-related terms used in the new docs. |
packages/tracker-client/src/http/client/responses/announce.rs |
Adds doc comment clarifying the IPv4-only compact peer parsing. |
packages/swarm-coordination-registry/examples/bench_peers.rs |
Adds a peer-retrieval microbenchmark example (currently does not compile due to referencing reverted APIs). |
docs/issues/open/1505-optimize-peer-ip-list-from-swarm/ISSUE.md |
Task spec capturing scope, plan, acceptance criteria, and the “rejected due to regression” outcome. |
docs/issues/open/1505-optimize-peer-ip-list-from-swarm/pre-implementation-analysis.md |
Research report documenting design reasoning and call-chain analysis. |
docs/issues/open/1505-optimize-peer-ip-list-from-swarm/baseline-performance.md |
Baseline benchmark report for before/after comparison. |
docs/issues/open/1505-optimize-peer-ip-list-from-swarm/post-performance.md |
Post-implementation benchmark report showing the regression and verdict. |
docs/issues/open/1505-optimize-peer-ip-list-from-swarm/aquatic-benchmarking-guide.md |
Detailed Aquatic setup/run/troubleshooting guide (has a small typo). |
docs/benchmarking.md |
Updates canonical benchmarking doc to reference Aquatic workflows and the new microbenchmark (contains a broken relative link). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use torrust_clock::DurationSinceUnixEpoch; | ||
| use torrust_tracker_primitives::peer::Peer; | ||
| use torrust_tracker_primitives::{AnnounceEvent, CompactPeer, NumberOfBytes, PeerId}; | ||
| use torrust_tracker_swarm_coordination_registry::event::sender::Sender; | ||
| use torrust_tracker_swarm_coordination_registry::swarm::coordinator::Coordinator; |
There was a problem hiding this comment.
Resolved in commit 3e9c9ba. The bench_peers.rs file was restored to pre-implementation state, removing all CompactPeer references and peers_excluding_compact calls.
| /// Tracker client compact peer entry (IPv4 only). | ||
| /// | ||
| /// issue-link: #1505 — the server-side `CompactPeer` in `torrust-tracker-primitives` | ||
| /// will support both IPv4 and IPv6. If the client needs to parse IPv6 compact peer | ||
| /// lists (the `peers6` key from BEP 7), this struct would need to be extended or | ||
| /// replaced alongside a follow-up. |
There was a problem hiding this comment.
Acknowledged. The doc comment in tracker-client still references a server-side CompactPeer that no longer exists because the implementation was reverted. This should be updated to describe only the IPv4 limitation without implying an existing server-side type. Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. The doc comment was rewritten to describe only the IPv4-only limitation without referencing the deleted server-side CompactPeer type.
| > For a detailed step-by-step guide with full command output and troubleshooting, see the | ||
| > [Aquatic Benchmarking Guide](docs/issues/open/1505-optimize-peer-ip-list-from-swarm/aquatic-benchmarking-guide.md) | ||
| > (created during issue #1505). |
There was a problem hiding this comment.
Acknowledged. The relative link from docs/benchmarking.md to docs/issues/... should be fixed. Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. The relative links were changed from 'docs/issues/...' to 'issues/...' (removing the duplicate 'docs/docs' path).
| sockets_per_worker = 4 | ||
| # Size of socket recv buffer. Use 0 for OS default. | ||
| # | ||
| # This setting can have a big impact on dropped packages. It might |
There was a problem hiding this comment.
Acknowledged. Typo fix: 'dropped packages' -> 'dropped packets'. Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. Changed 'dropped packages' to 'dropped packets'.
| > For a detailed step-by-step guide with full command output and troubleshooting, see the | ||
| > [Aquatic Benchmarking Guide](docs/issues/open/1505-optimize-peer-ip-list-from-swarm/aquatic-benchmarking-guide.md) | ||
| > (created during issue #1505). |
There was a problem hiding this comment.
Acknowledged. The relative link from docs/benchmarking.md to docs/issues/... should be fixed. Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. The relative links were changed from 'docs/issues/...' to 'issues/...' (removing the duplicate 'docs/docs' path).
| @@ -0,0 +1,124 @@ | |||
| //! Microbenchmark: `Coordinator::peers_excluding` throughput. | |||
| //! Usage: cargo run --package torrust-tracker-swarm-coordination-registry --example `bench_peers` --release | |||
There was a problem hiding this comment.
Outdated — resolved by commit 3e9c9ba. The bench_peers.rs file was restored to its pre-implementation state (before the compact-path feature), so these comments no longer apply.
There was a problem hiding this comment.
Fixed in commit 3e9c9ba. The bench_peers.rs file was restored to pre-implementation state which does not have this usage string pattern.
| use torrust_clock::DurationSinceUnixEpoch; | ||
| use torrust_tracker_primitives::peer::Peer; | ||
| use torrust_tracker_primitives::{AnnounceEvent, CompactPeer, NumberOfBytes, PeerId}; | ||
| use torrust_tracker_swarm_coordination_registry::event::sender::Sender; |
There was a problem hiding this comment.
Outdated — resolved by commit 3e9c9ba. The bench_peers.rs file was restored to its pre-implementation state (before the compact-path feature), so these comments no longer apply.
There was a problem hiding this comment.
Fixed in commit 3e9c9ba. The bench_peers.rs file was restored to pre-implementation state — no CompactPeer import remains.
| for _ in 0..1000 { | ||
| black_box(coordinator.peers_excluding_compact(&requesting_addr, Some(limit))); | ||
| } | ||
|
|
||
| let start = Instant::now(); | ||
| for _ in 0..iterations { | ||
| black_box(coordinator.peers_excluding_compact(&requesting_addr, Some(limit))); | ||
| } |
There was a problem hiding this comment.
Outdated — resolved by commit 3e9c9ba. The bench_peers.rs file was restored to its pre-implementation state (before the compact-path feature), so these comments no longer apply.
There was a problem hiding this comment.
Fixed in commit 3e9c9ba. The file was restored — no peers_excluding_compact calls remain.
| for i in 0..num_peers { | ||
| let peer = make_peer((i % 254) as u8 + 1, 6881 + (i % 10000) as u16, (i % 255) as u8); | ||
| let rt = tokio::runtime::Runtime::new().unwrap(); | ||
| rt.block_on(coordinator.handle_announcement(&peer)); | ||
| } |
There was a problem hiding this comment.
Outdated — resolved by commit 3e9c9ba. The bench_peers.rs file was restored to its pre-implementation state (before the compact-path feature), so these comments no longer apply.
There was a problem hiding this comment.
This was on a now-reverted version. The current version (cd92fbe) reuses a single Tokio runtime for setup.
| for i in 0..num_peers { | ||
| let peer = make_peer((i % 254) as u8 + 1, 6881 + (i % 10000) as u16, (i % 255) as u8); | ||
| let rt = tokio::runtime::Runtime::new().unwrap(); | ||
| rt.block_on(coordinator.handle_announcement(&peer)); | ||
| } |
There was a problem hiding this comment.
Outdated — resolved by commit 3e9c9ba. The bench_peers.rs file was restored to its pre-implementation state (before the compact-path feature), so these comments no longer apply.
There was a problem hiding this comment.
This was on the compact-path version which has been reverted. The current pre-implementation version only has the non-compact benchmark.
| - [x] AC1: `CompactPeer` struct exists with `From` conversions | ||
| - [x] AC2: Compact methods on Coordinator, Registry, InMemoryTorrentRepository | ||
| - [x] AC3: Compact response data type exists | ||
| - [x] AC4: UDP and HTTP response builders work correctly |
There was a problem hiding this comment.
Acknowledged. AC5 is correctly marked as 'REJECTED (not met)' in the current version of ISSUE.md because the implementation was reverted. The spec documents why.
There was a problem hiding this comment.
Fixed in commit 1369767. AC5 is marked as 'REJECTED (not met)' in the current ISSUE.md with a clear explanation why.
… (remove compact references)
josecelano
changed the title
| bottlenecked | ||
| bools |
There was a problem hiding this comment.
Acknowledged. 'bottlenecked' is out of alphabetical order in project-words.txt. Will fix sorting in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. 'bottlenecked' was moved to the correct case-insensitive alphabetical position (after 'bools').
| > For a detailed step-by-step guide with full command output and troubleshooting, see the | ||
| > [Aquatic Benchmarking Guide](docs/issues/open/1505-optimize-peer-ip-list-from-swarm/aquatic-benchmarking-guide.md) | ||
| > (created during issue #1505). |
There was a problem hiding this comment.
Acknowledged. The relative link from docs/benchmarking.md to docs/issues/... should be fixed. Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. The relative links were changed from 'docs/issues/...' to 'issues/...' (removing the duplicate 'docs/docs' path).
| /// Tracker client compact peer entry (IPv4 only). | ||
| /// | ||
| /// issue-link: #1505 — the server-side `CompactPeer` in `torrust-tracker-primitives` | ||
| /// will support both IPv4 and IPv6. If the client needs to parse IPv6 compact peer | ||
| /// lists (the `peers6` key from BEP 7), this struct would need to be extended or | ||
| /// replaced alongside a follow-up. |
There was a problem hiding this comment.
Acknowledged. The doc comment in tracker-client still references a server-side CompactPeer that no longer exists because the implementation was reverted. This should be updated to describe only the IPv4 limitation without implying an existing server-side type. Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. The doc comment was rewritten to describe only the IPv4-only limitation without referencing the deleted server-side CompactPeer type.
| // Populate swarm | ||
| for i in 0..num_peers { | ||
| let peer = make_peer((i % 254) as u8 + 1, 6881 + (i % 10000) as u16, (i % 255) as u8); | ||
| let rt = tokio::runtime::Runtime::new().unwrap(); | ||
| rt.block_on(coordinator.handle_announcement(&peer)); | ||
| } |
There was a problem hiding this comment.
Acknowledged. The file was restored to pre-implementation state which has this pattern. Will fix in a follow-up to reuse a single Tokio runtime for setup.
There was a problem hiding this comment.
Fixed in commit cd92fbe. The runtime is now created once outside the loop and reused for all peers.
| } | ||
| } | ||
|
|
||
| #[allow(clippy::cast_possible_truncation, clippy::cast_precision_loss, clippy::cast_sign_loss)] |
There was a problem hiding this comment.
Acknowledged. The #[allow(...)] suppression on bench_peers_excluding lacks a justification comment. Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. Added a justification comment above the #[allow(...)] explaining why each cast is safe.
| ### 2.2 Generate default load test config | ||
|
|
||
| ```bash | ||
| cd /home/josecelano/Documents/git/committer/me/github/greatest-ape/aquatic |
There was a problem hiding this comment.
Acknowledged. The absolute path should be replaced with a placeholder (e.g. /path/to/...). Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. All absolute paths were replaced with placeholders like /path/to/aquatic or /path/to/torrust-tracker.
| ### 2.3 Start the Torrust Tracker with benchmarking config | ||
|
|
||
| ```bash | ||
| cd /home/josecelano/Documents/git/committer/me/github/torrust/torrust-tracker-agent-02 |
There was a problem hiding this comment.
Acknowledged. The absolute path should be replaced with a placeholder (e.g. /path/to/...). Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. All absolute paths were replaced with placeholders like /path/to/aquatic or /path/to/torrust-tracker.
| ### 2.4 Run the UDP load test | ||
|
|
||
| ```bash | ||
| cd /home/josecelano/Documents/git/committer/me/github/greatest-ape/aquatic |
There was a problem hiding this comment.
Acknowledged. The absolute path should be replaced with a placeholder (e.g. /path/to/...). Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. All absolute paths were replaced with placeholders like /path/to/aquatic or /path/to/torrust-tracker.
| Then run: | ||
|
|
||
| ```bash | ||
| cd /home/josecelano/Documents/git/committer/me/github/greatest-ape/aquatic |
There was a problem hiding this comment.
Acknowledged. The absolute path should be replaced with a placeholder (e.g. /path/to/...). Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. All absolute paths were replaced with placeholders like /path/to/aquatic or /path/to/torrust-tracker.
|
|
||
| ### Aquatic bencher | ||
|
|
||
| The aquatic repository is at `/home/josecelano/Documents/git/committer/me/github/greatest-ape/aquatic/`. |
There was a problem hiding this comment.
Acknowledged. The absolute path should be replaced with a placeholder. Will address in a follow-up.
There was a problem hiding this comment.
Fixed in commit cd92fbe. All absolute paths were replaced with placeholders like /path/to/aquatic or /path/to/torrust-tracker.
- Fix tracker-client doc comment: remove reference to deleted server-side CompactPeer - Fix docs/benchmarking.md relative links (remove 'docs/docs' duplication) - Fix aquatic-benchmarking-guide.md: typo 'packages' -> 'packets', replace absolute paths with placeholders - Fix pre-implementation-analysis.md: replace absolute path with clone URL - Fix project-words.txt: sort 'bottlenecked' in correct position - Fix bench_peers.rs: add clippy allow justification comment, reuse single Tokio runtime for setup
|
ACK cd92fbe |
…ssues/open to docs/issues/closed 174904d chore(ci): point to ADR instead of closed issue spec in deployment-packages.yaml (Jose Celano) 2e2178f docs(skill): document branch-already-exists edge case in cleanup-completed-issues skill (Jose Celano) 1bfd306 chore(issues): archive closed issue specs #1447, #1459, #1505, #1926, #1938, #1944, #1959, #1964, #1965, #1969 to docs/issues/closed (Jose Celano) Pull request description: Archives the following 10 issue specs that are closed on GitHub, moving them from `docs/issues/open/` to `docs/issues/closed/`: | Issue | Title | PR | |-------|-------|-----| | #1447 | Change logging threshold for connection ID error to WARNING | #1975 | | #1459 | Docker Security Overhaul | #1958 | | #1505 | Optimize peer IP list from swarm (implementation rejected) | #1949 | | #1926 | Define package versioning strategy | #1961 | | #1938 | REST API Contract-First Migration EPIC | #1960 (SI-1→5) | | #1944 | SI-6: Align REST API client | #1968 | | #1959 | SI-7: Review tests, align v1 namespace | #1963 | | #1964 | Rename number-of-downloads BTreeMap type alias | #1972 | | #1965 | SI-34: Consolidate duplicate HTTP types | #1974 | | #1969 | SI-8: Eliminate unwraps from REST API client | #1973 | ### Changes Made - All 10 specs verified `CLOSED` on GitHub with merged PRs - Files/directories moved via `git mv` to `docs/issues/closed/` - Frontmatter updated: `status: done`, `spec-path` → closed path, `last-updated-utc: 2026-07-15` - Workflow checkpoints updated and progress log entries added - Supplementary files in multi-file directories updated with new paths - Parent EPIC specs (#1669, #1938) updated: subissue status, spec paths, and table references - Fixed stale references in `.github/workflows/deployment-packages.yaml`, `docs/adrs/`, and `docs/issues/closed/` files - Pre-commit and pre-push hooks passed (linter, tests, doc-tests, nightly checks) ACKs for top commit: josecelano: ACK 174904d Tree-SHA512: c7e38354bb5898fc5125a0b68fd61a02ca273689593638eb3b955b9176038ef82c40fc98b0a2f722eb18d8d429d2f2dbb6f205eee3061211ab9906c5be46942b
Description
This branch analyzes the feasibility of optimizing the announce call chain by introducing a lightweight
CompactPeertype. The implementation was built alongside the existing path (parallel compact path strategy) and benchmarked but ultimately rejected because it was ~2× slower (~407 ns → ~824 ns for 74 peers).Commit History
f940543f— Spec documents (ISSUE.md, analysis, benchmark templates)7b78a39c— Baseline performance results + benchmarking guide813f7851— Implementation (parallel compact path)13697675— Post-implementation performance report1544273b— Revert of the implementation codeOutcome
Implementation rejected. The compact peer path was benchmarked at ~2× slower than the existing
Arc<Peer>path becauseArc::clone(8 bytes, atomic) is cheaper than copying a 52-byteCompactPeerfrom eachArcdereference. The optimization would need to storeCompactPeerdirectly in the swarm'sBTreeMapto avoid the conversion cost — but that was out of scope (would change swarm internal storage).What's preserved
examples/bench_peers.rs) with old vs compact comparisonThis record prevents future re-litigation of the same approach.
Related
docs/issues/open/1505-optimize-peer-ip-list-from-swarm/