Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts how libtorrent computes per-block request lengths for v2-only torrents so peers don’t request bytes that fall into pad-space at file boundaries.
Changes:
- Add
torrent_info::piece_size_for_req()to select the appropriate piece size for request-length calculations (v1:piece_size(), v2-only:piece_size2()). - Update
torrent::to_req()andpeer_connection’s outgoing request sizing to usepiece_size_for_req(). - Add a ChangeLog entry describing the v2-only request size optimization.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/torrent.cpp | Use piece_size_for_req() when converting piece_block → peer_request. |
| src/peer_connection.cpp | Use piece_size_for_req() when computing outgoing request block sizes. |
| include/libtorrent/torrent_info.hpp | Introduce piece_size_for_req() helper API. |
| ChangeLog | Document the behavior change for v2-only request sizing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
arvidn
force-pushed
the
v2-to-req
branch
2 times, most recently
from
9e59ca5 to
7163b88
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -397,6 +391,12 @@ void web_peer_connection::write_request(peer_request const& r) | |||
| std::cerr << this << " REQ: p: " << pr.piece << " " << pr.start << std::endl; | |||
| #endif | |||
| size -= pr.length; | |||
| cur_start += pr.length; | |||
| if (cur_start >= info.piece_size_for_req(cur_piece)) | |||
| { | |||
| cur_start = 0; | |||
| ++cur_piece; | |||
| } | |||
There was a problem hiding this comment.
In the request-splitting loop, piece_size_for_req() is called multiple times per iteration (to compute pr.length and to check for piece boundary). For v2 torrents this ends up calling file_storage::piece_size2(), which does an upper_bound() over the file list (log N) each time. Consider caching the current piece's piece_size_for_req in a local variable and only recomputing it when cur_piece changes, to avoid extra work when pipelining many blocks.
| // returns the piece size appropriate for computing request lengths. | ||
| // for v2 torrents, pieces at the end of files may be shorter than | ||
| // the main piece size. This is the case for hybrid torrents as well. | ||
| int piece_size_for_req(piece_index_t index) const | ||
| { | ||
| return v2() | ||
| ? m_files.piece_size2(index) | ||
| : m_files.piece_size(index); | ||
| } |
There was a problem hiding this comment.
This PR introduces torrent_info::piece_size_for_req() as a new public helper with v2-specific semantics. There are already unit tests for file_storage::piece_size2(), but I couldn't find any tests that exercise piece_size_for_req() directly (including hybrid torrents) to ensure it keeps request sizing correct at file boundaries. Please add coverage to lock in the intended behavior (v2()/hybrid -> piece_size2, v1-only -> piece_size).
| peer_request torrent::to_req(piece_block const& p) const | ||
| { | ||
| int const block_offset = p.block_index * block_size(); | ||
| int const block = std::min(torrent_file().piece_size( | ||
| p.piece_index) - block_offset, block_size()); | ||
| int const piece_sz = torrent_file().piece_size_for_req(p.piece_index); | ||
| int const block = std::min(piece_sz - block_offset, block_size()); | ||
| TORRENT_ASSERT(block > 0); |
There was a problem hiding this comment.
torrent::to_req() now uses piece_size_for_req() to size the last block at v2 file boundaries. There don't appear to be any tests that assert the generated peer_request lengths for v2-only/hybrid torrents (i.e. that requests never extend into pad-space). Adding a focused unit/integration test for this would help prevent regressions since this affects core wire-protocol behavior.
No description provided.