From c4329519cdc690264ffda2319146b1c8c21f60ec Mon Sep 17 00:00:00 2001 From: Dmitry Arkhipov Date: Sat, 10 Jun 2023 21:29:51 +0300 Subject: [PATCH 1/2] do not reserve preemptively in object --- include/boost/json/impl/object.hpp | 61 ++++++++++++++++++++---------- include/boost/json/impl/object.ipp | 17 --------- include/boost/json/object.hpp | 5 +-- test/object.cpp | 28 ++++++++++++++ 4 files changed, 72 insertions(+), 39 deletions(-) diff --git a/include/boost/json/impl/object.hpp b/include/boost/json/impl/object.hpp index c0c960f10..eaf57868c 100644 --- a/include/boost/json/impl/object.hpp +++ b/include/boost/json/impl/object.hpp @@ -376,7 +376,7 @@ insert(P&& p) -> { key_value_pair v( std::forward

(p), sp_); - return insert_impl(pilfer(v)); + return emplace_impl( v.key(), pilfer(v) ); } template @@ -386,18 +386,14 @@ insert_or_assign( string_view key, M&& m) -> std::pair { - reserve(size() + 1); - auto const result = detail::find_in_object(*this, key); - if(result.first) + std::pair result = emplace_impl( + key, key, static_cast(m) ); + if( !result.second ) { - value(std::forward(m), - sp_).swap(result.first->value()); - return { result.first, false }; + value(static_cast(m), sp_).swap( + result.first->value()); } - key_value_pair kv(key, - std::forward(m), sp_); - return { insert_impl(pilfer(kv), - result.second), true }; + return result; } template @@ -408,14 +404,7 @@ emplace( Arg&& arg) -> std::pair { - reserve(size() + 1); - auto const result = detail::find_in_object(*this, key); - if(result.first) - return { result.first, false }; - key_value_pair kv(key, - std::forward(arg), sp_); - return { insert_impl(pilfer(kv), - result.second), true }; + return emplace_impl( key, key, static_cast(arg) ); } //---------------------------------------------------------- @@ -512,6 +501,40 @@ insert( r.commit(); } +template< class... Args > +std::pair +object:: +emplace_impl( string_view key, Args&& ... args ) +{ + std::pair search_result(nullptr, 0); + if( !empty() ) + { + search_result = detail::find_in_object(*this, key); + if( search_result.first ) + return { search_result.first, false }; + } + + // we create the new value before reserving, in case it is a reference to + // a subobject of the current object + key_value_pair kv( static_cast(args)..., sp_ ); + // the key might get deallocated too + key = kv.key(); + + std::size_t const old_capacity = capacity(); + reserve(size() + 1); + if( (empty() && capacity() > detail::small_object_size_) + || (capacity() != old_capacity) ) + search_result.second = detail::digest( + key.begin(), key.end(), t_->salt); + + BOOST_ASSERT( + t_->is_small() || + (search_result.second == + detail::digest(key.begin(), key.end(), t_->salt)) ); + + return { insert_impl(pilfer(kv), search_result.second), true }; +} + //---------------------------------------------------------- namespace detail { diff --git a/include/boost/json/impl/object.ipp b/include/boost/json/impl/object.ipp index dcad164e2..7446449ee 100644 --- a/include/boost/json/impl/object.ipp +++ b/include/boost/json/impl/object.ipp @@ -683,23 +683,6 @@ if_contains( // //---------------------------------------------------------- -auto -object:: -insert_impl( - pilfered p) -> - std::pair -{ - // caller is responsible - // for preventing aliasing. - reserve(size() + 1); - auto const result = - detail::find_in_object(*this, p.get().key()); - if(result.first) - return { result.first, false }; - return { insert_impl( - p, result.second), true }; -} - key_value_pair* object:: insert_impl( diff --git a/include/boost/json/object.hpp b/include/boost/json/object.hpp index c388411c0..05ca33a73 100644 --- a/include/boost/json/object.hpp +++ b/include/boost/json/object.hpp @@ -1596,10 +1596,9 @@ class object InputIt last, std::forward_iterator_tag); - BOOST_JSON_DECL + template< class... Args > std::pair - insert_impl( - pilfered p); + emplace_impl(string_view key, Args&& ... args ); BOOST_JSON_DECL key_value_pair* diff --git a/test/object.cpp b/test/object.cpp index b8da00aaa..70a4461bd 100644 --- a/test/object.cpp +++ b/test/object.cpp @@ -1595,6 +1595,33 @@ class object_test object({{"b",2}, {"c",3}}))); } + void + testStrongGurantee() + { + // We used to preemptively reserve storage even if we don't add a new + // element. That violated strong guarantee requirement. This test + // checks we don't do that any more. + + object o; + o.reserve(100); + std::size_t const capacity = o.capacity(); + for( std::size_t i = 0; i < o.capacity() ; ++i ) + o.emplace( std::to_string(i), i ); + BOOST_ASSERT( capacity == o.capacity() ); + + BOOST_TEST( !o.emplace("0", 0).second ); + BOOST_TEST( capacity == o.capacity() ); + + BOOST_TEST( !o.insert_or_assign("0", 0).second ); + BOOST_TEST( capacity == o.capacity() ); + + o["0"] = 0; + BOOST_TEST( capacity == o.capacity() ); + + o.insert( key_value_pair("0", nullptr) ); + BOOST_TEST( capacity == o.capacity() ); + } + void run() { @@ -1610,6 +1637,7 @@ class object_test testEquality(); testAllocation(); testHash(); + testStrongGurantee(); } }; From 8957955af4fc5ea8b8902481fd61545de9c39e56 Mon Sep 17 00:00:00 2001 From: Dmitry Arkhipov Date: Sat, 10 Jun 2023 23:45:59 +0300 Subject: [PATCH 2/2] restore object storage on failed range insert --- include/boost/json/impl/object.hpp | 34 ++++++++++++++++++++++++----- include/boost/json/impl/object.ipp | 14 ++++++------ include/boost/json/object.hpp | 24 +++++++++----------- test/object.cpp | 35 ++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 26 deletions(-) diff --git a/include/boost/json/impl/object.hpp b/include/boost/json/impl/object.hpp index eaf57868c..d2458fcfb 100644 --- a/include/boost/json/impl/object.hpp +++ b/include/boost/json/impl/object.hpp @@ -154,6 +154,7 @@ class object::revert_construct class object::revert_insert { object* obj_; + table* t_ = nullptr; std::size_t size_; BOOST_JSON_DECL @@ -163,24 +164,38 @@ class object::revert_insert public: explicit revert_insert( - object& obj) noexcept + object& obj, + std::size_t capacity) : obj_(&obj) , size_(obj_->size()) { + if( capacity > obj_->capacity() ) + t_ = obj_->reserve_impl(capacity); } ~revert_insert() { if(! obj_) return; + destroy(); - obj_->t_->size = static_cast< - index_t>(size_); + if( t_ ) + { + table::deallocate( obj_->t_, obj_->sp_ ); + obj_->t_ = t_; + } + else + { + obj_->t_->size = static_cast(size_); + } } void commit() noexcept { + BOOST_ASSERT(obj_); + if( t_ ) + table::deallocate( t_, obj_->sp_ ); obj_ = nullptr; } }; @@ -329,6 +344,16 @@ capacity() const noexcept -> return t_->capacity; } +void +object:: +reserve(std::size_t new_capacity) +{ + if( new_capacity <= capacity() ) + return; + table* const old_table = reserve_impl(new_capacity); + table::deallocate( old_table, sp_ ); +} + //---------------------------------------------------------- // // Lookup @@ -491,8 +516,7 @@ insert( BOOST_STATIC_CONSTEXPR source_location loc = BOOST_CURRENT_LOCATION; detail::throw_system_error( error::object_too_large, &loc ); } - reserve(n0 + n); - revert_insert r(*this); + revert_insert r( *this, n0 + n ); while(first != last) { insert(*first); diff --git a/include/boost/json/impl/object.ipp b/include/boost/json/impl/object.ipp index 7446449ee..fc11c399b 100644 --- a/include/boost/json/impl/object.ipp +++ b/include/boost/json/impl/object.ipp @@ -452,8 +452,7 @@ insert( BOOST_STATIC_CONSTEXPR source_location loc = BOOST_CURRENT_LOCATION; detail::throw_system_error( error::object_too_large, &loc ); } - reserve(n0 + init.size()); - revert_insert r(*this); + revert_insert r( *this, n0 + init.size() ); if(t_->is_small()) { for(auto& iv : init) @@ -708,10 +707,10 @@ insert_impl( return pv; } -// rehash to at least `n` buckets -void +// allocate new table, copy elements there, and rehash them +object::table* object:: -rehash(std::size_t new_capacity) +reserve_impl(std::size_t new_capacity) { BOOST_ASSERT( new_capacity > t_->capacity); @@ -726,8 +725,7 @@ rehash(std::size_t new_capacity) size() * sizeof( key_value_pair)); t->size = t_->size; - table::deallocate(t_, sp_); - t_ = t; + std::swap(t_, t); if(! t_->is_small()) { @@ -744,6 +742,8 @@ rehash(std::size_t new_capacity) head = i; } } + + return t; } bool diff --git a/include/boost/json/object.hpp b/include/boost/json/object.hpp index 05ca33a73..418750734 100644 --- a/include/boost/json/object.hpp +++ b/include/boost/json/object.hpp @@ -898,13 +898,9 @@ class object @throw system_error `new_capacity > max_size()` */ + inline void - reserve(std::size_t new_capacity) - { - if(new_capacity <= capacity()) - return; - rehash(new_capacity); - } + reserve(std::size_t new_capacity); //------------------------------------------------------ // @@ -982,9 +978,9 @@ class object are two keys within the range that are equal to each other, only the first will be inserted. - If the size necessary to accomodate elements from the range exceeds - @ref capacity(), a rehashing can occur. In that case all iterators and - references are invalidated. Otherwise, they are not affected. + Insertion may result in rehashing of the container. In that case all + iterators and references are invalidated. Otherwise, they are not + affected. @par Precondition `first` and `last` are not iterators into `*this`. @@ -999,7 +995,8 @@ class object Linear in `std::distance(first, last)`. @par Exception Safety - Basic guarantee. + Strong guarantee for forward iterators, basic guarantee for input + iterators. Calls to `memory_resource::allocate` may throw. @param first An input iterator pointing to the first @@ -1033,8 +1030,7 @@ class object are two keys within the initializer list that are equal to each other, only the first will be inserted. - If the size necessary to accomodate elements from the initializer list - exceeds @ref capacity(), a rehashing can occur. In that case all + Insertion may result in rehashing of the container. In that case all iterators and references are invalidated. Otherwise, they are not affected. @@ -1607,8 +1603,8 @@ class object std::size_t hash); BOOST_JSON_DECL - void - rehash(std::size_t new_capacity); + table* + reserve_impl(std::size_t new_capacity); BOOST_JSON_DECL bool diff --git a/test/object.cpp b/test/object.cpp index 70a4461bd..1114e18cd 100644 --- a/test/object.cpp +++ b/test/object.cpp @@ -28,6 +28,28 @@ namespace boost { namespace json { +namespace { + +struct throws_on_convert +{ + // this member only exists due to MSVC code analysis bug that marks lines + // in callers of the type's operator key_value_pair() as unreachable (due + // to exception thrown), even if that caller is a function template, and + // the line is reachable in other instantiations + bool should_throw = true; + + throws_on_convert() = default; + + operator key_value_pair() + { + if( should_throw ) + throw std::invalid_argument(""); + return key_value_pair( "", nullptr); + } +}; + +} // namespace + BOOST_STATIC_ASSERT( std::is_nothrow_destructible::value ); BOOST_STATIC_ASSERT( std::is_nothrow_move_constructible::value ); @@ -1620,6 +1642,19 @@ class object_test o.insert( key_value_pair("0", nullptr) ); BOOST_TEST( capacity == o.capacity() ); + + // Check that insertion rolls back reserve when cannot insert all + // elements. + std::array input; + try + { + o.insert( input.begin(), input.end() ); + } + catch( ... ) + { + // ignore + } + BOOST_TEST( capacity == o.capacity() ); } void