From f0d7ec395414d7d2f53639bbb05852778a307264 Mon Sep 17 00:00:00 2001 From: Steven Siloti Date: Thu, 7 Jul 2016 05:34:52 -0700 Subject: [PATCH] refactor optimistic unchoke extension API (#888) refactor optimistic unchoke extension API. The new interface is more efficient to implement and only allows extensions to affect the unchoke order in cases where the last unchoked timestamp is equal. This is safer because it prevents extensions from overriding the round-robin ordering of optimistic unchokes. --- include/libtorrent/extensions.hpp | 21 +++--- src/session_impl.cpp | 120 +++++++++++++++++++----------- 2 files changed, 86 insertions(+), 55 deletions(-) diff --git a/include/libtorrent/extensions.hpp b/include/libtorrent/extensions.hpp index 2fcee8f1b..632c67663 100644 --- a/include/libtorrent/extensions.hpp +++ b/include/libtorrent/extensions.hpp @@ -173,6 +173,7 @@ POSSIBILITY OF SUCH DAMAGE. #include "libtorrent/sha1_hash.hpp" // for sha1_hash #include "libtorrent/error_code.hpp" #include "libtorrent/session_handle.hpp" +#include "libtorrent/peer_connection_handle.hpp" #include "libtorrent/aux_/array_view.hpp" namespace libtorrent @@ -185,7 +186,6 @@ namespace libtorrent class alert; struct torrent_plugin; struct add_torrent_params; - struct peer_connection_handle; struct torrent_handle; // this is the base class for a session plugin. One primary feature @@ -262,15 +262,16 @@ namespace libtorrent // ``tick_feature`` in the return value from implemented_features(). virtual void on_tick() {} - // called when choosing peers to optimistically unchoke. peer's will be - // unchoked in the order they appear in the given vector. if - // the plugin returns true then the ordering provided will be used and no - // other plugin will be allowed to change it. If your plugin expects this - // to be called, make sure to include the flag - // ``optimistic_unchoke_feature`` in the return value from - // implemented_features(). - virtual bool on_optimistic_unchoke(std::vector& /* peers */) - { return false; } + // called when choosing peers to optimistically unchoke. The return value + // indicates the peer's priority for unchoking. Lower return values + // correspond to higher priority. Priorities above 2^63-1 are reserved. + // If your plugin has no priority to assign a peer it should return 2^64-1. + // If your plugin expects this to be called, make sure to include the flag + // ``optimistic_unchoke_feature`` in the return value from implemented_features(). + // If multiple plugins implement this function the lowest return value + // (i.e. the highest priority) is used. + virtual uint64_t get_unchoke_priority(peer_connection_handle /* peer */) + { return std::numeric_limits::max(); } // called when saving settings state virtual void save_state(entry&) const {} diff --git a/src/session_impl.cpp b/src/session_impl.cpp index c411f4ccf..df98313bf 100644 --- a/src/session_impl.cpp +++ b/src/session_impl.cpp @@ -3732,12 +3732,69 @@ namespace aux { } namespace { - bool last_optimistic_unchoke_cmp(torrent_peer const* const l - , torrent_peer const* const r) + uint64_t const priority_undetermined = std::numeric_limits::max() - 1; + + struct opt_unchoke_candidate { - return l->last_optimistically_unchoked - < r->last_optimistically_unchoked; - } + opt_unchoke_candidate(boost::shared_ptr const* tp) + : peer(tp) + {} + + boost::shared_ptr const* peer; +#ifndef TORRENT_DISABLE_EXTENSIONS + // this is mutable because comparison functors passed to std::partial_sort + // are not supposed to modify the elements they are sorting. Here the mutation + // being applied is idempotent so it should not pose a problem. + mutable uint64_t ext_priority = priority_undetermined; +#endif + }; + + struct last_optimistic_unchoke_cmp + { +#ifndef TORRENT_DISABLE_EXTENSIONS + last_optimistic_unchoke_cmp(std::vector>& ps) + : plugins(ps) + {} + + std::vector>& plugins; +#endif + + uint64_t get_ext_priority(opt_unchoke_candidate const& peer) const + { +#ifndef TORRENT_DISABLE_EXTENSIONS + if (peer.ext_priority == priority_undetermined) + { + peer.ext_priority = std::numeric_limits::max(); + for (auto& e : plugins) + { + uint64_t const priority = e->get_unchoke_priority(peer_connection_handle(*peer.peer)); + peer.ext_priority = (std::min)(priority, peer.ext_priority); + } + } + return peer.ext_priority; +#else + TORRENT_UNUSED(peer); + return std::numeric_limits::max(); +#endif + } + + bool operator()(opt_unchoke_candidate const& l + , opt_unchoke_candidate const& r) const + { + torrent_peer* pil = (*l.peer)->peer_info_struct(); + torrent_peer* pir = (*r.peer)->peer_info_struct(); + if (pil->last_optimistically_unchoked + != pir->last_optimistically_unchoked) + { + return pil->last_optimistically_unchoked + < pir->last_optimistically_unchoked; + } + else + { + return get_ext_priority(l) < get_ext_priority(r); + } + } + }; } void session_impl::recalculate_optimistic_unchoke_slots() @@ -3747,7 +3804,7 @@ namespace aux { TORRENT_ASSERT(is_single_thread()); if (m_stats_counters[counters::num_unchoke_slots] == 0) return; - std::vector opt_unchoke; + std::vector opt_unchoke; // collect the currently optimistically unchoked peers here, so we can // choke them when we've found new optimistic unchoke candidates. @@ -3760,10 +3817,9 @@ namespace aux { // collect the n best candidates. maybe just a queue of peers would make // even more sense, just pick the next peer in the queue for unchoking. It // would be O(1). - for (connection_map::iterator i = m_connections.begin() - , end(m_connections.end()); i != end; ++i) + for (auto& i : m_connections) { - peer_connection* p = i->get(); + peer_connection* p = i.get(); TORRENT_ASSERT(p); torrent_peer* pi = p->peer_info_struct(); if (!pi) continue; @@ -3789,7 +3845,7 @@ namespace aux { && !p->ignore_unchoke_slots() && t->valid_metadata()) { - opt_unchoke.push_back(pi); + opt_unchoke.emplace_back(&i); } } @@ -3805,46 +3861,22 @@ namespace aux { // find the n best optimistic unchoke candidates std::partial_sort(opt_unchoke.begin() , opt_unchoke.begin() + num_opt_unchoke - , opt_unchoke.end(), &last_optimistic_unchoke_cmp); - + , opt_unchoke.end() #ifndef TORRENT_DISABLE_EXTENSIONS - auto const& plugins = m_ses_extensions[plugins_optimistic_unchoke_idx]; - if (plugins.size() > 0) - { - // if there is an extension that wants to reorder the optimistic - // unchoke peers, first convert the vector into one containing - // peer_connection_handles, since that's the exported API - std::vector peers; - peers.reserve(opt_unchoke.size()); - for (std::vector::iterator i = opt_unchoke.begin() - , end(opt_unchoke.end()); i != end; ++i) - { - peers.push_back(peer_connection_handle(static_cast((*i)->connection)->self())); - } - for (auto& e : plugins) - { - if (e->on_optimistic_unchoke(peers)) - break; - } - // then convert back to the internal torrent_peer pointers - opt_unchoke.clear(); - for (std::vector::iterator i = peers.begin() - , end(peers.end()); i != end; ++i) - { - opt_unchoke.push_back(i->native_handle()->peer_info_struct()); - } - } + , last_optimistic_unchoke_cmp(m_ses_extensions[plugins_optimistic_unchoke_idx]) +#else + , last_optimistic_unchoke_cmp() #endif + ); // unchoke the first num_opt_unchoke peers in the candidate set // and make sure that the others are choked - std::vector::iterator opt_unchoke_end = opt_unchoke.begin() + auto opt_unchoke_end = opt_unchoke.begin() + num_opt_unchoke; - for (std::vector::iterator i = opt_unchoke.begin(); - i != opt_unchoke_end; ++i) + for (auto i = opt_unchoke.begin(); i != opt_unchoke_end; ++i) { - torrent_peer* pi = *i; + torrent_peer* pi = (*i->peer)->peer_info_struct(); peer_connection* p = static_cast(pi->connection); if (pi->optimistically_unchoked) { @@ -3882,10 +3914,8 @@ namespace aux { } // now, choke all the previous optimistically unchoked peers - for (std::vector::iterator i = prev_opt_unchoke.begin() - , end(prev_opt_unchoke.end()); i != end; ++i) + for (torrent_peer* pi : prev_opt_unchoke) { - torrent_peer* pi = *i; TORRENT_ASSERT(pi->optimistically_unchoked); peer_connection* p = static_cast(pi->connection); boost::shared_ptr t = p->associated_torrent().lock();