From 4d927f4029fa489aa9a08454edc5a97b58b0c9f0 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Thu, 16 Jun 2016 08:24:41 -0400 Subject: [PATCH] modernize disk_buffer_holder to be move-only (#824) modernize disk_buffer_holder to be move-only --- include/libtorrent/aux_/session_impl.hpp | 4 +- include/libtorrent/bt_peer_connection.hpp | 2 +- include/libtorrent/disk_buffer_holder.hpp | 34 +++++++-------- include/libtorrent/disk_interface.hpp | 2 +- include/libtorrent/disk_io_thread.hpp | 6 +-- include/libtorrent/peer_connection.hpp | 4 +- include/libtorrent/web_connection_base.hpp | 2 +- src/bt_peer_connection.cpp | 4 +- src/disk_buffer_holder.cpp | 49 ++++++++++++++-------- src/disk_io_thread.cpp | 40 ++++++++++-------- src/peer_connection.cpp | 16 +++---- src/session_impl.cpp | 4 +- src/torrent.cpp | 9 ++-- 13 files changed, 97 insertions(+), 79 deletions(-) diff --git a/include/libtorrent/aux_/session_impl.hpp b/include/libtorrent/aux_/session_impl.hpp index cd4ffde28..36dc6152c 100644 --- a/include/libtorrent/aux_/session_impl.hpp +++ b/include/libtorrent/aux_/session_impl.hpp @@ -576,8 +576,8 @@ namespace libtorrent // implements buffer_allocator_interface void free_disk_buffer(char* buf) override; - char* allocate_disk_buffer(char const* category) override; - char* allocate_disk_buffer(bool& exceeded + disk_buffer_holder allocate_disk_buffer(char const* category) override; + disk_buffer_holder allocate_disk_buffer(bool& exceeded , boost::shared_ptr o , char const* category) override; void reclaim_block(block_cache_reference ref) override; diff --git a/include/libtorrent/bt_peer_connection.hpp b/include/libtorrent/bt_peer_connection.hpp index ab6e12c08..24aa2e331 100644 --- a/include/libtorrent/bt_peer_connection.hpp +++ b/include/libtorrent/bt_peer_connection.hpp @@ -219,7 +219,7 @@ namespace libtorrent void write_bitfield() override; void write_have(int index) override; void write_dont_have(int index) override; - void write_piece(peer_request const& r, disk_buffer_holder& buffer) override; + void write_piece(peer_request const& r, disk_buffer_holder buffer) override; void write_keepalive() override; void write_handshake(); #ifndef TORRENT_DISABLE_EXTENSIONS diff --git a/include/libtorrent/disk_buffer_holder.hpp b/include/libtorrent/disk_buffer_holder.hpp index d51670862..fcd9bd3c0 100644 --- a/include/libtorrent/disk_buffer_holder.hpp +++ b/include/libtorrent/disk_buffer_holder.hpp @@ -49,13 +49,14 @@ namespace libtorrent { struct disk_io_thread; struct disk_observer; + struct disk_buffer_holder; struct TORRENT_EXTRA_EXPORT buffer_allocator_interface { - virtual char* allocate_disk_buffer(char const* category) = 0; virtual void free_disk_buffer(char* b) = 0; virtual void reclaim_block(block_cache_reference ref) = 0; - virtual char* allocate_disk_buffer(bool& exceeded + virtual disk_buffer_holder allocate_disk_buffer(char const* category) = 0; + virtual disk_buffer_holder allocate_disk_buffer(bool& exceeded , boost::shared_ptr o , char const* category) = 0; protected: @@ -66,15 +67,19 @@ namespace libtorrent // when it's destructed, unless it's released. ``release`` returns the disk // buffer and transfers ownership and responsibility to free it to the caller. // - // A disk buffer is freed by passing it to ``session_impl::free_disk_buffer()``. - // - // ``get()`` returns the pointer without transferring responsibility. If - // this buffer has been released, ``buffer()`` will return 0. + // ``get()`` returns the pointer without transferring ownership. If + // this buffer has been released, ``get()`` will return nullptr. struct TORRENT_EXPORT disk_buffer_holder { // internal disk_buffer_holder(buffer_allocator_interface& alloc, char* buf); + disk_buffer_holder& operator=(disk_buffer_holder&&); + disk_buffer_holder(disk_buffer_holder&&); + + disk_buffer_holder& operator=(disk_buffer_holder const&) = delete; + disk_buffer_holder(disk_buffer_holder const&) = delete; + // construct a buffer holder that will free the held buffer // using a disk buffer pool directly (there's only one // disk_buffer_pool per session) @@ -82,7 +87,7 @@ namespace libtorrent // frees any unreleased disk buffer held by this object ~disk_buffer_holder(); - + // return the held disk buffer and clear it from the // holder. The responsibility to free it is passed on // to the caller @@ -100,25 +105,20 @@ namespace libtorrent // swap pointers of two disk buffer holders. void swap(disk_buffer_holder& h) { - TORRENT_ASSERT(&h.m_allocator == &m_allocator); + TORRENT_ASSERT(h.m_allocator == m_allocator); std::swap(h.m_buf, m_buf); std::swap(h.m_ref, m_ref); } block_cache_reference ref() const { return m_ref; } - typedef char* (disk_buffer_holder::*unspecified_bool_type)(); - - // internal - operator unspecified_bool_type() const - { return m_buf == 0? 0: &disk_buffer_holder::release; } + // implicitly convertible to true if the object is currently holding a + // buffer + explicit operator bool() const { return m_buf != nullptr; } private: - // non-copyable - disk_buffer_holder& operator=(disk_buffer_holder const&); - disk_buffer_holder(disk_buffer_holder const*); - buffer_allocator_interface& m_allocator; + buffer_allocator_interface* m_allocator; char* m_buf; block_cache_reference m_ref; }; diff --git a/include/libtorrent/disk_interface.hpp b/include/libtorrent/disk_interface.hpp index df6e4aa3d..e751549b6 100644 --- a/include/libtorrent/disk_interface.hpp +++ b/include/libtorrent/disk_interface.hpp @@ -56,7 +56,7 @@ namespace libtorrent , boost::function const& handler, void* requester , int flags = 0) = 0; virtual void async_write(piece_manager* storage, peer_request const& r - , disk_buffer_holder& buffer + , disk_buffer_holder buffer , boost::function const& handler , int flags = 0) = 0; virtual void async_hash(piece_manager* storage, int piece, int flags diff --git a/include/libtorrent/disk_io_thread.hpp b/include/libtorrent/disk_io_thread.hpp index 84ab78e7c..09773b738 100644 --- a/include/libtorrent/disk_io_thread.hpp +++ b/include/libtorrent/disk_io_thread.hpp @@ -303,7 +303,7 @@ namespace libtorrent , boost::function const& handler, void* requester , int flags = 0) override; void async_write(piece_manager* storage, peer_request const& r - , disk_buffer_holder& buffer + , disk_buffer_holder buffer , boost::function const& handler , int flags = 0) override; void async_hash(piece_manager* storage, int piece, int flags @@ -354,14 +354,14 @@ namespace libtorrent // implements buffer_allocator_interface void reclaim_block(block_cache_reference ref) override; void free_disk_buffer(char* buf) override { m_disk_cache.free_buffer(buf); } - char* allocate_disk_buffer(char const* category) override + disk_buffer_holder allocate_disk_buffer(char const* category) override { bool exceed = false; return allocate_disk_buffer(exceed, boost::shared_ptr(), category); } void trigger_cache_trim(); - char* allocate_disk_buffer(bool& exceeded, boost::shared_ptr o + disk_buffer_holder allocate_disk_buffer(bool& exceeded, boost::shared_ptr o , char const* category) override; bool exceeded_cache_use() const diff --git a/include/libtorrent/peer_connection.hpp b/include/libtorrent/peer_connection.hpp index dc244ba38..680da8406 100644 --- a/include/libtorrent/peer_connection.hpp +++ b/include/libtorrent/peer_connection.hpp @@ -574,7 +574,7 @@ namespace libtorrent void incoming_dont_have(int piece_index); void incoming_bitfield(bitfield const& bits); void incoming_request(peer_request const& r); - void incoming_piece(peer_request const& p, disk_buffer_holder& data); + void incoming_piece(peer_request const& p, disk_buffer_holder data); void incoming_piece(peer_request const& p, char const* data); void incoming_piece_fragment(int bytes); void start_receive_piece(peer_request const& r); @@ -730,7 +730,7 @@ namespace libtorrent virtual void write_have(int index) = 0; virtual void write_dont_have(int index) = 0; virtual void write_keepalive() = 0; - virtual void write_piece(peer_request const& r, disk_buffer_holder& buffer) = 0; + virtual void write_piece(peer_request const& r, disk_buffer_holder buffer) = 0; virtual void write_suggest(int piece) = 0; virtual void write_bitfield() = 0; diff --git a/include/libtorrent/web_connection_base.hpp b/include/libtorrent/web_connection_base.hpp index ef43f8016..ac73e97af 100644 --- a/include/libtorrent/web_connection_base.hpp +++ b/include/libtorrent/web_connection_base.hpp @@ -108,7 +108,7 @@ namespace libtorrent void write_cancel(peer_request const&) {} void write_have(int) {} void write_dont_have(int) {} - void write_piece(peer_request const&, disk_buffer_holder&) + void write_piece(peer_request const&, disk_buffer_holder) { TORRENT_ASSERT_FAIL(); } void write_keepalive() {} void on_connected(); diff --git a/src/bt_peer_connection.cpp b/src/bt_peer_connection.cpp index e9a64d2fe..61b739338 100644 --- a/src/bt_peer_connection.cpp +++ b/src/bt_peer_connection.cpp @@ -2472,7 +2472,7 @@ namespace libtorrent } // anonymous namespace - void bt_peer_connection::write_piece(peer_request const& r, disk_buffer_holder& buffer) + void bt_peer_connection::write_piece(peer_request const& r, disk_buffer_holder buffer) { INVARIANT_CHECK; @@ -2532,7 +2532,7 @@ namespace libtorrent send_buffer(msg, 13); } - if (buffer.ref().storage == 0) + if (buffer.ref().storage == nullptr) { append_send_buffer(buffer.get(), r.length , &buffer_free_disk_buf, &m_allocator); diff --git a/src/disk_buffer_holder.cpp b/src/disk_buffer_holder.cpp index 497e22897..84dd0ea8a 100644 --- a/src/disk_buffer_holder.cpp +++ b/src/disk_buffer_holder.cpp @@ -38,33 +38,48 @@ namespace libtorrent { disk_buffer_holder::disk_buffer_holder(buffer_allocator_interface& alloc, char* buf) - : m_allocator(alloc), m_buf(buf) + : m_allocator(&alloc), m_buf(buf) { - m_ref.storage = 0; + m_ref.storage = nullptr; m_ref.piece = -1; m_ref.block = -1; } - disk_buffer_holder::disk_buffer_holder(buffer_allocator_interface& alloc, disk_io_job const& j) - : m_allocator(alloc), m_buf(j.buffer.disk_block), m_ref(j.d.io.ref) + disk_buffer_holder& disk_buffer_holder::operator=(disk_buffer_holder&& h) { - TORRENT_ASSERT(m_ref.storage == 0 || m_ref.piece >= 0); - TORRENT_ASSERT(m_ref.storage == 0 || m_ref.block >= 0); - TORRENT_ASSERT(m_ref.storage == 0 || m_ref.piece < static_cast(m_ref.storage)->files()->num_pieces()); - TORRENT_ASSERT(m_ref.storage == 0 || m_ref.block <= static_cast(m_ref.storage)->files()->piece_length() / 0x4000); + disk_buffer_holder(std::move(h)).swap(*this); + return *this; + } + + disk_buffer_holder::disk_buffer_holder(disk_buffer_holder&& h) + : m_allocator(h.m_allocator), m_buf(h.m_buf), m_ref(h.m_ref) + { + // we own this buffer now + h.release(); + } + + disk_buffer_holder::disk_buffer_holder(buffer_allocator_interface& alloc, disk_io_job const& j) + : m_allocator(&alloc), m_buf(j.buffer.disk_block), m_ref(j.d.io.ref) + { + TORRENT_ASSERT(m_ref.storage == nullptr || m_ref.piece >= 0); + TORRENT_ASSERT(m_ref.storage == nullptr || m_ref.block >= 0); + TORRENT_ASSERT(m_ref.storage == nullptr + || m_ref.piece < static_cast(m_ref.storage)->files()->num_pieces()); + TORRENT_ASSERT(m_ref.storage == nullptr + || m_ref.block <= static_cast(m_ref.storage)->files()->piece_length() / 0x4000); TORRENT_ASSERT(j.action != disk_io_job::rename_file); TORRENT_ASSERT(j.action != disk_io_job::move_storage); } void disk_buffer_holder::reset(disk_io_job const& j) { - if (m_ref.storage) m_allocator.reclaim_block(m_ref); - else if (m_buf) m_allocator.free_disk_buffer(m_buf); + if (m_ref.storage) m_allocator->reclaim_block(m_ref); + else if (m_buf) m_allocator->free_disk_buffer(m_buf); m_buf = j.buffer.disk_block; m_ref = j.d.io.ref; TORRENT_ASSERT(m_ref.piece >= 0); - TORRENT_ASSERT(m_ref.storage != 0); + TORRENT_ASSERT(m_ref.storage != nullptr); TORRENT_ASSERT(m_ref.block >= 0); TORRENT_ASSERT(m_ref.piece < static_cast(m_ref.storage)->files()->num_pieces()); TORRENT_ASSERT(m_ref.block <= static_cast(m_ref.storage)->files()->piece_length() / 0x4000); @@ -72,19 +87,19 @@ namespace libtorrent TORRENT_ASSERT(j.action != disk_io_job::move_storage); } - void disk_buffer_holder::reset(char* buf) + void disk_buffer_holder::reset(char* const buf) { - if (m_ref.storage) m_allocator.reclaim_block(m_ref); - else if (m_buf) m_allocator.free_disk_buffer(m_buf); + if (m_ref.storage) m_allocator->reclaim_block(m_ref); + else if (m_buf) m_allocator->free_disk_buffer(m_buf); m_buf = buf; - m_ref.storage = 0; + m_ref.storage = nullptr; } char* disk_buffer_holder::release() { char* ret = m_buf; - m_buf = 0; - m_ref.storage = 0; + m_buf = nullptr; + m_ref.storage = nullptr; return ret; } diff --git a/src/disk_io_thread.cpp b/src/disk_io_thread.cpp index 7f524f425..620bd4495 100644 --- a/src/disk_io_thread.cpp +++ b/src/disk_io_thread.cpp @@ -1590,7 +1590,7 @@ namespace libtorrent } void disk_io_thread::async_write(piece_manager* storage, peer_request const& r - , disk_buffer_holder& buffer + , disk_buffer_holder buffer , boost::function const& handler , int flags) { @@ -1672,24 +1672,28 @@ namespace libtorrent // if the buffer was successfully added to the cache // our holder should no longer own it - if (dpe) buffer.release(); - - if (dpe && dpe->outstanding_flush == 0) + if (dpe) { - dpe->outstanding_flush = 1; - l.unlock(); + buffer.release(); - // the block and write job were successfully inserted - // into the cache. Now, see if we should trigger a flush - j = allocate_job(disk_io_job::flush_hashed); - j->storage = storage->shared_from_this(); - j->piece = r.piece; - j->flags = flags; - add_job(j); + if (dpe->outstanding_flush == 0) + { + dpe->outstanding_flush = 1; + l.unlock(); + + // the block and write job were successfully inserted + // into the cache. Now, see if we should trigger a flush + j = allocate_job(disk_io_job::flush_hashed); + j->storage = storage->shared_from_this(); + j->piece = r.piece; + j->flags = flags; + add_job(j); + } + + // if we added the block (regardless of whether we also + // issued a flush job or not), we're done. + return; } - // if we added the block (regardless of whether we also - // issued a flush job or not), we're done. - if (dpe) return; l.unlock(); add_job(j); @@ -3330,12 +3334,12 @@ namespace libtorrent submit_jobs(); } - char* disk_io_thread::allocate_disk_buffer(bool& exceeded + disk_buffer_holder disk_io_thread::allocate_disk_buffer(bool& exceeded , boost::shared_ptr o , char const* category) { char* ret = m_disk_cache.allocate_buffer(exceeded, o, category); - return ret; + return disk_buffer_holder(*this, ret); } void disk_io_thread::add_completed_job(disk_io_job* j) diff --git a/src/peer_connection.cpp b/src/peer_connection.cpp index cdd4b5bc9..ce7016b55 100644 --- a/src/peer_connection.cpp +++ b/src/peer_connection.cpp @@ -2611,9 +2611,10 @@ namespace libtorrent { TORRENT_ASSERT(is_single_thread()); bool exceeded = false; - char* buffer = m_allocator.allocate_disk_buffer(exceeded, self(), "receive buffer"); + disk_buffer_holder buffer + = m_allocator.allocate_disk_buffer(exceeded, self(), "receive buffer"); - if (buffer == 0) + if (!buffer) { disconnect(errors::no_memory, op_alloc_recvbuf); return; @@ -2635,12 +2636,11 @@ namespace libtorrent #endif } - disk_buffer_holder holder(m_allocator, buffer); - std::memcpy(buffer, data, p.length); - incoming_piece(p, holder); + std::memcpy(buffer.get(), data, p.length); + incoming_piece(p, std::move(buffer)); } - void peer_connection::incoming_piece(peer_request const& p, disk_buffer_holder& data) + void peer_connection::incoming_piece(peer_request const& p, disk_buffer_holder data) { TORRENT_ASSERT(is_single_thread()); INVARIANT_CHECK; @@ -2849,7 +2849,7 @@ namespace libtorrent return; } t->inc_refcount("async_write"); - m_disk_thread.async_write(&t->storage(), p, data + m_disk_thread.async_write(&t->storage(), p, std::move(data) , std::bind(&peer_connection::on_disk_write_complete , self(), _1, p, t)); @@ -5359,7 +5359,7 @@ namespace libtorrent { t->add_suggest_piece(r.piece); } - write_piece(r, buffer); + write_piece(r, std::move(buffer)); } void peer_connection::assign_bandwidth(int channel, int amount) diff --git a/src/session_impl.cpp b/src/session_impl.cpp index fd91742ae..3b076bd83 100644 --- a/src/session_impl.cpp +++ b/src/session_impl.cpp @@ -6675,7 +6675,7 @@ namespace aux { m_disk_thread.reclaim_block(ref); } - char* session_impl::allocate_disk_buffer(char const* category) + disk_buffer_holder session_impl::allocate_disk_buffer(char const* category) { return m_disk_thread.allocate_disk_buffer(category); } @@ -6685,7 +6685,7 @@ namespace aux { m_disk_thread.free_disk_buffer(buf); } - char* session_impl::allocate_disk_buffer(bool& exceeded + disk_buffer_holder session_impl::allocate_disk_buffer(bool& exceeded , boost::shared_ptr o , char const* category) { diff --git a/src/torrent.cpp b/src/torrent.cpp index dc09c4527..aae2a4c0c 100644 --- a/src/torrent.cpp +++ b/src/torrent.cpp @@ -1299,15 +1299,14 @@ namespace libtorrent continue; p.length = (std::min)(piece_size - p.start, int(block_size())); - char* buffer = m_ses.allocate_disk_buffer("add piece"); + disk_buffer_holder buffer = m_ses.allocate_disk_buffer("add piece"); // out of memory - if (buffer == 0) + if (!buffer) { picker().dec_refcount(piece, 0); return; } - disk_buffer_holder holder(m_ses, buffer); - std::memcpy(buffer, data + p.start, p.length); + std::memcpy(buffer.get(), data + p.start, p.length); if (!need_loaded()) { @@ -1316,7 +1315,7 @@ namespace libtorrent return; } inc_refcount("add_piece"); - m_ses.disk_thread().async_write(&storage(), p, holder + m_ses.disk_thread().async_write(&storage(), p, std::move(buffer) , std::bind(&torrent::on_disk_write_complete , shared_from_this(), _1, p)); piece_block block(piece, i);