feedc0de/boost_container
1
0
Fork 0
forked from boostorg/container
Code Pull Requests Activity

Fixes #197 ("small_vector::swap causes spurious allocations and suboptimal performance")

Browse Source
This commit is contained in:
Ion Gaztañaga
2021-11-01 00:19:16 +01:00
parent 7f35ef420e
commit f6a03fd3f2
3 changed files with 79 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
doc/container.qbk
View File

@@ -1345,6 +1345,7 @@ use [*Boost.Container]? There are several reasons for that:
* [@https://github.com/boostorg/container/issues/187 GitHub #187: ['"flat_map::erase and unique keys"]].
* [@https://github.com/boostorg/container/issues/188 GitHub #188: ['"Build fails when RTTI is disabled"]].
* [@https://github.com/boostorg/container/issues/192 GitHub #192: ['"basic_string::clear() has poor codegen compared to STL implementations"]].
* [@https://github.com/boostorg/container/issues/197 GitHub #197: ['"small_vector::swap causes spurious allocations and suboptimal performance"]].
[endsect]

46
include/boost/container/vector.hpp
View File

@@ -87,8 +87,8 @@ class vec_iterator
//Defining element_type to make libstdc++'s std::pointer_traits well-formed leads to ambiguity
//due to LWG3446. So we need to specialize std::pointer_traits. See
//https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96416 for details. /Many thanks to Jonathan Wakely
//for explaning the issue.
//https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96416 for details. Many thanks to Jonathan Wakely
//for explaining the issue.
#ifndef BOOST_GNU_STDLIB
//Define element_
typedef typename boost::intrusive::pointer_traits<Pointer>::element_type element_type;
@@ -317,7 +317,8 @@ struct vector_alloc_holder
(void)propagate_allocator; (void)p; (void)to_alloc; (void)from_alloc;
const bool all_storage_propagable = !allocator_traits_type::is_partially_propagable::value ||
!allocator_traits_type::storage_is_unpropagable(from_alloc, p);
return all_storage_propagable && (propagate_allocator || allocator_traits_type::equal(from_alloc, to_alloc));
return all_storage_propagable &&
(propagate_allocator || allocator_traits_type::is_always_equal::value || allocator_traits_type::equal(from_alloc, to_alloc));
}
BOOST_CONTAINER_FORCEINLINE
@@ -2549,31 +2550,40 @@ private:
void priv_swap(Vector &x, dtl::false_type) //version_N
{
const bool propagate_alloc = allocator_traits_type::propagate_on_container_swap::value;
if(are_swap_propagable( this->get_stored_allocator(), this->m_holder.start()
, x.get_stored_allocator(), x.m_holder.start(), propagate_alloc)){
if (BOOST_UNLIKELY(&x == this)){
return;
}
else if(are_swap_propagable( this->get_stored_allocator(), this->m_holder.start()
, x.get_stored_allocator(), x.m_holder.start(), propagate_alloc)){
//Just swap internals
this->m_holder.swap_resources(x.m_holder);
}
else{
if (BOOST_UNLIKELY(&x == this))
return;
//Else swap element by element...
bool const t_smaller = this->size() < x.size();
vector &sml = t_smaller ? *this : x;
vector &big = t_smaller ? x : *this;
size_type const common_elements = sml.size();
for(size_type i = 0; i != common_elements; ++i){
boost::adl_move_swap(sml[i], big[i]);
//For empty containers, maybe storage can be moved from the other (just like in the move constructor)
if(sml.empty() && is_propagable_from(big.get_stored_allocator(), big.data(), sml.get_allocator(), propagate_alloc)){
if(BOOST_LIKELY(0u != sml.capacity()))
sml.m_holder.deallocate(sml.m_holder.m_start, sml.m_holder.m_capacity);
sml.steal_resources(big);
}
else {
//Else swap element by element...
size_type const common_elements = sml.size();
for(size_type i = 0; i != common_elements; ++i){
boost::adl_move_swap(sml[i], big[i]);
}
//... and move-insert the remaining range
sml.insert( sml.cend()
, boost::make_move_iterator(boost::movelib::iterator_to_raw_pointer(big.nth(common_elements)))
, boost::make_move_iterator(boost::movelib::iterator_to_raw_pointer(big.end()))
);
//Destroy remaining elements
big.erase(big.nth(common_elements), big.cend());
}
//... and move-insert the remaining range
sml.insert( sml.cend()
, boost::make_move_iterator(boost::movelib::iterator_to_raw_pointer(big.nth(common_elements)))
, boost::make_move_iterator(boost::movelib::iterator_to_raw_pointer(big.end()))
);
//Destroy remaining elements
big.erase(big.nth(common_elements), big.cend());
}
//And now swap the allocator
dtl::swap_alloc(this->m_holder.alloc(), x.m_holder.alloc(), dtl::bool_<propagate_alloc>());

75
test/vector_test.hpp
View File

@@ -43,7 +43,7 @@ namespace container {
namespace test{
template<class Vector>
struct vector_hash_function_capacity
struct vector_has_function_capacity
{
typedef typename Vector::size_type size_type;
template <typename U, size_type (U::*)() const> struct Check;
@@ -57,32 +57,57 @@ struct vector_hash_function_capacity
template<class V1, class V2>
bool vector_capacity_test(V1&, V2&, boost::container::dtl::false_type)
{
//deque has no reserve
return true;
}
template<class MyBoostVector, class MyStdVector>
bool vector_capacity_test(MyBoostVector&boostvector, MyStdVector&stdvector, boost::container::dtl::true_type)
{
//deque has no reserve
boostvector.reserve(boostvector.size()*2);
stdvector.reserve(stdvector.size()*2);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
{
boostvector.reserve(boostvector.size()*2);
stdvector.reserve(stdvector.size()*2);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
std::size_t cap = boostvector.capacity();
boostvector.reserve(cap*2);
stdvector.reserve(cap*2);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
boostvector.resize(0);
stdvector.resize(0);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
std::size_t cap = boostvector.capacity();
boostvector.reserve(cap*2);
stdvector.reserve(cap*2);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
boostvector.resize(0);
stdvector.resize(0);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
boostvector.resize(cap*2);
stdvector.resize(cap*2);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
boostvector.resize(cap*2);
stdvector.resize(cap*2);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
boostvector.resize(cap*2);
stdvector.resize(cap*2);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
boostvector.resize(cap*2);
stdvector.resize(cap*2);
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
}
//test capacity swapping in swap
{
MyBoostVector a, b;
a.resize(1000);
const std::size_t sz = a.size();
const std::size_t cap = a.capacity();
a.resize(1000);
a.swap(b);
if( !(b.capacity() == cap) ) return false;
if( !(b.size() == sz) ) return false;
if( !(a.capacity() != cap) ) return false;
if( !(a.empty()) ) return false;
a.swap(b);
if( !(a.capacity() == cap) ) return false;
if( !(a.size() == sz) ) return false;
if( !(b.capacity() != cap) ) return false;
if( !(b.empty()) ) return false;
}
return true;
}
@@ -161,21 +186,21 @@ bool vector_copyable_only(MyBoostVector &boostvector, MyStdVector &stdvector, bo
stdvector.clear();
boostvector.assign(v1.begin(), v1.end());
stdvector.assign(v2.begin(), v2.end());
if(!test::CheckEqualContainers(boostvector, stdvector)) return 1;
if(!test::CheckEqualContainers(boostvector, stdvector)) return false;
}
{ //Vector(n, T)
::boost::movelib::unique_ptr<MyStdVector> const stdvectorp =
::boost::movelib::make_unique<MyStdVector>(100u, int(5));
::boost::movelib::unique_ptr<MyBoostVector> const boostvectorp =
::boost::movelib::make_unique<MyBoostVector>(100u, IntType(5));
if(!test::CheckEqualContainers(*boostvectorp, *stdvectorp)) return 1;
if(!test::CheckEqualContainers(*boostvectorp, *stdvectorp)) return false;
}
{ //Vector(n, T, alloc)
::boost::movelib::unique_ptr<MyStdVector> const stdvectorp =
::boost::movelib::make_unique<MyStdVector>(100u, int(5));
::boost::movelib::unique_ptr<MyBoostVector> const boostvectorp =
::boost::movelib::make_unique<MyBoostVector>(100u, IntType(5), typename MyBoostVector::allocator_type());
if(!test::CheckEqualContainers(*boostvectorp, *stdvectorp)) return 1;
if(!test::CheckEqualContainers(*boostvectorp, *stdvectorp)) return false;
}
{ //Vector(It, It)
::boost::movelib::unique_ptr<MyStdVector> const stdvectorp =
@@ -184,7 +209,7 @@ bool vector_copyable_only(MyBoostVector &boostvector, MyStdVector &stdvector, bo
::boost::movelib::make_unique<MyBoostVector>(100u);
::boost::movelib::unique_ptr<MyBoostVector> const boostvectorp2 =
::boost::movelib::make_unique<MyBoostVector>(boostvectorp->begin(), boostvectorp->end());
if(!test::CheckEqualContainers(*boostvectorp2, *stdvectorp)) return 1;
if(!test::CheckEqualContainers(*boostvectorp2, *stdvectorp)) return false;
}
{ //Vector(It, It, alloc)
::boost::movelib::unique_ptr<MyStdVector> const stdvectorp =
@@ -193,7 +218,7 @@ bool vector_copyable_only(MyBoostVector &boostvector, MyStdVector &stdvector, bo
::boost::movelib::make_unique<MyBoostVector>(100u);
::boost::movelib::unique_ptr<MyBoostVector> const boostvectorp2 =
::boost::movelib::make_unique<MyBoostVector>(boostvectorp->begin(), boostvectorp->end(), typename MyBoostVector::allocator_type());
if(!test::CheckEqualContainers(*boostvectorp2, *stdvectorp)) return 1;
if(!test::CheckEqualContainers(*boostvectorp2, *stdvectorp)) return false;
}
{ //resize(n, T)
::boost::movelib::unique_ptr<MyStdVector> const stdvectorp =
@@ -202,7 +227,7 @@ bool vector_copyable_only(MyBoostVector &boostvector, MyStdVector &stdvector, bo
::boost::movelib::make_unique<MyBoostVector>();
stdvectorp->resize(100u, int(9));
boostvectorp->resize(100u, IntType(9));
if(!test::CheckEqualContainers(*boostvectorp, *stdvectorp)) return 1;
if(!test::CheckEqualContainers(*boostvectorp, *stdvectorp)) return false;
}
//operator=
{
@@ -498,7 +523,7 @@ int vector_test()
if(!test::CheckEqualContainers(boostvector, stdvector)) return 1;
}
if(!vector_capacity_test(boostvector, stdvector, dtl::bool_<vector_hash_function_capacity<MyBoostVector>::value>()))
if(!vector_capacity_test(boostvector, stdvector, dtl::bool_<vector_has_function_capacity<MyBoostVector>::value>()))
return 1;
boostvector.clear();