diff --git a/libtransmission/peer-mgr-wishlist.cc b/libtransmission/peer-mgr-wishlist.cc index 0a1e38019..a6cf3f454 100644 --- a/libtransmission/peer-mgr-wishlist.cc +++ b/libtransmission/peer-mgr-wishlist.cc @@ -9,7 +9,7 @@ #include #include -#include +#include #include #define LIBTRANSMISSION_PEER_MODULE @@ -21,11 +21,6 @@ #include "libtransmission/tr-macros.h" #include "libtransmission/peer-mgr-wishlist.h" -// Asserts in this file are expensive, so hide them in #ifdef -#ifdef TR_WISHLIST_ASSERT -#include "libtransmission/tr-assert.h" -#endif - namespace { [[nodiscard]] std::vector make_spans(small::vector const& blocks) @@ -44,12 +39,8 @@ namespace return lhs + 1U != rhs; }; - auto span_end = std::adjacent_find(span_begin, end, NotAdjacent); - if (span_end == end) - { - --span_end; - } - spans.push_back({ *span_begin, *span_end + 1 }); + auto const span_end = std::min(std::adjacent_find(span_begin, end, NotAdjacent), std::prev(end)); + spans.push_back({ *span_begin, *span_end + 1U }); span_begin = std::next(span_end); } @@ -68,14 +59,13 @@ class Wishlist::Impl , replication{ mediator->count_piece_replication(piece_in) } , priority{ mediator->priority(piece_in) } , salt{ salt_in } - , mediator_{ mediator } { - n_reqs.reserve(block_span.end - block_span.begin); - for (auto [block, end] = block_span; block < end; ++block) + unrequested.reserve(block_span.end - block_span.begin); + for (auto [begin, i] = block_span; i > begin; --i) { - if (!mediator_->client_has_block(block)) + if (auto const block = i - 1U; !mediator->client_has_block(block)) { - n_reqs.try_emplace(block, mediator_->count_active_requests(block)); + unrequested.insert(block); } } } @@ -87,10 +77,17 @@ class Wishlist::Impl return compare(that) < 0; } + [[nodiscard]] constexpr auto block_belongs(tr_block_index_t const block) const + { + return block_span.begin <= block && block < block_span.end; + } + tr_piece_index_t piece; tr_block_span_t block_span; - small::map n_reqs; + // This is sorted in reverse order so that smaller blocks indices + // can be taken from the end of the list, avoiding a move operation. + small::set, std::greater<>> unrequested; // Caching the following 2 values are highly beneficial, because: // - they are often used (mainly because resort_piece() is called @@ -102,9 +99,6 @@ class Wishlist::Impl tr_priority_t priority; tr_piece_index_t salt; - - private: - Mediator const* mediator_; }; using CandidateVec = std::vector; @@ -114,35 +108,16 @@ public: [[nodiscard]] std::vector next( size_t n_wanted_blocks, - std::function const& peer_has_piece, - std::function const& has_active_request_to_peer); + std::function const& peer_has_piece); private: - constexpr void set_candidates_dirty() noexcept - { - candidates_dirty_ = true; - } - - // --- - TR_CONSTEXPR20 void dec_replication() noexcept { - if (!candidates_dirty_) - { - std::for_each( - std::begin(candidates_), - std::end(candidates_), - [](Candidate& candidate) { --candidate.replication; }); - } + std::for_each(std::begin(candidates_), std::end(candidates_), [](Candidate& candidate) { --candidate.replication; }); } TR_CONSTEXPR20 void dec_replication_bitfield(tr_bitfield const& bitfield) { - if (candidates_dirty_) - { - return; - } - if (bitfield.has_none()) { return; @@ -167,22 +142,11 @@ private: TR_CONSTEXPR20 void inc_replication() noexcept { - if (!candidates_dirty_) - { - std::for_each( - std::begin(candidates_), - std::end(candidates_), - [](Candidate& candidate) { ++candidate.replication; }); - } + std::for_each(std::begin(candidates_), std::end(candidates_), [](Candidate& candidate) { ++candidate.replication; }); } void inc_replication_bitfield(tr_bitfield const& bitfield) { - if (candidates_dirty_) - { - return; - } - if (bitfield.has_none()) { return; @@ -205,13 +169,8 @@ private: std::sort(std::begin(candidates_), std::end(candidates_)); } - TR_CONSTEXPR20 void inc_replication_piece(tr_piece_index_t piece) + TR_CONSTEXPR20 void inc_replication_piece(tr_piece_index_t const piece) { - if (candidates_dirty_) - { - return; - } - if (auto iter = find_by_piece(piece); iter != std::end(candidates_)) { ++iter->replication; @@ -221,89 +180,56 @@ private: // --- - TR_CONSTEXPR20 void inc_active_request_span(tr_block_span_t block_span) + TR_CONSTEXPR20 void requested_block_span(tr_block_span_t const block_span) { - if (candidates_dirty_) - { - return; - } - for (auto block = block_span.begin; block < block_span.end;) { auto it_p = find_by_block(block); if (it_p == std::end(candidates_)) { - set_candidates_dirty(); + // std::unreachable(); break; } - auto& n_reqs = it_p->n_reqs; + auto& unreq = it_p->unrequested; - auto it_b_begin = std::begin(n_reqs); - it_b_begin = it_b_begin->first >= block_span.begin ? it_b_begin : n_reqs.lower_bound(block_span.begin); + auto it_b_end = std::end(unreq); + it_b_end = *std::prev(it_b_end) >= block_span.begin ? it_b_end : unreq.upper_bound(block_span.begin); - auto it_b_end = std::end(n_reqs); - it_b_end = std::prev(it_b_end)->first < block_span.end ? it_b_end : n_reqs.lower_bound(block_span.end); + auto it_b_begin = std::begin(unreq); + it_b_begin = *it_b_begin < block_span.end ? it_b_begin : unreq.upper_bound(block_span.end); - for (auto it_b = it_b_begin; it_b != it_b_end; ++it_b) - { - ++it_b->second; - } + unreq.erase(it_b_begin, it_b_end); block = it_p->block_span.end; + + resort_piece(it_p); } } - TR_CONSTEXPR20 void dec_active_request_block(tr_block_index_t block) + TR_CONSTEXPR20 void reset_block(tr_block_index_t block) { - if (candidates_dirty_) - { - return; - } - if (auto it_p = find_by_block(block); it_p != std::end(candidates_)) { - auto& n_reqs = it_p->n_reqs; - if (auto it_b = n_reqs.find(block); it_b != std::end(n_reqs) && it_b->second > 0U) - { - --it_b->second; - } + it_p->unrequested.insert(block); + resort_piece(it_p); } } - TR_CONSTEXPR20 void dec_active_request_bitfield(tr_bitfield const& requests) + TR_CONSTEXPR20 void reset_blocks_bitfield(tr_bitfield const& requests) { - if (candidates_dirty_) - { - return; - } - for (auto& candidate : candidates_) { - for (auto& [block, n_req] : candidate.n_reqs) + for (auto [begin, i] = candidate.block_span; i > begin; --i) { - if (n_req > 0U && requests.test(block)) + if (auto const block = i - 1U; requests.test(block)) { - --n_req; + candidate.unrequested.insert(block); } } } - } - // --- - - TR_CONSTEXPR20 void client_got_block(tr_block_index_t block) - { - if (candidates_dirty_) - { - return; - } - - if (auto iter = find_by_block(block); iter != std::end(candidates_)) - { - iter->n_reqs.erase(block); - resort_piece(iter); - } + std::sort(std::begin(candidates_), std::end(candidates_)); } // --- @@ -311,7 +237,21 @@ private: TR_CONSTEXPR20 void peer_disconnect(tr_bitfield const& have, tr_bitfield const& requests) { dec_replication_bitfield(have); - dec_active_request_bitfield(requests); + reset_blocks_bitfield(requests); + } + + // --- + + TR_CONSTEXPR20 void got_bad_piece(tr_piece_index_t const piece) + { + if (auto iter = find_by_piece(piece); iter != std::end(candidates_)) + { + for (auto [begin, i] = iter->block_span; i > begin; --i) + { + iter->unrequested.insert(i - 1U); + } + resort_piece(iter); + } } // --- @@ -329,7 +269,7 @@ private: return std::find_if( std::begin(candidates_), std::end(candidates_), - [block](auto const& c) { return c.block_span.begin <= block && block < c.block_span.end; }); + [block](auto const& c) { return c.block_belongs(block); }); } static constexpr tr_piece_index_t get_salt( @@ -357,41 +297,10 @@ private: return piece + 1U; } - void maybe_rebuild_candidate_list() - { - if (!candidates_dirty_) - { - return; - } - candidates_dirty_ = false; - candidates_.clear(); - - auto salter = tr_salt_shaker{}; - auto const is_sequential = mediator_.is_sequential_download(); - auto const n_pieces = mediator_.piece_count(); - candidates_.reserve(n_pieces); - for (tr_piece_index_t piece = 0U; piece < n_pieces; ++piece) - { - if (mediator_.client_has_piece(piece) || !mediator_.client_wants_piece(piece)) - { - continue; - } - - auto const salt = get_salt(piece, n_pieces, salter(), is_sequential); - candidates_.emplace_back(piece, salt, &mediator_); - } - std::sort(std::begin(candidates_), std::end(candidates_)); - } - // --- void recalculate_wanted_pieces() { - if (candidates_dirty_) - { - return; - } - auto n_old_c = std::size(candidates_); auto salter = tr_salt_shaker{}; auto const is_sequential = mediator_.is_sequential_download(); @@ -431,24 +340,43 @@ private: TR_CONSTEXPR20 void remove_piece(tr_piece_index_t const piece) { - if (candidates_dirty_) - { - return; - } - if (auto iter = find_by_piece(piece); iter != std::end(candidates_)) { candidates_.erase(iter); } } - TR_CONSTEXPR20 void resort_piece(CandidateVec::iterator const& pos_old) + // --- + + void recalculate_salt() { - if (candidates_dirty_) + auto salter = tr_salt_shaker{}; + auto const is_sequential = mediator_.is_sequential_download(); + auto const n_pieces = mediator_.piece_count(); + for (auto& candidate : candidates_) { - return; + candidate.salt = get_salt(candidate.piece, n_pieces, salter(), is_sequential); } + std::sort(std::begin(candidates_), std::end(candidates_)); + } + + // --- + + void recalculate_priority() + { + for (auto& candidate : candidates_) + { + candidate.priority = mediator_.priority(candidate.piece); + } + + std::sort(std::begin(candidates_), std::end(candidates_)); + } + + // --- + + TR_CONSTEXPR20 void resort_piece(CandidateVec::iterator const& pos_old) + { auto const pos_begin = std::begin(candidates_); // Candidate needs to be moved towards the front of the list @@ -466,8 +394,6 @@ private: } CandidateVec candidates_; - bool candidates_dirty_ = true; - bool is_endgame_ = false; std::array const tags_; @@ -476,41 +402,64 @@ private: Wishlist::Impl::Impl(Mediator& mediator_in) : tags_{ { + // candidates mediator_in.observe_files_wanted_changed([this](tr_torrent*, tr_file_index_t const*, tr_file_index_t, bool) { recalculate_wanted_pieces(); }), + // replication, unrequested mediator_in.observe_peer_disconnect([this](tr_torrent*, tr_bitfield const& b, tr_bitfield const& ar) { peer_disconnect(b, ar); }), - mediator_in.observe_got_bad_piece([this](tr_torrent*, tr_piece_index_t) { set_candidates_dirty(); }), + // unrequested + mediator_in.observe_got_bad_piece([this](tr_torrent*, tr_piece_index_t p) { got_bad_piece(p); }), + // replication mediator_in.observe_got_bitfield([this](tr_torrent*, tr_bitfield const& b) { inc_replication_bitfield(b); }), - mediator_in.observe_got_block([this](tr_torrent*, tr_block_index_t b) { client_got_block(b); }), - mediator_in.observe_got_choke([this](tr_torrent*, tr_bitfield const& b) { dec_active_request_bitfield(b); }), + // unrequested + mediator_in.observe_got_choke([this](tr_torrent*, tr_bitfield const& b) { reset_blocks_bitfield(b); }), + // replication mediator_in.observe_got_have([this](tr_torrent*, tr_piece_index_t p) { inc_replication_piece(p); }), + // replication mediator_in.observe_got_have_all([this](tr_torrent*) { inc_replication(); }), - mediator_in.observe_got_reject([this](tr_torrent*, tr_peer*, tr_block_index_t b) { dec_active_request_block(b); }), + // unrequested + mediator_in.observe_got_reject([this](tr_torrent*, tr_peer*, tr_block_index_t b) { reset_block(b); }), + // candidates mediator_in.observe_piece_completed([this](tr_torrent*, tr_piece_index_t p) { remove_piece(p); }), + // priority mediator_in.observe_priority_changed([this](tr_torrent*, tr_file_index_t const*, tr_file_index_t, tr_priority_t) - { set_candidates_dirty(); }), - mediator_in.observe_sent_cancel([this](tr_torrent*, tr_peer*, tr_block_index_t b) { dec_active_request_block(b); }), - mediator_in.observe_sent_request([this](tr_torrent*, tr_peer*, tr_block_span_t bs) { inc_active_request_span(bs); }), - mediator_in.observe_sequential_download_changed([this](tr_torrent*, bool) { set_candidates_dirty(); }), + { recalculate_priority(); }), + // unrequested + mediator_in.observe_sent_cancel([this](tr_torrent*, tr_peer*, tr_block_index_t b) { reset_block(b); }), + // unrequested + mediator_in.observe_sent_request([this](tr_torrent*, tr_peer*, tr_block_span_t bs) { requested_block_span(bs); }), + // salt + mediator_in.observe_sequential_download_changed([this](tr_torrent*, bool) { recalculate_salt(); }), } } , mediator_{ mediator_in } { + auto salter = tr_salt_shaker{}; + auto const is_sequential = mediator_.is_sequential_download(); + auto const n_pieces = mediator_.piece_count(); + candidates_.reserve(n_pieces); + for (tr_piece_index_t piece = 0U; piece < n_pieces; ++piece) + { + if (mediator_.client_has_piece(piece) || !mediator_.client_wants_piece(piece)) + { + continue; + } + + auto const salt = get_salt(piece, n_pieces, salter(), is_sequential); + candidates_.emplace_back(piece, salt, &mediator_); + } + std::sort(std::begin(candidates_), std::end(candidates_)); } std::vector Wishlist::Impl::next( - size_t n_wanted_blocks, - std::function const& peer_has_piece, - std::function const& has_active_request_to_peer) + size_t const n_wanted_blocks, + std::function const& peer_has_piece) { if (n_wanted_blocks == 0U) { return {}; } - maybe_rebuild_candidate_list(); - - auto const max_peers = is_endgame_ ? EndgameMaxPeers : NormalMaxPeers; auto blocks = small::vector{}; blocks.reserve(n_wanted_blocks); for (auto const& candidate : candidates_) @@ -527,49 +476,28 @@ std::vector Wishlist::Impl::next( continue; } - // walk the blocks in this piece that we don't have - for (auto const& [block, n_req] : candidate.n_reqs) + // walk the blocks in this piece that we don't have or not requested + for (auto it = std::rbegin(candidate.unrequested), end = std::rend(candidate.unrequested); it != end; ++it) { if (std::size(blocks) >= n_wanted_blocks) { break; } -#ifdef TR_WISHLIST_ASSERT - auto const n_req_truth = mediator_.count_active_requests(block); - TR_ASSERT_MSG( - n_req == n_req_truth, - fmt::format("piece = {}, block = {}, n_req = {}, truth = {}", candidate.piece, block, n_req, n_req_truth)); -#endif - - // don't request from too many peers - if (n_req >= max_peers) - { - continue; - } - - // don't request block from peers which we already requested from - if (has_active_request_to_peer(block)) - { - continue; - } - - blocks.emplace_back(block); + blocks.emplace_back(*it); } } - is_endgame_ = std::size(blocks) < n_wanted_blocks; - // Ensure the list of blocks are sorted // The list needs to be unique as well, but that should come naturally std::sort(std::begin(blocks), std::end(blocks)); return make_spans(blocks); } -int Wishlist::Impl::Candidate::compare(Wishlist::Impl::Candidate const& that) const noexcept +int Wishlist::Impl::Candidate::compare(Candidate const& that) const noexcept { // prefer pieces closer to completion - if (auto const val = tr_compare_3way(std::size(n_reqs), std::size(that.n_reqs)); val != 0) + if (auto const val = tr_compare_3way(std::size(unrequested), std::size(that.unrequested)); val != 0) { return val; } @@ -599,9 +527,8 @@ Wishlist::Wishlist(Mediator& mediator_in) Wishlist::~Wishlist() = default; std::vector Wishlist::next( - size_t n_wanted_blocks, - std::function const& peer_has_piece, - std::function const& has_active_pending_to_peer) + size_t const n_wanted_blocks, + std::function const& peer_has_piece) { - return impl_->next(n_wanted_blocks, peer_has_piece, has_active_pending_to_peer); + return impl_->next(n_wanted_blocks, peer_has_piece); } diff --git a/libtransmission/peer-mgr-wishlist.h b/libtransmission/peer-mgr-wishlist.h index 6f4d5c7e7..1a3c5b821 100644 --- a/libtransmission/peer-mgr-wishlist.h +++ b/libtransmission/peer-mgr-wishlist.h @@ -28,16 +28,12 @@ struct tr_peer; class Wishlist { public: - static auto constexpr EndgameMaxPeers = size_t{ 2U }; - static auto constexpr NormalMaxPeers = size_t{ 1U }; - struct Mediator { [[nodiscard]] virtual bool client_has_block(tr_block_index_t block) const = 0; [[nodiscard]] virtual bool client_has_piece(tr_piece_index_t piece) const = 0; [[nodiscard]] virtual bool client_wants_piece(tr_piece_index_t piece) const = 0; [[nodiscard]] virtual bool is_sequential_download() const = 0; - [[nodiscard]] virtual uint8_t count_active_requests(tr_block_index_t block) const = 0; [[nodiscard]] virtual size_t count_piece_replication(tr_piece_index_t piece) const = 0; [[nodiscard]] virtual tr_block_span_t block_span(tr_piece_index_t piece) const = 0; [[nodiscard]] virtual tr_piece_index_t piece_count() const = 0; @@ -51,8 +47,6 @@ public: libtransmission::SimpleObservable::Observer observer) = 0; [[nodiscard]] virtual libtransmission::ObserverTag observe_got_bitfield( libtransmission::SimpleObservable::Observer observer) = 0; - [[nodiscard]] virtual libtransmission::ObserverTag observe_got_block( - libtransmission::SimpleObservable::Observer observer) = 0; [[nodiscard]] virtual libtransmission::ObserverTag observe_got_choke( libtransmission::SimpleObservable::Observer observer) = 0; [[nodiscard]] virtual libtransmission::ObserverTag observe_got_have( @@ -82,8 +76,7 @@ public: // the next blocks that we should request from a peer [[nodiscard]] std::vector next( size_t n_wanted_blocks, - std::function const& peer_has_piece, - std::function const& has_active_pending_to_peer); + std::function const& peer_has_piece); private: class Impl; diff --git a/libtransmission/peer-mgr.cc b/libtransmission/peer-mgr.cc index f8bf62da4..af92745f2 100644 --- a/libtransmission/peer-mgr.cc +++ b/libtransmission/peer-mgr.cc @@ -384,7 +384,6 @@ public: [[nodiscard]] bool client_has_piece(tr_piece_index_t piece) const override; [[nodiscard]] bool client_wants_piece(tr_piece_index_t piece) const override; [[nodiscard]] bool is_sequential_download() const override; - [[nodiscard]] uint8_t count_active_requests(tr_block_index_t block) const override; [[nodiscard]] size_t count_piece_replication(tr_piece_index_t piece) const override; [[nodiscard]] tr_block_span_t block_span(tr_piece_index_t piece) const override; [[nodiscard]] tr_piece_index_t piece_count() const override; @@ -399,8 +398,6 @@ public: libtransmission::SimpleObservable::Observer observer) override; [[nodiscard]] libtransmission::ObserverTag observe_got_bitfield( libtransmission::SimpleObservable::Observer observer) override; - [[nodiscard]] libtransmission::ObserverTag observe_got_block( - libtransmission::SimpleObservable::Observer observer) override; [[nodiscard]] libtransmission::ObserverTag observe_got_choke( libtransmission::SimpleObservable::Observer observer) override; [[nodiscard]] libtransmission::ObserverTag observe_got_have( @@ -670,7 +667,6 @@ public: libtransmission::SimpleObservable peer_disconnect; libtransmission::SimpleObservable got_bitfield; - libtransmission::SimpleObservable got_block; libtransmission::SimpleObservable got_choke; libtransmission::SimpleObservable got_have; libtransmission::SimpleObservable got_have_all; @@ -910,7 +906,6 @@ private: peer->blocks_sent_to_client.add(tr_time(), 1); peer->blame.set(loc.piece); tor->on_block_received(loc.block); - s->got_block.emit(tor, loc.block); } break; @@ -1025,16 +1020,6 @@ bool tr_swarm::WishlistMediator::is_sequential_download() const return tor_.is_sequential_download(); } -uint8_t tr_swarm::WishlistMediator::count_active_requests(tr_block_index_t block) const -{ - auto const op = [block](uint8_t acc, auto const& peer) - { - return acc + (peer->active_requests.test(block) ? 1U : 0U); - }; - return std::accumulate(std::begin(swarm_.peers), std::end(swarm_.peers), uint8_t{}, op) + - std::accumulate(std::begin(swarm_.webseeds), std::end(swarm_.webseeds), uint8_t{}, op); -} - size_t tr_swarm::WishlistMediator::count_piece_replication(tr_piece_index_t piece) const { auto const op = [piece](size_t acc, auto const& peer) @@ -1094,12 +1079,6 @@ libtransmission::ObserverTag tr_swarm::WishlistMediator::observe_got_bitfield( return swarm_.got_bitfield.observe(std::move(observer)); } -libtransmission::ObserverTag tr_swarm::WishlistMediator::observe_got_block( - libtransmission::SimpleObservable::Observer observer) -{ - return swarm_.got_block.observe(std::move(observer)); -} - libtransmission::ObserverTag tr_swarm::WishlistMediator::observe_got_choke( libtransmission::SimpleObservable::Observer observer) { @@ -1324,15 +1303,13 @@ void tr_peerMgrFree(tr_peerMgr* manager) std::vector tr_peerMgrGetNextRequests(tr_torrent* torrent, tr_peer const* peer, size_t numwant) { TR_ASSERT(!torrent->is_done()); - tr_swarm& swarm = *torrent->swarm; + tr_swarm const& swarm = *torrent->swarm; + TR_ASSERT(swarm.wishlist); if (!swarm.wishlist) { - swarm.wishlist = std::make_unique(swarm.wishlist_mediator); + return {}; } - return swarm.wishlist->next( - numwant, - [peer](tr_piece_index_t p) { return peer->has_piece(p); }, - [peer](tr_block_index_t b) { return peer->active_requests.test(b); }); + return swarm.wishlist->next(numwant, [peer](tr_piece_index_t p) { return peer->has_piece(p); }); } namespace @@ -1720,6 +1697,7 @@ void tr_swarm::on_torrent_started() auto const lock = unique_lock(); is_running = true; manager->rechokeSoon(); + wishlist = std::make_unique(wishlist_mediator); } void tr_swarm::on_torrent_stopped() diff --git a/libtransmission/peer-msgs.cc b/libtransmission/peer-msgs.cc index 7213cbbd4..b901704d1 100644 --- a/libtransmission/peer-msgs.cc +++ b/libtransmission/peer-msgs.cc @@ -745,7 +745,7 @@ private: static auto constexpr SendPexInterval = 90s; // how many seconds we expect the next piece block to arrive - static auto constexpr RequestTimeoutSecs = time_t{ 90 }; + static auto constexpr RequestTimeoutSecs = time_t{ 15 }; }; // --- diff --git a/tests/libtransmission/peer-mgr-wishlist-test.cc b/tests/libtransmission/peer-mgr-wishlist-test.cc index 4f4b90a52..395c6fb75 100644 --- a/tests/libtransmission/peer-mgr-wishlist-test.cc +++ b/tests/libtransmission/peer-mgr-wishlist-test.cc @@ -60,11 +60,6 @@ protected: return is_sequential_download_; } - [[nodiscard]] uint8_t count_active_requests(tr_block_index_t block) const override - { - return active_request_count_[block]; - } - [[nodiscard]] size_t count_piece_replication(tr_piece_index_t piece) const override { return piece_replication_[piece]; @@ -80,7 +75,7 @@ protected: return piece_count_; } - [[nodiscard]] tr_priority_t priority(tr_piece_index_t piece) const final + [[nodiscard]] tr_priority_t priority(tr_piece_index_t piece) const override { return piece_priority_[piece]; } @@ -110,12 +105,6 @@ protected: return parent_.got_bitfield_.observe(std::move(observer)); } - [[nodiscard]] libtransmission::ObserverTag observe_got_block( - libtransmission::SimpleObservable::Observer observer) override - { - return parent_.got_block_.observe(std::move(observer)); - } - [[nodiscard]] libtransmission::ObserverTag observe_got_choke( libtransmission::SimpleObservable::Observer observer) override { @@ -176,7 +165,6 @@ protected: libtransmission::SimpleObservable peer_disconnect_; libtransmission::SimpleObservable got_bad_piece_; libtransmission::SimpleObservable got_bitfield_; - libtransmission::SimpleObservable got_block_; libtransmission::SimpleObservable got_choke_; libtransmission::SimpleObservable got_have_; libtransmission::SimpleObservable got_have_all_; @@ -191,10 +179,6 @@ protected: { return true; }; - static auto constexpr ClientHasNoActiveRequests = [](tr_block_index_t) - { - return false; - }; }; TEST_F(PeerMgrWishlistTest, doesNotRequestPiecesThatAreNotWanted) @@ -216,8 +200,7 @@ TEST_F(PeerMgrWishlistTest, doesNotRequestPiecesThatAreNotWanted) mediator.client_wants_piece_.insert(0); // we should only get the first piece back - auto wishlist = Wishlist{ mediator }; - auto const spans = wishlist.next(1000, PeerHasAllPieces, ClientHasNoActiveRequests); + auto const spans = Wishlist{ mediator }.next(1000, PeerHasAllPieces); ASSERT_EQ(1U, std::size(spans)); EXPECT_EQ(mediator.block_span_[0].begin, spans[0].begin); EXPECT_EQ(mediator.block_span_[0].end, spans[0].end); @@ -252,11 +235,11 @@ TEST_F(PeerMgrWishlistTest, onlyRequestBlocksThePeerHas) // even if we ask wishlist for more blocks than what the peer has, // it should only return blocks [100..200) - auto const spans = Wishlist{ mediator }.next(250, IsPieceOne, ClientHasNoActiveRequests); + auto const spans = Wishlist{ mediator }.next(250, IsPieceOne); auto requested = tr_bitfield{ 250 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(100U, requested.count()); EXPECT_EQ(0U, requested.count(0, 100)); @@ -264,7 +247,7 @@ TEST_F(PeerMgrWishlistTest, onlyRequestBlocksThePeerHas) EXPECT_EQ(0U, requested.count(200, 250)); } -TEST_F(PeerMgrWishlistTest, doesNotRequestSameBlockTwiceFromSamePeer) +TEST_F(PeerMgrWishlistTest, doesNotRequestSameBlockTwice) { auto mediator = MockMediator{ *this }; @@ -284,117 +267,26 @@ TEST_F(PeerMgrWishlistTest, doesNotRequestSameBlockTwiceFromSamePeer) mediator.client_wants_piece_.insert(1); mediator.client_wants_piece_.insert(2); + // allow the wishlist to build its cache + auto wishlist = Wishlist{ mediator }; + // but we've already requested blocks [0..10) from this peer, // so we don't want to send repeated requests - static auto constexpr IsBetweenZeroToTen = [](tr_block_index_t b) - { - return b < 10U; - }; + sent_request_.emit(nullptr, nullptr, { 0, 10 }); // even if we ask wishlist for all the blocks, // it should omit blocks [0..10) from the return set - auto const spans = Wishlist{ mediator }.next(250, PeerHasAllPieces, IsBetweenZeroToTen); + auto const spans = wishlist.next(250, PeerHasAllPieces); auto requested = tr_bitfield{ 250 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(240U, requested.count()); EXPECT_EQ(0U, requested.count(0, 10)); EXPECT_EQ(240U, requested.count(10, 250)); } -TEST_F(PeerMgrWishlistTest, doesNotRequestDupesWhenNotInEndgame) -{ - auto mediator = MockMediator{ *this }; - // setup: three pieces, all missing - mediator.piece_count_ = 3; - mediator.block_span_[0] = { 0, 100 }; - mediator.block_span_[1] = { 100, 200 }; - mediator.block_span_[2] = { 200, 250 }; - - // peer has all pieces - mediator.piece_replication_[0] = 1; - mediator.piece_replication_[1] = 1; - mediator.piece_replication_[2] = 1; - - // and we want all three pieces - mediator.client_wants_piece_.insert(0); - mediator.client_wants_piece_.insert(1); - mediator.client_wants_piece_.insert(2); - - // but we've already requested blocks [0..10) from someone else, - // and it is not endgame, so we don't want to send repeated requests - for (tr_block_index_t block = 0; block < 10; ++block) - { - mediator.active_request_count_[block] = 1; - } - - // even if we ask wishlist for all the blocks, - // it should omit blocks [0..10) from the return set - auto const spans = Wishlist{ mediator }.next(250, PeerHasAllPieces, ClientHasNoActiveRequests); - auto requested = tr_bitfield{ 250 }; - for (auto const& span : spans) - { - requested.set_span(span.begin, span.end); - } - EXPECT_EQ(240U, requested.count()); - EXPECT_EQ(0U, requested.count(0, 10)); - EXPECT_EQ(240U, requested.count(10, 250)); -} - -TEST_F(PeerMgrWishlistTest, onlyRequestsDupesDuringEndgame) -{ - auto mediator = MockMediator{ *this }; - - // setup: three pieces, all missing - mediator.piece_count_ = 3; - mediator.block_span_[0] = { 0, 100 }; - mediator.block_span_[1] = { 100, 200 }; - mediator.block_span_[2] = { 200, 250 }; - - // peer has all pieces - mediator.piece_replication_[0] = 1; - mediator.piece_replication_[1] = 1; - mediator.piece_replication_[2] = 1; - - // and we want all three pieces - mediator.client_wants_piece_.insert(0); - mediator.client_wants_piece_.insert(1); - mediator.client_wants_piece_.insert(2); - - // we've already requested blocks [0..10) from someone else, - // but it is endgame, so we can request each block twice. - // blocks [5..10) are already requested twice - for (tr_block_index_t block = 0; block < 5; ++block) - { - mediator.active_request_count_[block] = 1; - } - for (tr_block_index_t block = 5; block < 10; ++block) - { - mediator.active_request_count_[block] = 2; - } - - auto wishlist = Wishlist{ mediator }; - - // the endgame state takes effect after it runs out of - // blocks for the first time, so we trigger it here - (void)wishlist.next(1000, PeerHasAllPieces, ClientHasNoActiveRequests); - - // if we ask wishlist for more blocks than exist, - // it should omit blocks [5..10) from the return set - auto const spans = wishlist.next(1000, PeerHasAllPieces, ClientHasNoActiveRequests); - auto requested = tr_bitfield{ 250 }; - for (auto const& span : spans) - { - requested.set_span(span.begin, span.end); - } - EXPECT_EQ(245U, requested.count()); - EXPECT_EQ(5U, requested.count(0, 5)); - EXPECT_EQ(0U, requested.count(5, 10)); - EXPECT_EQ(240U, requested.count(10, 250)); -} - TEST_F(PeerMgrWishlistTest, sequentialDownload) { auto const get_spans = [this](size_t n_wanted) @@ -420,7 +312,7 @@ TEST_F(PeerMgrWishlistTest, sequentialDownload) // we enabled sequential download mediator.is_sequential_download_ = true; - return Wishlist{ mediator }.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return Wishlist{ mediator }.next(n_wanted, PeerHasAllPieces); }; // when we ask for blocks, apart from the last piece, @@ -434,9 +326,9 @@ TEST_F(PeerMgrWishlistTest, sequentialDownload) { auto requested = tr_bitfield{ 250 }; auto const spans = get_spans(100); - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(100U, requested.count()); EXPECT_EQ(50U, requested.count(0, 100)); @@ -449,9 +341,9 @@ TEST_F(PeerMgrWishlistTest, sequentialDownload) { auto requested = tr_bitfield{ 250 }; auto const spans = get_spans(200); - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(200U, requested.count()); EXPECT_EQ(100U, requested.count(0, 100)); @@ -483,14 +375,14 @@ TEST_F(PeerMgrWishlistTest, doesNotRequestTooManyBlocks) // but we only ask for 10 blocks, // so that's how many we should get back - auto const n_wanted = 10U; - auto const spans = Wishlist{ mediator }.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + static constexpr auto NumWanted = 10U; + auto const spans = Wishlist{ mediator }.next(NumWanted, PeerHasAllPieces); auto n_got = size_t{}; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - n_got += span.end - span.begin; + n_got += end - begin; } - EXPECT_EQ(n_wanted, n_got); + EXPECT_EQ(NumWanted, n_got); } TEST_F(PeerMgrWishlistTest, prefersHighPriorityPieces) @@ -519,7 +411,7 @@ TEST_F(PeerMgrWishlistTest, prefersHighPriorityPieces) // and the second piece is high priority mediator.piece_priority_[1] = TR_PRI_HIGH; - return Wishlist{ mediator }.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return Wishlist{ mediator }.next(n_wanted, PeerHasAllPieces); }; // wishlist should pick the high priority piece's blocks first. @@ -532,9 +424,9 @@ TEST_F(PeerMgrWishlistTest, prefersHighPriorityPieces) { auto const spans = get_spans(10); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(10U, requested.count()); EXPECT_EQ(0U, requested.count(0, 100)); @@ -571,16 +463,16 @@ TEST_F(PeerMgrWishlistTest, prefersNearlyCompletePieces) static_assert(std::size(MissingBlockCount) == 3); for (tr_piece_index_t piece = 0; piece < 3; ++piece) { - auto const& span = mediator.block_span_[piece]; - auto const have_end = span.end - MissingBlockCount[piece]; + auto const& [begin, end] = mediator.block_span_[piece]; + auto const have_end = end - MissingBlockCount[piece]; - for (tr_piece_index_t i = span.begin; i < have_end; ++i) + for (tr_piece_index_t i = begin; i < have_end; ++i) { mediator.client_has_block_.insert(i); } } - return Wishlist{ mediator }.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return Wishlist{ mediator }.next(n_wanted, PeerHasAllPieces); }; // wishlist prefers to get pieces completed ASAP, so it @@ -591,11 +483,11 @@ TEST_F(PeerMgrWishlistTest, prefersNearlyCompletePieces) static auto constexpr NumRuns = 1000; for (int run = 0; run < NumRuns; ++run) { - auto const ranges = get_spans(10); + auto const spans = get_spans(10); auto requested = tr_bitfield{ 300 }; - for (auto const& range : ranges) + for (auto const& [begin, end] : spans) { - requested.set_span(range.begin, range.end); + requested.set_span(begin, end); } EXPECT_EQ(10U, requested.count()); EXPECT_EQ(10U, requested.count(0, 100)); @@ -607,11 +499,11 @@ TEST_F(PeerMgrWishlistTest, prefersNearlyCompletePieces) // those blocks should be next in line. for (int run = 0; run < NumRuns; ++run) { - auto const ranges = get_spans(20); + auto const spans = get_spans(20); auto requested = tr_bitfield{ 300 }; - for (auto const& range : ranges) + for (auto const& [begin, end] : spans) { - requested.set_span(range.begin, range.end); + requested.set_span(begin, end); } EXPECT_EQ(20U, requested.count()); EXPECT_EQ(10U, requested.count(0, 100)); @@ -643,7 +535,7 @@ TEST_F(PeerMgrWishlistTest, prefersRarerPieces) mediator.piece_replication_[1] = 3; mediator.piece_replication_[2] = 2; - return Wishlist{ mediator }.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return Wishlist{ mediator }.next(n_wanted, PeerHasAllPieces); }; // wishlist prefers to request rarer pieces, so it @@ -656,9 +548,9 @@ TEST_F(PeerMgrWishlistTest, prefersRarerPieces) { auto const spans = get_spans(100); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(100U, requested.count()); EXPECT_EQ(100U, requested.count(0, 100)); @@ -672,9 +564,9 @@ TEST_F(PeerMgrWishlistTest, prefersRarerPieces) { auto const spans = get_spans(150); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(150U, requested.count()); EXPECT_EQ(100U, requested.count(0, 100)); @@ -708,7 +600,6 @@ TEST_F(PeerMgrWishlistTest, peerDisconnectDecrementsReplication) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // a peer that has only the first piece disconnected, now the // first piece should be the rarest piece according to the cache @@ -719,7 +610,7 @@ TEST_F(PeerMgrWishlistTest, peerDisconnectDecrementsReplication) // this is what a real mediator should return at this point: // mediator.piece_replication_[0] = 1; - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; // wishlist prefers to request rarer pieces, so it @@ -732,9 +623,9 @@ TEST_F(PeerMgrWishlistTest, peerDisconnectDecrementsReplication) { auto const spans = get_spans(100); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(100U, requested.count()); EXPECT_EQ(100U, requested.count(0, 100)); @@ -748,9 +639,9 @@ TEST_F(PeerMgrWishlistTest, peerDisconnectDecrementsReplication) { auto const spans = get_spans(150); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(150U, requested.count()); EXPECT_EQ(100U, requested.count(0, 100)); @@ -758,22 +649,18 @@ TEST_F(PeerMgrWishlistTest, peerDisconnectDecrementsReplication) } } -TEST_F(PeerMgrWishlistTest, gotBadPieceRebuildsWishlist) +TEST_F(PeerMgrWishlistTest, gotBadPieceResetsPiece) { auto const get_spans = [this](size_t n_wanted) { auto mediator = MockMediator{ *this }; - // setup: three pieces, we thought we have all of them + // setup: three pieces, all missing mediator.piece_count_ = 3; mediator.block_span_[0] = { 0, 100 }; mediator.block_span_[1] = { 100, 200 }; mediator.block_span_[2] = { 200, 300 }; - mediator.client_has_piece_.insert(0); - mediator.client_has_piece_.insert(1); - mediator.client_has_piece_.insert(2); - // and we want everything for (tr_piece_index_t i = 0; i < 3; ++i) { @@ -787,49 +674,38 @@ TEST_F(PeerMgrWishlistTest, gotBadPieceRebuildsWishlist) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); - // piece 1 turns out to be corrupted - got_bad_piece_.emit(nullptr, 1); - mediator.client_has_piece_.erase(1); + // we already requested 50 blocks each from every piece + sent_request_.emit(nullptr, nullptr, { 0, 50 }); + sent_request_.emit(nullptr, nullptr, { 100, 150 }); + sent_request_.emit(nullptr, nullptr, { 200, 250 }); - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + // we request the rest of a random piece + auto const random_piece = tr_rand_int(3U); + sent_request_.emit(nullptr, nullptr, { (random_piece * 100U) + 50U, (random_piece + 1U) * 100U }); + + // the random piece turns out to be corrupted, so all blocks should be missing again + got_bad_piece_.emit(nullptr, random_piece); + + return std::pair{ wishlist.next(n_wanted, PeerHasAllPieces), random_piece }; }; - // The wishlist should consider piece 1 missing, so it will request - // blocks from it. + // The wishlist should request the bad piece last, since it now became + // the piece with the most unrequested blocks. // NB: when all other things are equal in the wishlist, pieces are // picked at random so this test -could- pass even if there's a bug. // So test several times to shake out any randomness static auto constexpr NumRuns = 1000; for (int run = 0; run < NumRuns; ++run) { - auto const spans = get_spans(100); + auto const [spans, bad_piece] = get_spans(101); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } - EXPECT_EQ(100U, requested.count()); - EXPECT_EQ(0U, requested.count(0, 100)); - EXPECT_EQ(100U, requested.count(100, 200)); - EXPECT_EQ(0U, requested.count(200, 300)); - } - - // Same premise as previous test, but ask for more blocks. - // But since only piece 1 is missing, we will get 100 blocks only. - for (int run = 0; run < NumRuns; ++run) - { - auto const spans = get_spans(150); - auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) - { - requested.set_span(span.begin, span.end); - } - EXPECT_EQ(100U, requested.count()); - EXPECT_EQ(0U, requested.count(0, 100)); - EXPECT_EQ(100U, requested.count(100, 200)); - EXPECT_EQ(0U, requested.count(200, 300)); + EXPECT_EQ(101U, requested.count()); + EXPECT_EQ(1U, requested.count(bad_piece * size_t{ 100U }, (bad_piece + 1U) * size_t{ 100U })); } } @@ -858,7 +734,6 @@ TEST_F(PeerMgrWishlistTest, gotBitfieldIncrementsReplication) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // a peer with first 2 pieces connected and sent a bitfield, now the // third piece should be the rarest piece according to the cache @@ -870,7 +745,7 @@ TEST_F(PeerMgrWishlistTest, gotBitfieldIncrementsReplication) // mediator.piece_replication_[0] = 3; // mediator.piece_replication_[1] = 3; - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; // wishlist prefers to request rarer pieces, so it @@ -909,7 +784,7 @@ TEST_F(PeerMgrWishlistTest, gotBitfieldIncrementsReplication) } } -TEST_F(PeerMgrWishlistTest, gotBlockResortsPiece) +TEST_F(PeerMgrWishlistTest, sentRequestsResortsPiece) { auto const get_spans = [this](size_t n_wanted) { @@ -934,18 +809,16 @@ TEST_F(PeerMgrWishlistTest, gotBlockResortsPiece) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); - // we received block 0 from someone, the wishlist should resort the - // candidate list cache by consulting the mediator - mediator.client_has_block_.insert(0); - got_block_.emit(nullptr, 0); + // we requested block 0 from someone, the wishlist should resort the + // candidate list cache + sent_request_.emit(nullptr, nullptr, { 0, 1 }); - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; // wishlist prefers to get pieces completed ASAP, so it - // should pick the ones with the fewest missing blocks first. + // should pick the ones with the fewest unrequested blocks first. // NB: when all other things are equal in the wishlist, pieces are // picked at random so this test -could- pass even if there's a bug. // So test several times to shake out any randomness @@ -954,9 +827,9 @@ TEST_F(PeerMgrWishlistTest, gotBlockResortsPiece) { auto const spans = get_spans(100); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(100U, requested.count()); EXPECT_EQ(99U, requested.count(0, 100)); @@ -970,9 +843,9 @@ TEST_F(PeerMgrWishlistTest, gotBlockResortsPiece) { auto const spans = get_spans(150); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(150U, requested.count()); EXPECT_EQ(99U, requested.count(0, 100)); @@ -1005,7 +878,6 @@ TEST_F(PeerMgrWishlistTest, gotHaveIncrementsReplication) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // a peer sent a "Have" message for the first piece, now the // first piece should be the least rare piece according to the cache @@ -1014,7 +886,7 @@ TEST_F(PeerMgrWishlistTest, gotHaveIncrementsReplication) // this is what a real mediator should return at this point: // mediator.piece_replication_[0] = 3; - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; // wishlist prefers to request rarer pieces, so it @@ -1027,9 +899,9 @@ TEST_F(PeerMgrWishlistTest, gotHaveIncrementsReplication) { auto const spans = get_spans(200); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(200U, requested.count()); EXPECT_EQ(0U, requested.count(0, 100)); @@ -1043,9 +915,9 @@ TEST_F(PeerMgrWishlistTest, gotHaveIncrementsReplication) { auto const spans = get_spans(250); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(250U, requested.count()); EXPECT_EQ(50U, requested.count(0, 100)); @@ -1053,7 +925,7 @@ TEST_F(PeerMgrWishlistTest, gotHaveIncrementsReplication) } } -TEST_F(PeerMgrWishlistTest, gotChokeDecrementsActiveRequest) +TEST_F(PeerMgrWishlistTest, gotChokeResetsRequestedBlocks) { auto const get_spans = [this](size_t n_wanted) { @@ -1076,37 +948,33 @@ TEST_F(PeerMgrWishlistTest, gotChokeDecrementsActiveRequest) mediator.client_wants_piece_.insert(i); } - // we have active requests to the first 250 blocks - for (tr_block_index_t i = 0; i < 250; ++i) - { - mediator.active_request_count_[i] = 1; - } - // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); + + // we have active requests to the first 250 blocks + sent_request_.emit(nullptr, nullptr, { 0, 250 }); // a peer sent a "Choke" message, which cancels some active requests tr_bitfield requested{ 300 }; requested.set_span(0, 10); got_choke_.emit(nullptr, requested); - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; - // wishlist only picks blocks with no active requests when not in - // end game mode, which are [0, 10) and [250, 300). + // wishlist only picks blocks with no active requests, which are + // [0, 10) and [250, 300). // NB: when all other things are equal in the wishlist, pieces are // picked at random so this test -could- pass even if there's a bug. // So test several times to shake out any randomness static auto constexpr NumRuns = 1000; for (int run = 0; run < NumRuns; ++run) { - auto const ranges = get_spans(300); + auto const spans = get_spans(300); auto requested = tr_bitfield{ 300 }; - for (auto const& range : ranges) + for (auto const& [begin, end] : spans) { - requested.set_span(range.begin, range.end); + requested.set_span(begin, end); } EXPECT_EQ(60U, requested.count()); EXPECT_EQ(10U, requested.count(0, 10)); @@ -1140,7 +1008,6 @@ TEST_F(PeerMgrWishlistTest, gotHaveAllDoesNotAffectOrder) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // a peer sent a "Have All" message, this should not affect the piece order got_have_all_.emit(nullptr); @@ -1150,7 +1017,7 @@ TEST_F(PeerMgrWishlistTest, gotHaveAllDoesNotAffectOrder) // mediator.piece_replication_[1] = 3; // mediator.piece_replication_[2] = 4; - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; // wishlist prefers to request rarer pieces, so it @@ -1161,11 +1028,11 @@ TEST_F(PeerMgrWishlistTest, gotHaveAllDoesNotAffectOrder) static auto constexpr NumRuns = 1000; for (int run = 0; run < NumRuns; ++run) { - auto const ranges = get_spans(150); + auto const spans = get_spans(150); auto requested = tr_bitfield{ 300 }; - for (auto const& range : ranges) + for (auto const& [begin, end] : spans) { - requested.set_span(range.begin, range.end); + requested.set_span(begin, end); } EXPECT_EQ(150U, requested.count()); EXPECT_EQ(100U, requested.count(0, 100)); @@ -1178,9 +1045,9 @@ TEST_F(PeerMgrWishlistTest, gotHaveAllDoesNotAffectOrder) { auto const spans = get_spans(250); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(250U, requested.count()); EXPECT_EQ(200U, requested.count(0, 200)); @@ -1188,7 +1055,7 @@ TEST_F(PeerMgrWishlistTest, gotHaveAllDoesNotAffectOrder) } } -TEST_F(PeerMgrWishlistTest, gotRejectDecrementsActiveRequest) +TEST_F(PeerMgrWishlistTest, gotRejectResetsBlock) { auto const get_spans = [this](size_t n_wanted) { @@ -1211,45 +1078,37 @@ TEST_F(PeerMgrWishlistTest, gotRejectDecrementsActiveRequest) mediator.client_wants_piece_.insert(i); } - // we have active requests to the first 250 blocks - for (tr_block_index_t i = 0; i < 250; ++i) - { - mediator.active_request_count_[i] = 1; - } - // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); + + // we have active requests to the first 250 blocks + sent_request_.emit(nullptr, nullptr, { 0, 250 }); // a peer sent some "Reject" messages, which cancels active requests - auto rejected_set = std::set{}; auto rejected_bitfield = tr_bitfield{ 300 }; - for (tr_block_index_t i = 0, n = tr_rand_int(250U); i < n; ++i) - { - rejected_set.insert(tr_rand_int(250U)); - } - for (auto const block : rejected_set) + for (tr_block_index_t i = 0, n = tr_rand_int(250U); i <= n; ++i) { + auto const block = tr_rand_int(250U); rejected_bitfield.set(block); got_reject_.emit(nullptr, nullptr, block); } - return std::pair{ wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests), std::move(rejected_bitfield) }; + return std::pair{ wishlist.next(n_wanted, PeerHasAllPieces), std::move(rejected_bitfield) }; }; - // wishlist only picks blocks with no active requests when not in - // end game mode, which are [250, 300) and some other random blocks. + // wishlist only picks blocks with no active requests, which are + // [250, 300) and some other random blocks. // NB: when all other things are equal in the wishlist, pieces are // picked at random so this test -could- pass even if there's a bug. // So test several times to shake out any randomness static auto constexpr NumRuns = 1000; for (int run = 0; run < NumRuns; ++run) { - auto const [ranges, expected] = get_spans(300); + auto const [spans, expected] = get_spans(300); auto requested = tr_bitfield{ 300 }; - for (auto const& range : ranges) + for (auto const& [begin, end] : spans) { - requested.set_span(range.begin, range.end); + requested.set_span(begin, end); } EXPECT_EQ(50U + expected.count(), requested.count()); EXPECT_EQ(50U, requested.count(250, 300)); @@ -1260,7 +1119,58 @@ TEST_F(PeerMgrWishlistTest, gotRejectDecrementsActiveRequest) } } -TEST_F(PeerMgrWishlistTest, sentCancelDecrementsActiveRequest) +TEST_F(PeerMgrWishlistTest, gotRejectResortsPiece) +{ + auto const get_spans = [this](size_t n_wanted) + { + auto mediator = MockMediator{ *this }; + + // setup: two pieces, all missing + mediator.piece_count_ = 2; + mediator.block_span_[0] = { 0, 100 }; + mediator.block_span_[1] = { 100, 200 }; + + // peers has all pieces + mediator.piece_replication_[0] = 2; + mediator.piece_replication_[1] = 2; + + // and we want everything + mediator.client_wants_piece_.insert(0); + mediator.client_wants_piece_.insert(1); + + // allow the wishlist to build its cache + auto wishlist = Wishlist{ mediator }; + + // we have active requests to the first 50 blocks of each piece + sent_request_.emit(nullptr, nullptr, { 0, 50 }); + sent_request_.emit(nullptr, nullptr, { 100, 150 }); + + // a peer sent a "Reject" messages, which cancels active requests + auto const random_piece = tr_rand_int(2U); + got_reject_.emit(nullptr, nullptr, mediator.block_span_[random_piece].begin); + + return std::pair{ wishlist.next(n_wanted, PeerHasAllPieces), 1U - random_piece }; + }; + + // wishlist prioritises pieces that have fewer unrequested blocks. + // NB: when all other things are equal in the wishlist, pieces are + // picked at random so this test -could- pass even if there's a bug. + // So test several times to shake out any randomness + static auto constexpr NumRuns = 1000; + for (int run = 0; run < NumRuns; ++run) + { + auto const [spans, expected_piece] = get_spans(1); + auto requested = tr_bitfield{ 200 }; + for (auto const& [begin, end] : spans) + { + requested.set_span(begin, end); + } + EXPECT_EQ(1U, requested.count()); + EXPECT_TRUE(requested.test((expected_piece * 100U) + 50U)); + } +} + +TEST_F(PeerMgrWishlistTest, sentCancelResetsBlocks) { auto const get_spans = [this](size_t n_wanted) { @@ -1283,45 +1193,37 @@ TEST_F(PeerMgrWishlistTest, sentCancelDecrementsActiveRequest) mediator.client_wants_piece_.insert(i); } - // we have active requests to the first 250 blocks - for (tr_block_index_t i = 0; i < 250; ++i) - { - mediator.active_request_count_[i] = 1; - } - // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); - // a peer sent some "Reject" messages, which cancels active requests - auto cancelled_set = std::set{}; + // we have active requests to the first 250 blocks + sent_request_.emit(nullptr, nullptr, { 0, 250 }); + + // we sent some "Cancel" messages auto cancelled_bitfield = tr_bitfield{ 300 }; - for (tr_block_index_t i = 0, n = tr_rand_int(250U); i < n; ++i) - { - cancelled_set.insert(tr_rand_int(250U)); - } - for (auto const block : cancelled_set) + for (tr_block_index_t i = 0, n = tr_rand_int(250U); i <= n; ++i) { + auto const block = tr_rand_int(250U); cancelled_bitfield.set(block); sent_cancel_.emit(nullptr, nullptr, block); } - return std::pair{ wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests), std::move(cancelled_bitfield) }; + return std::pair{ wishlist.next(n_wanted, PeerHasAllPieces), std::move(cancelled_bitfield) }; }; - // wishlist only picks blocks with no active requests when not in - // end game mode, which are [250, 300) and some other random blocks. + // wishlist only picks blocks with no active requests, which are + // [250, 300) and some other random blocks. // NB: when all other things are equal in the wishlist, pieces are // picked at random so this test -could- pass even if there's a bug. // So test several times to shake out any randomness static auto constexpr NumRuns = 1000; for (int run = 0; run < NumRuns; ++run) { - auto const [ranges, expected] = get_spans(300); + auto const [spans, expected] = get_spans(300); auto requested = tr_bitfield{ 300 }; - for (auto const& range : ranges) + for (auto const& [begin, end] : spans) { - requested.set_span(range.begin, range.end); + requested.set_span(begin, end); } EXPECT_EQ(50U + expected.count(), requested.count()); EXPECT_EQ(50U, requested.count(250, 300)); @@ -1332,7 +1234,7 @@ TEST_F(PeerMgrWishlistTest, sentCancelDecrementsActiveRequest) } } -TEST_F(PeerMgrWishlistTest, sentRequestIncrementsActiveRequests) +TEST_F(PeerMgrWishlistTest, doesNotRequestBlockAfterBlockCompleted) { auto const get_spans = [this](size_t n_wanted) { @@ -1357,27 +1259,26 @@ TEST_F(PeerMgrWishlistTest, sentRequestIncrementsActiveRequests) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // we sent "Request" messages sent_request_.emit(nullptr, nullptr, { 0, 120 }); - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; - // wishlist only picks blocks with no active requests when not in - // end game mode, which are [0, 10) and [250, 300). + // wishlist only picks blocks with no active requests, which are + // [0, 120). // NB: when all other things are equal in the wishlist, pieces are // picked at random so this test -could- pass even if there's a bug. // So test several times to shake out any randomness static auto constexpr NumRuns = 1000; for (int run = 0; run < NumRuns; ++run) { - auto const ranges = get_spans(300); + auto const spans = get_spans(300); auto requested = tr_bitfield{ 300 }; - for (auto const& range : ranges) + for (auto const& [begin, end] : spans) { - requested.set_span(range.begin, range.end); + requested.set_span(begin, end); } EXPECT_EQ(180U, requested.count()); EXPECT_EQ(0U, requested.count(0, 120)); @@ -1409,7 +1310,6 @@ TEST_F(PeerMgrWishlistTest, doesNotRequestPieceAfterPieceCompleted) // allow the wishlist to build its cache, it should have all 3 pieces // at this point auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // we just completed piece 0 mediator.client_has_piece_.insert(0); @@ -1417,18 +1317,18 @@ TEST_F(PeerMgrWishlistTest, doesNotRequestPieceAfterPieceCompleted) // receiving a "piece_completed" signal removes the piece from the // wishlist's cache, its blocks should not be in the return set. - auto const spans = wishlist.next(10, PeerHasAllPieces, ClientHasNoActiveRequests); + auto const spans = wishlist.next(10, PeerHasAllPieces); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(10U, requested.count()); EXPECT_EQ(0U, requested.count(0, 100)); EXPECT_EQ(10U, requested.count(100, 300)); } -TEST_F(PeerMgrWishlistTest, settingPriorityRebuildsWishlist) +TEST_F(PeerMgrWishlistTest, settingPriorityResortsCandidates) { auto const get_spans = [this](size_t n_wanted) { @@ -1453,14 +1353,13 @@ TEST_F(PeerMgrWishlistTest, settingPriorityRebuildsWishlist) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // a file priority changed, the cache should be rebuilt. // let's say the file was in piece 1 mediator.piece_priority_[1] = TR_PRI_HIGH; priority_changed_.emit(nullptr, nullptr, 0U, TR_PRI_HIGH); - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; // wishlist should pick the high priority piece's blocks first. @@ -1473,9 +1372,9 @@ TEST_F(PeerMgrWishlistTest, settingPriorityRebuildsWishlist) { auto const spans = get_spans(10); auto requested = tr_bitfield{ 300 }; - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(10U, requested.count()); EXPECT_EQ(0U, requested.count(0, 100)); @@ -1484,7 +1383,7 @@ TEST_F(PeerMgrWishlistTest, settingPriorityRebuildsWishlist) } } -TEST_F(PeerMgrWishlistTest, settingSequentialDownloadRebuildsWishlist) +TEST_F(PeerMgrWishlistTest, settingSequentialDownloadResortsCandidates) { auto const get_spans = [this](size_t n_wanted) { @@ -1509,14 +1408,13 @@ TEST_F(PeerMgrWishlistTest, settingSequentialDownloadRebuildsWishlist) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // the sequential download setting was changed, // the cache should be rebuilt mediator.is_sequential_download_ = true; sequential_download_changed_.emit(nullptr, true); - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; // we should get pieces in sequential order when we ask for blocks, @@ -1529,9 +1427,9 @@ TEST_F(PeerMgrWishlistTest, settingSequentialDownloadRebuildsWishlist) { auto requested = tr_bitfield{ 300 }; auto spans = get_spans(150); - for (auto const& span : spans) + for (auto const& [begin, end] : spans) { - requested.set_span(span.begin, span.end); + requested.set_span(begin, end); } EXPECT_EQ(150U, requested.count()); EXPECT_EQ(100U, requested.count(0, 100)); @@ -1580,7 +1478,6 @@ TEST_F(PeerMgrWishlistTest, setFileWantedUpdatesCandidateListAdd) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // now we want the file that consists of piece 2 and piece 3 also mediator.client_wants_piece_.insert(2); @@ -1589,7 +1486,7 @@ TEST_F(PeerMgrWishlistTest, setFileWantedUpdatesCandidateListAdd) // a candidate should be inserted into the wishlist for // piece 2 and piece 3 - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; // We should request all 4 pieces here. @@ -1640,7 +1537,6 @@ TEST_F(PeerMgrWishlistTest, setFileWantedUpdatesCandidateListRemove) // allow the wishlist to build its cache auto wishlist = Wishlist{ mediator }; - (void)wishlist.next(1, PeerHasAllPieces, ClientHasNoActiveRequests); // we no longer want the file that consists of piece 2 and piece 3 mediator.client_wants_piece_.erase(2); @@ -1648,7 +1544,7 @@ TEST_F(PeerMgrWishlistTest, setFileWantedUpdatesCandidateListRemove) files_wanted_changed_.emit(nullptr, nullptr, 0, true); // the candidate objects for piece 2 and piece 3 should be removed - return wishlist.next(n_wanted, PeerHasAllPieces, ClientHasNoActiveRequests); + return wishlist.next(n_wanted, PeerHasAllPieces); }; // We should request only the first 2 pieces here.