Skip to content

Fix object strong guarantee #907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
grisumbras merged 2 commits into from
Jun 14, 2023
Merged

Fix object strong guarantee #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c432951
do not reserve preemptively in object
grisumbras Jun 10, 2023
8957955
restore object storage on failed range insert
grisumbras Jun 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 71 additions & 24 deletions include/boost/json/impl/object.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ class object::revert_construct
class object::revert_insert
{
object* obj_;
table* t_ = nullptr;
std::size_t size_;

BOOST_JSON_DECL
Expand All @@ -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<index_t>(size_);
}
}

void
commit() noexcept
{
BOOST_ASSERT(obj_);
if( t_ )
table::deallocate( t_, obj_->sp_ );
obj_ = nullptr;
}
};
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -376,7 +401,7 @@ insert(P&& p) ->
{
key_value_pair v(
std::forward<P>(p), sp_);
return insert_impl(pilfer(v));
return emplace_impl( v.key(), pilfer(v) );
}

template<class M>
Expand All @@ -386,18 +411,14 @@ insert_or_assign(
string_view key, M&& m) ->
std::pair<iterator, bool>
{
reserve(size() + 1);
auto const result = detail::find_in_object(*this, key);
if(result.first)
std::pair<iterator, bool> result = emplace_impl(
key, key, static_cast<M&&>(m) );
if( !result.second )
{
value(std::forward<M>(m),
sp_).swap(result.first->value());
return { result.first, false };
value(static_cast<M>(m), sp_).swap(
result.first->value());
}
key_value_pair kv(key,
std::forward<M>(m), sp_);
return { insert_impl(pilfer(kv),
result.second), true };
return result;
}

template<class Arg>
Expand All @@ -408,14 +429,7 @@ emplace(
Arg&& arg) ->
std::pair<iterator, bool>
{
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>(arg), sp_);
return { insert_impl(pilfer(kv),
result.second), true };
return emplace_impl( key, key, static_cast<Arg&&>(arg) );
}

//----------------------------------------------------------
Expand Down Expand Up @@ -502,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);
Expand All @@ -512,6 +525,40 @@ insert(
r.commit();
}

template< class... Args >
std::pair<object::iterator, bool>
object::
emplace_impl( string_view key, Args&& ... args )
{
std::pair<iterator, std::size_t> 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&&>(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 {
Expand Down
31 changes: 7 additions & 24 deletions include/boost/json/impl/object.ipp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -683,23 +682,6 @@ if_contains(
//
//----------------------------------------------------------

auto
object::
insert_impl(
pilfered<key_value_pair> p) ->
std::pair<iterator, bool>
{
// 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(
Expand All @@ -725,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);
Expand All @@ -743,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())
{
Expand All @@ -761,6 +742,8 @@ rehash(std::size_t new_capacity)
head = i;
}
}

return t;
}

bool
Expand Down
29 changes: 12 additions & 17 deletions include/boost/json/object.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

//------------------------------------------------------
//
Expand Down Expand Up @@ -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`.
Expand All @@ -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
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -1596,10 +1592,9 @@ class object
InputIt last,
std::forward_iterator_tag);

BOOST_JSON_DECL
template< class... Args >
std::pair<iterator, bool>
insert_impl(
pilfered<key_value_pair> p);
emplace_impl(string_view key, Args&& ... args );

BOOST_JSON_DECL
key_value_pair*
Expand All @@ -1608,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
Expand Down
63 changes: 63 additions & 0 deletions test/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<object>::value );
BOOST_STATIC_ASSERT( std::is_nothrow_move_constructible<object>::value );

Expand Down Expand Up @@ -1595,6 +1617,46 @@ 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() );

// Check that insertion rolls back reserve when cannot insert all
// elements.
std::array<throws_on_convert, 10> input;
try
{
o.insert( input.begin(), input.end() );
}
catch( ... )
{
// ignore
}
BOOST_TEST( capacity == o.capacity() );
}

void
run()
{
Expand All @@ -1610,6 +1672,7 @@ class object_test
testEquality();
testAllocation();
testHash();
testStrongGurantee();
}
};

Expand Down