mirror of
https://github.com/nlohmann/json.git
synced 2026-10-07 06:57:14 +00:00
Make basic_json destruction allocation-free and non-recursive (#5762)
* Use the provided allocator in destroy() (#4842) Uses the provided allocator to allocate the stack used to avoid recursion in the destroy() implementation used by ~basic_json. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Test that the destructor uses the provided allocator Adds a regression test for #4842: destroying a nested array or object must allocate its temporary stack through the basic_json allocator, not std::allocator. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Fix allocation failure during JSON destruction Signed-off-by: Michael Sam <michaelsam94@users.noreply.github.com> Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Make json_value::destroy() non-recursive and allocation-free destroy() used to flatten a nested array/object into a heap-allocated std::vector to avoid recursing per nesting level. That vector could itself throw bad_alloc under memory pressure, and since it now used the basic_json's own allocator (#4842), a failing allocator supplied by the caller made this more likely, not less. An exception thrown from inside ~basic_json(), which is noexcept, terminates the program (#5135). Replace the vector-based stack with a pointer-reversal walk that visits the tree without recursing per level and without allocating anything: cur is the array/object currently being emptied, prev is its parent (or null at the top). A parent's last child slot doubles as storage for that parent's own parent link while we are below it, so no extra memory is needed. A child is only ever removed once it is a scalar or an empty array/object, which neither allocates nor recurses more than one level deep. take() moves m_data between these locals directly, bypassing set_parents()/assert_invariant() (the former is O(#children) per call under JSON_DIAGNOSTICS, which would make the walk quadratic otherwise). This also removes the std::vector<basic_json, allocator_type> stack added by #4842, so the extra allocations it introduced disappear along with it. Co-authored-by: Michael Sam <9461037+michaelsam94@users.noreply.github.com> Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Test that destroy() performs no allocation, even under memory pressure Update the #4842 regression test: it used to check that destroying a nested array/object made at least one allocation through the provided allocator (the old flattening stack). Now that destroy() does not allocate at all, assert the opposite: zero allocations, deallocations only. Rework the #5135 regression test to use a dedicated failing/counting allocator instead of overriding the process-wide ::operator new and ::operator delete, which affected every allocation in the whole unit-regression2 binary rather than just the values under test. Keep the original small repro as one case, and add deep (100000 levels) and wide-and-deep nested array/object/ordered_json cases, all destroyed while every further allocation is made to fail: the destructor must complete without allocating, without throwing, and without leaking. Co-authored-by: Michael Sam <9461037+michaelsam94@users.noreply.github.com> Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Refactor destroy() for readability and add edge-case tests Apply review feedback from Greg Marr on the json_value::destroy() non-recursive, allocation-free destruction walk (#5135): - last_child() now uses object->rbegin()->second instead of std::prev(object->end())->second; pop_last_child() keeps std::prev(end()) since erase() needs a forward iterator. - is_empty_container() becomes has_no_children(), a switch that returns true for every non-container type as well as empty array/object, simplifying the "scalar or already-empty child" check at the call site. The local variable `last` is renamed to `cur_last_ref` for clarity. - free_container() asserts the array/object is already empty before freeing it, and the object branches assert the expected type. - destroy(value_t t) is now a thin dispatcher to destroy_string(), destroy_binary(), and destroy_container(t), each handling its own "not initialized" check and sharing the simple cases first in the switch. - destroy_container() moves the top-level container into the local stand-in via a plain swap of the json_value union, instead of a manual copy plus clearing array/object by hand. - The "cur has no children and there is no parent" case now frees cur and returns immediately, so the main loop is a plain while (true) with no trailing code after it. Also adds edge-case tests for both json and ordered_json (mixes of empty/non-empty arrays and objects, container children in first/last position, single-element chains, top-level empty containers, and destruction via erase()/assignment), plus a mixed-tree case in the "destructor performs no allocation" test. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Make the destroy() walk helpers private They modify basic_json internals without maintaining its invariants and are only meant for destroy_container(), so they no longer need to be reachable from the rest of basic_json. Signed-off-by: Niels Lohmann <mail@nlohmann.me> --------- Signed-off-by: Niels Lohmann <mail@nlohmann.me> Signed-off-by: Michael Sam <michaelsam94@users.noreply.github.com> Co-authored-by: Vesko Karaganev <vesko.karaganev@gmail.com> Co-authored-by: Michael Sam <michaelsam94@users.noreply.github.com> Co-authored-by: Michael Sam <9461037+michaelsam94@users.noreply.github.com>
This commit is contained in:
4 files changed
+734
-146
No files matched your search
@@ -607,3 +607,88 @@ TEST_CASE("bad my_allocator::construct")
|
||||
j["test"].push_back("should not leak");
|
||||
}
|
||||
}
|
||||
|
||||
namespace
|
||||
{
|
||||
std::size_t counting_allocator_allocations = 0;
|
||||
std::size_t counting_allocator_deallocations = 0;
|
||||
|
||||
template<class T>
|
||||
struct counting_allocator : std::allocator<T>
|
||||
{
|
||||
using std::allocator<T>::allocator;
|
||||
|
||||
T* allocate(std::size_t n)
|
||||
{
|
||||
++counting_allocator_allocations;
|
||||
return std::allocator<T>::allocate(n);
|
||||
}
|
||||
|
||||
void deallocate(T* p, std::size_t n)
|
||||
{
|
||||
++counting_allocator_deallocations;
|
||||
std::allocator<T>::deallocate(p, n);
|
||||
}
|
||||
|
||||
template <class U>
|
||||
struct rebind
|
||||
{
|
||||
using other = counting_allocator<U>;
|
||||
};
|
||||
};
|
||||
} // namespace
|
||||
|
||||
TEST_CASE("destructor performs no allocation, only deallocation")
|
||||
{
|
||||
// see https://github.com/nlohmann/json/issues/4842 and
|
||||
// https://github.com/nlohmann/json/issues/5135: destroying nested
|
||||
// arrays/objects used to allocate a temporary stack (first with
|
||||
// std::allocator, later - after #4842 - with the provided allocator).
|
||||
// Since that stack could itself throw bad_alloc from inside the
|
||||
// noexcept destructor (#5135), destroy() no longer allocates anything:
|
||||
// it only ever frees what is already there.
|
||||
using counting_json = nlohmann::basic_json<std::map,
|
||||
std::vector,
|
||||
std::string,
|
||||
bool,
|
||||
std::int64_t,
|
||||
std::uint64_t,
|
||||
double,
|
||||
counting_allocator>;
|
||||
|
||||
SECTION("array")
|
||||
{
|
||||
auto* j = new counting_json({1, {2, {3, 4}}, 5}); // NOLINT(cppcoreguidelines-owning-memory)
|
||||
const auto allocations_before = counting_allocator_allocations;
|
||||
const auto deallocations_before = counting_allocator_deallocations;
|
||||
delete j; // NOLINT(cppcoreguidelines-owning-memory)
|
||||
CHECK(counting_allocator_allocations == allocations_before);
|
||||
CHECK(counting_allocator_deallocations > deallocations_before);
|
||||
}
|
||||
|
||||
SECTION("object")
|
||||
{
|
||||
auto* j = new counting_json({{"a", {{"b", {1, 2}}}}, {"c", 3}}); // NOLINT(cppcoreguidelines-owning-memory)
|
||||
const auto allocations_before = counting_allocator_allocations;
|
||||
const auto deallocations_before = counting_allocator_deallocations;
|
||||
delete j; // NOLINT(cppcoreguidelines-owning-memory)
|
||||
CHECK(counting_allocator_allocations == allocations_before);
|
||||
CHECK(counting_allocator_deallocations > deallocations_before);
|
||||
}
|
||||
|
||||
SECTION("mixed tree of empty/non-empty arrays and objects")
|
||||
{
|
||||
auto* j = new counting_json( // NOLINT(cppcoreguidelines-owning-memory)
|
||||
{
|
||||
{"empty_obj", counting_json::object()},
|
||||
{"empty_arr", counting_json::array()},
|
||||
{"nested", {{"a", counting_json::array({1, 2, counting_json::object()})}, {"b", 3}}},
|
||||
{"tail", counting_json::array({counting_json::array({1}), 2, counting_json::array({3})})}
|
||||
});
|
||||
const auto allocations_before = counting_allocator_allocations;
|
||||
const auto deallocations_before = counting_allocator_deallocations;
|
||||
delete j; // NOLINT(cppcoreguidelines-owning-memory)
|
||||
CHECK(counting_allocator_allocations == allocations_before);
|
||||
CHECK(counting_allocator_deallocations > deallocations_before);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user