Repository navigation
Fix quadratic parent updates for ordered_json with JSON_DIAGNOSTICS - #5832
Conversation
|
Reworked in c1d3493 (written by Claude Code, on Niels's behalf):
unit-diagnostics passes. unit-modifiers and unit-ordered_json pass with and without |
c1d3493 to
1e18938
Compare
nlohmann
added
the
review needed
1e18938 to
8516093
Compare
8516093 to
ebab8a1
Compare
| static std::size_t capacity_of(const C& /*container*/) noexcept | ||
| { | ||
| return detail::unknown_size(); | ||
| return std::is_same<C, array_t>::value || detail::is_ordered_map<C>::value ? detail::unknown_size() : 0; |
There was a problem hiding this comment.
Should detail::unknown_size() be the default unless the container is known to not move? In other words, we check for containers that we know don't move and return "may move" for everything else, rather than checking for containers that we know move, and return "won't move" for everything else?
There was a problem hiding this comment.
Agreed, and it was worse than it looked: is_ordered_map is just has_capacity, so in that overload the check was always false, and every object type without capacity() counted as "never moves". Fixed in 80692a1. Only std::map, std::multimap, std::unordered_map and std::unordered_multimap count as stable (new detail::has_stable_elements), and only the real nlohmann::ordered_map has its capacity tracked (new detail::is_nlohmann_ordered_map). Everything else is treated as "may move". That also covers a sorted flat map that has capacity() but shifts members on a middle insert. The new test uses an object type that moves its members without changing its capacity; it fails without the change.
(Written by Claude Code on my behalf.)
There was a problem hiding this comment.
Similar comment to https://github.com/nlohmann/json/pull/5832/changes#r4285291400
There was a problem hiding this comment.
Done. set_parents_after_object_erase() now re-parents unless has_stable_elements<object_t>. After this PR, set_parent() had no callers left, so I removed it instead of changing it (80692a1).
(Written by Claude Code on my behalf.)
| // set_parent() also repairs the other members, which ordered_json | ||
| // relocates when adding a key makes its vector grow | ||
| set_parent(m_data.m_value.object->operator[](it.key()) = std::move(it.value())); | ||
| // operator[] repairs the other members if ordered_json relocates |
There was a problem hiding this comment.
Should these all say "the container" rather than "ordered_json"?
There was a problem hiding this comment.
Yes, reworded to "the object type" in both update() comments (80692a1).
(Written by Claude Code on my behalf.)
80692a1 to
a88d94f
Compare
Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Address review: decide whether an insertion may have moved the other members by naming the containers that are known not to, instead of the ones that are known to. - detail::has_stable_elements: std::map, std::multimap, std::unordered_map and std::unordered_multimap; object_capacity() reports 0 for them. - detail::is_nlohmann_ordered_map: only the real nlohmann::ordered_map (which only appends) reports its capacity(). - Any other object type reports unknown_size(), so it gets a full set_parents() on every insertion. Before, an object type without capacity() was treated as never moving, and one with capacity() (a sorted flat map, say) was trusted to move only on reallocation. - set_parents_after_object_erase() re-parents unless the object type is stable; the unused set_parent() is removed. - Comments say "the object type" instead of "ordered_json". - New test with an object type that moves its members without changing its capacity; it fails without this change. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
ed76b17 to
608808b
Compare
These PRs were merged while their CI runs were cancelled, so the warnings went unnoticed. All of them fail the -Werror builds: - json_pointer.hpp (#5836): size_t += int under GCC -Wsign-conversion - binary_reader.hpp (#5839): char_int_type passed as char under GCC -Wconversion - json.hpp (#5838): @throw without a description under clang -Wdocumentation - string_escape.hpp (#5836): escape_into() was inserted between escape() and its documentation, so the @return was attached to a void function - unit-diagnostics.cpp (#5832): clang -Wnrvo in a lambda without an explicit return type - unit-to_json-rvalue.cpp (#5837): GCC -Wnoexcept (C++20) and clang -Wunneeded-member-function on the test type Signed-off-by: Niels Lohmann <mail@nlohmann.me>
- object_capacity() (#5832) overloaded a static with a non-static member function template, which MSVC 2015 rejects (C2686); make all three overloads non-static. - The JSON pointer hash constants (#5836) truncate on 32-bit size_t (C4309 with /WX); mask them to size_t's range before the cast. - unit-diagnostics: MSVC 2015 cannot use-declare the base's Container typedef (C2886); use alias declarations instead. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
With
JSON_DIAGNOSTICS=1,set_parent()called the fullset_parents()forordered_jsonon every call:push_back/emplace_back,at(), andoperator[]/at()on existing keys. Building anordered_jsonarray was therefore quadratic.Changes
at()and array append set only the one child'sm_parent.operator[]/push_back/emplacerecord the container capacity before the insert and callset_parents()only if it changed. Arrays and objects share onecontainer_capacity()and oneset_parent_after_insert()helper (arrays report theircapacity(); for objects, only the node-based standard containers (std::map,std::multimap,std::unordered_map,std::unordered_multimap) report 0 = never moves, onlynlohmann::ordered_mapreports itscapacity(), and any other object type reports unknown = always re-parent).update()(all overloads, incl.merge_objectsand the iterative variant) now assigns throughoperator[]instead of callingset_parent()per key;merge_patch()already goes throughoperator[], so it benefits too.erasekeeps itsset_parents_after_object_erase()(O(n) forordered_mapanyway).set_parents()in the copy constructor (copy_level/copy_iterativelyalready re-parent each level). The move constructor keeps itsset_parents()(required).jsonandordered_json) across parse, copy, move, copy-assign, swap, push_back/emplace_back/emplace/operator[] growth andat();update()(both overloads,merge_objects) andmerge_patch()onjson/ordered_jsonchecking parents and diagnostic paths. No timing test (would be flaky).Numbers (ordered_json, nested-array push_back, -O2, diagnostics on)
ordered_json::update()with 20k new keys: 372 → 295 ms (no diagnostics: 259 ms), i.e. the diagnostics overhead is gone; the rest isordered_map's inherent linear key lookup (100k keys: 14.0 → 10.0 s).at(i)x10000: 31.7 → 0.017 ms. Parse unchanged; copies −5–8 % with diagnostics.Breaking changes
None. No public API change; behaviour without
JSON_DIAGNOSTICSis unchanged.Tested with
-DJSON_DIAGNOSTICS=1: unit-diagnostics, unit-ordered_json, unit-element_access1/2, unit-modifiers.make amalgamaterun.