From 5602a24b221ed3dbc3dcd8c2a39578fdd9ebd284 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Thu, 21 Apr 2016 08:46:43 -0400 Subject: [PATCH] Fix basic_streambuf: prepare() is rewritten to operate more simply; the state of the container is always consistent even between internal operations. --- include/beast/basic_streambuf.hpp | 3 + include/beast/impl/basic_streambuf.ipp | 162 +++++++------- test/basic_streambuf.cpp | 292 +++++++++++++++++++++++++ test/streambuf.cpp | 280 ------------------------ 4 files changed, 378 insertions(+), 359 deletions(-) diff --git a/include/beast/basic_streambuf.hpp b/include/beast/basic_streambuf.hpp index 9a65ce08d..96003323f 100644 --- a/include/beast/basic_streambuf.hpp +++ b/include/beast/basic_streambuf.hpp @@ -41,6 +41,9 @@ public: template rebind_alloc; private: + // Storage for the list of buffers representing the input + // and output sequences. The allocation for each element + // contains `element` followed by raw storage bytes. class element; using alloc_traits = std::allocator_traits; diff --git a/include/beast/impl/basic_streambuf.ipp b/include/beast/impl/basic_streambuf.ipp index 46ec6ab28..edf88eec5 100644 --- a/include/beast/impl/basic_streambuf.ipp +++ b/include/beast/impl/basic_streambuf.ipp @@ -19,65 +19,65 @@ namespace beast { /* These diagrams illustrate the layout and state variables. - Input and output contained entirely in one element: +1 Input and output contained entirely in one element: 0 out_ |<-------------+------------------------------------------->| - in_pos_ out_pos_ out_end_ + in_pos_ out_pos_ out_end_ - Output contained in first and second elements: +2 Output contained in first and second elements: out_ |<------+----------+------->| |<----------+-------------->| - in_pos_ out_pos_ out_end_ + in_pos_ out_pos_ out_end_ - Output contained in the second element: +3 Output contained in the second element: out_ |<------------+------------>| |<----+-------------------->| in_pos_ out_pos_ out_end_ - Output contained in second and third elements: +4 Output contained in second and third elements: out_ |<-----+-------->| |<-------+------>| |<--------------->| - in_pos_ out_pos_ out_end_ + in_pos_ out_pos_ out_end_ - Input sequence is empty: +5 Input sequence is empty: out_ |<------+------------------>| |<-----------+------------->| - out_pos_ out_end_ - in_pos_ + out_pos_ out_end_ + in_pos_ - Output sequence is empty: +6 Output sequence is empty: out_ |<------+------------------>| |<------+------------------>| - in_pos_ out_pos_ - out_end_ + in_pos_ out_pos_ + out_end_ - The end of output can point to the end of an element. +7 The end of output can point to the end of an element. But out_pos_ should never point to the end: out_ |<------+------------------>| |<------+------------------>| - in_pos_ out_pos_ out_end_ + in_pos_ out_pos_ out_end_ - When the input sequence entirely fills the last element and +8 When the input sequence entirely fills the last element and the output sequence is empty, out_ will point to the end of the list of buffers, and out_pos_ and out_end_ will be 0: |<------+------------------>| out_ == list_.end() - in_pos_ out_pos_ == 0 + in_pos_ out_pos_ == 0 out_end_ == 0 */ @@ -478,74 +478,77 @@ auto basic_streambuf::prepare(size_type n) -> mutable_buffers_type { - iterator pos = out_; - if(pos != list_.end()) + list_type reuse; + if(out_ != list_.end()) { - auto const avail = pos->size() - out_pos_; + if(out_ != list_.iterator_to(list_.back())) + { + out_end_ = out_->size(); + reuse.splice(reuse.end(), list_, + std::next(out_), list_.end()); + debug_check(); + } + auto const avail = out_->size() - out_pos_; if(n > avail) { + out_end_ = out_->size(); n -= avail; - out_end_ = pos->size(); - while(++pos != list_.end()) - { - if(n < pos->size()) - { - out_end_ = n; - n = 0; - ++pos; - break; - } - n -= pos->size(); - } } else { - ++pos; out_end_ = out_pos_ + n; n = 0; } + debug_check(); } - - if(n > 0) + while(n > 0 && ! reuse.empty()) { - assert(pos == list_.end()); - for(;;) + auto& e = reuse.front(); + reuse.erase(reuse.iterator_to(e)); + list_.push_back(e); + if(n > e.size()) { - auto const size = std::max(alloc_size_, n); - auto& e = *reinterpret_cast( - alloc_traits::allocate(this->member(), - size + sizeof(element))); - alloc_traits::construct(this->member(), &e, size); - list_.push_back(e); - if(out_ == list_.end()) - { - out_ = list_.iterator_to(e); - debug_check(); - } - if(n <= size) - { - out_end_ = n; - debug_check(); - break; - } - n -= size; - debug_check(); + out_end_ = e.size(); + n -= e.size(); } - } - else - { - while(pos != list_.end()) + else { - auto& e = *pos++; - list_.erase(list_.iterator_to(e)); - auto const len = e.size() + sizeof(e); - alloc_traits::destroy(this->member(), &e); - alloc_traits::deallocate(this->member(), - reinterpret_cast(&e), len); + out_end_ = n; + n = 0; } debug_check(); } - + while(n > 0) + { + auto const size = std::max(alloc_size_, n); + auto& e = *reinterpret_cast( + alloc_traits::allocate(this->member(), + sizeof(element) + size)); + alloc_traits::construct(this->member(), &e, size); + list_.push_back(e); + if(out_ == list_.end()) + out_ = list_.iterator_to(e); + if(n > e.size()) + { + out_end_ = e.size(); + n -= e.size(); + } + else + { + out_end_ = n; + n = 0; + } + debug_check(); + } + for(auto it = reuse.begin(); it != reuse.end();) + { + auto& e = *it++; + reuse.erase(list_.iterator_to(e)); + auto const len = e.size() + sizeof(e); + alloc_traits::destroy(this->member(), &e); + alloc_traits::deallocate(this->member(), + reinterpret_cast(&e), len); + } return mutable_buffers_type(*this); } @@ -557,8 +560,9 @@ basic_streambuf::commit(size_type n) return; if(out_ == list_.end()) return; - auto const last = std::prev(list_.end()); - while(out_ != last) + auto const back = + list_.iterator_to(list_.back()); + while(out_ != back) { auto const avail = out_->size() - out_pos_; @@ -603,12 +607,11 @@ basic_streambuf::consume(size_type n) if(list_.empty()) return; - auto pos = list_.begin(); for(;;) { - if(pos != out_) + if(list_.begin() != out_) { - auto const avail = pos->size() - in_pos_; + auto const avail = list_.front().size() - in_pos_; if(n < avail) { in_size_ -= n; @@ -619,14 +622,13 @@ basic_streambuf::consume(size_type n) n -= avail; in_size_ -= avail; in_pos_ = 0; - debug_check(); - - element& e = *pos++; + auto& e = list_.front(); list_.erase(list_.iterator_to(e)); auto const len = e.size() + sizeof(e); alloc_traits::destroy(this->member(), &e); alloc_traits::deallocate(this->member(), reinterpret_cast(&e), len); + debug_check(); } else { @@ -638,15 +640,15 @@ basic_streambuf::consume(size_type n) } else { - in_size_ -= avail; - if(out_pos_ != out_end_|| - out_ != list_.iterator_to(list_.back())) + in_size_ = 0; + if(out_ != list_.iterator_to(list_.back()) || + out_pos_ != out_end_) { in_pos_ = out_pos_; } else { - // Use the whole buffer now. + // Input and output sequences are empty, reuse buffer. // Alternatively we could deallocate it. in_pos_ = 0; out_pos_ = 0; @@ -759,6 +761,8 @@ void basic_streambuf::debug_check() const { #ifndef NDEBUG + using boost::asio::buffer_size; + assert(buffer_size(data()) == in_size_); if(list_.empty()) { assert(in_pos_ == 0); diff --git a/test/basic_streambuf.cpp b/test/basic_streambuf.cpp index f82d53854..6b446fd55 100644 --- a/test/basic_streambuf.cpp +++ b/test/basic_streambuf.cpp @@ -7,3 +7,295 @@ // Test that header file is self-contained. #include + +#include +#include +#include +#include +#include +#include + +namespace beast { +namespace test { + +struct test_allocator_info +{ + std::size_t ncopy = 0; + std::size_t nmove = 0; + std::size_t nselect = 0; +}; + +template +class test_allocator; + +template +struct test_allocator_base +{ +}; + +template +struct test_allocator_base +{ + static + test_allocator + select_on_container_copy_construction( + test_allocator const& a) + { + return test_allocator{}; + } +}; + +template +class test_allocator : public test_allocator_base< + T, Assign, Move, Swap, Select> +{ + std::size_t id_; + std::shared_ptr info_; + + template + friend class test_allocator; + +public: + using value_type = T; + using propagate_on_container_copy_assignment = + std::integral_constant; + using propagate_on_container_move_assignment = + std::integral_constant; + using propagate_on_container_swap = + std::integral_constant; + + template + struct rebind + { + using other = test_allocator< + U, Assign, Move, Swap, Select>; + }; + + test_allocator() + : id_([] + { + static std::atomic< + std::size_t> sid(0); + return ++sid; + }()) + , info_(std::make_shared()) + { + } + + test_allocator(test_allocator const& u) noexcept + : id_(u.id_) + , info_(u.info_) + { + ++info_->ncopy; + } + + template + test_allocator(test_allocator< + U, Assign, Move, Swap, Select> const& u) noexcept + : id_(u.id_) + , info_(u.info_) + { + ++info_->ncopy; + } + + test_allocator(test_allocator&& t) + : id_(t.id_) + , info_(t.info_) + { + ++info_->nmove; + } + + value_type* + allocate(std::size_t n) + { + return static_cast( + ::operator new (n*sizeof(value_type))); + } + + void + deallocate(value_type* p, std::size_t) noexcept + { + ::operator delete(p); + } + + std::size_t + id() const + { + return id_; + } + + test_allocator_info const* + operator->() const + { + return info_.get(); + } +}; + +class basic_streambuf_test : public beast::detail::unit_test::suite +{ +public: + template + static + std::string + to_string(ConstBufferSequence const& bs) + { + using boost::asio::buffer_cast; + using boost::asio::buffer_size; + std::string s; + s.reserve(buffer_size(bs)); + for(auto const& b : bs) + s.append(buffer_cast(b), + buffer_size(b)); + return s; + } + + void + testPrepare() + { + using boost::asio::buffer_size; + streambuf sb(2); + expect(buffer_size(sb.prepare(5)) == 5); + expect(buffer_size(sb.prepare(8)) == 8); + expect(buffer_size(sb.prepare(7)) == 7); + } + + void testStreambuf() + { + using boost::asio::buffer; + using boost::asio::buffer_cast; + using boost::asio::buffer_size; + std::string const s = "Hello, world"; + expect(s.size() == 12); + for(std::size_t i = 1; i < 12; ++i) { + for(std::size_t x = 1; x < 4; ++x) { + for(std::size_t y = 1; y < 4; ++y) { + for(std::size_t t = 1; t < 4; ++ t) { + for(std::size_t u = 1; u < 4; ++ u) { + std::size_t z = s.size() - (x + y); + std::size_t v = s.size() - (t + u); + { + streambuf sb(i); + decltype(sb)::mutable_buffers_type d; + d = sb.prepare(z); expect(buffer_size(d) == z); + d = sb.prepare(0); expect(buffer_size(d) == 0); + d = sb.prepare(y); expect(buffer_size(d) == y); + d = sb.prepare(x); expect(buffer_size(d) == x); + sb.commit(buffer_copy(d, buffer(s.data(), x))); + expect(sb.size() == x); + expect(buffer_size(sb.data()) == sb.size()); + d = sb.prepare(x); expect(buffer_size(d) == x); + d = sb.prepare(0); expect(buffer_size(d) == 0); + d = sb.prepare(z); expect(buffer_size(d) == z); + d = sb.prepare(y); expect(buffer_size(d) == y); + sb.commit(buffer_copy(d, buffer(s.data()+x, y))); + sb.commit(1); + expect(sb.size() == x + y); + expect(buffer_size(sb.data()) == sb.size()); + d = sb.prepare(x); expect(buffer_size(d) == x); + d = sb.prepare(y); expect(buffer_size(d) == y); + d = sb.prepare(0); expect(buffer_size(d) == 0); + d = sb.prepare(z); expect(buffer_size(d) == z); + sb.commit(buffer_copy(d, buffer(s.data()+x+y, z))); + sb.commit(2); + expect(sb.size() == x + y + z); + expect(buffer_size(sb.data()) == sb.size()); + expect(to_string(sb.data()) == s); + sb.consume(t); + d = sb.prepare(0); expect(buffer_size(d) == 0); + expect(to_string(sb.data()) == s.substr(t, std::string::npos)); + sb.consume(u); + expect(to_string(sb.data()) == s.substr(t + u, std::string::npos)); + sb.consume(v); + expect(to_string(sb.data()) == ""); + sb.consume(1); + d = sb.prepare(0); expect(buffer_size(d) == 0); + } + }}}}} + } + + template + static + bool + eq(basic_streambuf const& sb1, + basic_streambuf const& sb2) + { + return to_string(sb1.data()) == to_string(sb2.data()); + } + + void testSpecial() + { + using boost::asio::buffer; + using boost::asio::buffer_cast; + using boost::asio::buffer_size; + std::string const s = "Hello, world"; + expect(s.size() == 12); + for(std::size_t i = 1; i < 12; ++i) { + for(std::size_t x = 1; x < 4; ++x) { + for(std::size_t y = 1; y < 4; ++y) { + std::size_t z = s.size() - (x + y); + { + streambuf sb(i); + sb.commit(buffer_copy(sb.prepare(x), buffer(s.data(), x))); + sb.commit(buffer_copy(sb.prepare(y), buffer(s.data()+x, y))); + sb.commit(buffer_copy(sb.prepare(z), buffer(s.data()+x+y, z))); + expect(to_string(sb.data()) == s); + { + streambuf sb2(sb); + expect(eq(sb, sb2)); + } + { + streambuf sb2; + sb2 = sb; + expect(eq(sb, sb2)); + } + { + streambuf sb2(std::move(sb)); + expect(to_string(sb2.data()) == s); + expect(buffer_size(sb.data()) == 0); + sb = std::move(sb2); + expect(to_string(sb.data()) == s); + expect(buffer_size(sb2.data()) == 0); + } + } + }}} + } + + void testAllocator() + { + { + using alloc_type = + test_allocator; + using sb_type = basic_streambuf; + sb_type sb; + expect(sb.get_allocator().id() == 1); + } + { + using alloc_type = + test_allocator; + using sb_type = basic_streambuf; + sb_type sb; + expect(sb.get_allocator().id() == 2); + sb_type sb2(sb); + expect(sb2.get_allocator().id() == 2); + sb_type sb3(sb, alloc_type{}); + //expect(sb3.get_allocator().id() == 3); + } + } + + void run() override + { + testPrepare(); + testStreambuf(); + testSpecial(); + testAllocator(); + } +}; + +BEAST_DEFINE_TESTSUITE(basic_streambuf,asio,beast); + +} // test +} // beast diff --git a/test/streambuf.cpp b/test/streambuf.cpp index 8f7bc414a..608d5427e 100644 --- a/test/streambuf.cpp +++ b/test/streambuf.cpp @@ -7,283 +7,3 @@ // Test that header file is self-contained. #include - -#include -#include -#include -#include -#include - -namespace beast { -namespace test { - -struct test_allocator_info -{ - std::size_t ncopy = 0; - std::size_t nmove = 0; - std::size_t nselect = 0; -}; - -template -class test_allocator; - -template -struct test_allocator_base -{ -}; - -template -struct test_allocator_base -{ - static - test_allocator - select_on_container_copy_construction( - test_allocator const& a) - { - return test_allocator{}; - } -}; - -template -class test_allocator : public test_allocator_base< - T, Assign, Move, Swap, Select> -{ - std::size_t id_; - std::shared_ptr info_; - - template - friend class test_allocator; - -public: - using value_type = T; - using propagate_on_container_copy_assignment = - std::integral_constant; - using propagate_on_container_move_assignment = - std::integral_constant; - using propagate_on_container_swap = - std::integral_constant; - - template - struct rebind - { - using other = test_allocator< - U, Assign, Move, Swap, Select>; - }; - - test_allocator() - : id_([] - { - static std::atomic< - std::size_t> sid(0); - return ++sid; - }()) - , info_(std::make_shared()) - { - } - - test_allocator(test_allocator const& u) noexcept - : id_(u.id_) - , info_(u.info_) - { - ++info_->ncopy; - } - - template - test_allocator(test_allocator< - U, Assign, Move, Swap, Select> const& u) noexcept - : id_(u.id_) - , info_(u.info_) - { - ++info_->ncopy; - } - - test_allocator(test_allocator&& t) - : id_(t.id_) - , info_(t.info_) - { - ++info_->nmove; - } - - value_type* - allocate(std::size_t n) - { - return static_cast( - ::operator new (n*sizeof(value_type))); - } - - void - deallocate(value_type* p, std::size_t) noexcept - { - ::operator delete(p); - } - - std::size_t - id() const - { - return id_; - } - - test_allocator_info const* - operator->() const - { - return info_.get(); - } -}; - -class streambuf_test : public beast::detail::unit_test::suite -{ -public: - template - static - std::string - to_string(ConstBufferSequence const& bs) - { - using boost::asio::buffer_cast; - using boost::asio::buffer_size; - std::string s; - s.reserve(buffer_size(bs)); - for(auto const& b : bs) - s.append(buffer_cast(b), - buffer_size(b)); - return s; - } - - void testStreambuf() - { - using boost::asio::buffer; - using boost::asio::buffer_cast; - using boost::asio::buffer_size; - std::string const s = "Hello, world"; - expect(s.size() == 12); - for(std::size_t i = 1; i < 12; ++i) { - for(std::size_t x = 1; x < 4; ++x) { - for(std::size_t y = 1; y < 4; ++y) { - for(std::size_t t = 1; t < 4; ++ t) { - for(std::size_t u = 1; u < 4; ++ u) { - std::size_t z = s.size() - (x + y); - std::size_t v = s.size() - (t + u); - { - streambuf sb(i); - decltype(sb)::mutable_buffers_type d; - d = sb.prepare(z); expect(buffer_size(d) == z); - d = sb.prepare(0); expect(buffer_size(d) == 0); - d = sb.prepare(y); expect(buffer_size(d) == y); - d = sb.prepare(x); expect(buffer_size(d) == x); - sb.commit(buffer_copy(d, buffer(s.data(), x))); - expect(sb.size() == x); - expect(buffer_size(sb.data()) == sb.size()); - d = sb.prepare(x); expect(buffer_size(d) == x); - d = sb.prepare(0); expect(buffer_size(d) == 0); - d = sb.prepare(z); expect(buffer_size(d) == z); - d = sb.prepare(y); expect(buffer_size(d) == y); - sb.commit(buffer_copy(d, buffer(s.data()+x, y))); - sb.commit(1); - expect(sb.size() == x + y); - expect(buffer_size(sb.data()) == sb.size()); - d = sb.prepare(x); expect(buffer_size(d) == x); - d = sb.prepare(y); expect(buffer_size(d) == y); - d = sb.prepare(0); expect(buffer_size(d) == 0); - d = sb.prepare(z); expect(buffer_size(d) == z); - sb.commit(buffer_copy(d, buffer(s.data()+x+y, z))); - sb.commit(2); - expect(sb.size() == x + y + z); - expect(buffer_size(sb.data()) == sb.size()); - expect(to_string(sb.data()) == s); - sb.consume(t); - d = sb.prepare(0); expect(buffer_size(d) == 0); - expect(to_string(sb.data()) == s.substr(t, std::string::npos)); - sb.consume(u); - expect(to_string(sb.data()) == s.substr(t + u, std::string::npos)); - sb.consume(v); - expect(to_string(sb.data()) == ""); - sb.consume(1); - d = sb.prepare(0); expect(buffer_size(d) == 0); - } - }}}}} - } - - template - static - bool - eq(basic_streambuf const& sb1, - basic_streambuf const& sb2) - { - return to_string(sb1.data()) == to_string(sb2.data()); - } - - void testSpecial() - { - using boost::asio::buffer; - using boost::asio::buffer_cast; - using boost::asio::buffer_size; - std::string const s = "Hello, world"; - expect(s.size() == 12); - for(std::size_t i = 1; i < 12; ++i) { - for(std::size_t x = 1; x < 4; ++x) { - for(std::size_t y = 1; y < 4; ++y) { - std::size_t z = s.size() - (x + y); - { - streambuf sb(i); - sb.commit(buffer_copy(sb.prepare(x), buffer(s.data(), x))); - sb.commit(buffer_copy(sb.prepare(y), buffer(s.data()+x, y))); - sb.commit(buffer_copy(sb.prepare(z), buffer(s.data()+x+y, z))); - expect(to_string(sb.data()) == s); - { - streambuf sb2(sb); - expect(eq(sb, sb2)); - } - { - streambuf sb2; - sb2 = sb; - expect(eq(sb, sb2)); - } - { - streambuf sb2(std::move(sb)); - expect(to_string(sb2.data()) == s); - expect(buffer_size(sb.data()) == 0); - sb = std::move(sb2); - expect(to_string(sb.data()) == s); - expect(buffer_size(sb2.data()) == 0); - } - } - }}} - } - - void testAllocator() - { - { - using alloc_type = - test_allocator; - using sb_type = basic_streambuf; - sb_type sb; - expect(sb.get_allocator().id() == 1); - } - { - using alloc_type = - test_allocator; - using sb_type = basic_streambuf; - sb_type sb; - expect(sb.get_allocator().id() == 2); - sb_type sb2(sb); - expect(sb2.get_allocator().id() == 2); - sb_type sb3(sb, alloc_type{}); - //expect(sb3.get_allocator().id() == 3); - } - } - - void run() override - { - testStreambuf(); - testSpecial(); - testAllocator(); - } -}; - -BEAST_DEFINE_TESTSUITE(streambuf,asio,beast); - -} // test -} // beast