From fcf23660629b96a984a61c2d5039d219cb99f8cd Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Tue, 20 Oct 2026 07:56:23 +0200 Subject: [PATCH] Address the rest of the review of editable json documents From gregmarr's review of #5630: - alloc_nodes() gives a request of half of the next chunk or more a chunk of its own, so that the rest of the current chunk stays available for smaller requests instead of being abandoned. - The comments on edit_state, append_text(), and reserve_moved() say that views stay valid across every edit (not just the first), that n is 0 for empty strings, and why both capacities are checked. - The architecture page names flag bits 3, 4, and 5 (linked) separately and says that a discarded view has no node; erase.md rewords overload 3 and says why it never throws invalid_iterator.202. Signed-off-by: Niels Lohmann --- .../docs/api/basic_json_document/erase.md | 12 +++---- docs/mkdocs/docs/home/architecture.md | 6 ++-- .../nlohmann/detail/view/document_data.hpp | 10 +++--- include/nlohmann/detail/view/edit_storage.hpp | 25 ++++++++++--- single_include/nlohmann/json_view.hpp | 35 ++++++++++++++----- tests/src/unit-json_view_edit.cpp | 24 +++++++++++++ 6 files changed, 86 insertions(+), 26 deletions(-) diff --git a/docs/mkdocs/docs/api/basic_json_document/erase.md b/docs/mkdocs/docs/api/basic_json_document/erase.md index 5f6b7fc0a..4cbc50d19 100644 --- a/docs/mkdocs/docs/api/basic_json_document/erase.md +++ b/docs/mkdocs/docs/api/basic_json_document/erase.md @@ -18,10 +18,10 @@ has `erase`; calling it on a read-only `basic_json_document` fails to compile (` 1. Removes every member of `object` whose key is `key` (see [Notes](#notes) on duplicate keys) and returns how many were removed; `#!cpp 0` if `object` has no member with this key. 2. Removes the element at index `idx` of `array`, which must already exist (`#!cpp idx < array.size()`). -3. Removes the value the JSON pointer `ptr` refers to, relative to [`root()`](root.md), and returns how many values - were removed: the *parent* of the target must already exist, and the target itself is removed as in 1. (an object - member; `#!cpp 0` or more) or 2. (an array element; always `#!cpp 1`). `ptr` must not be empty -- [`root()`](root.md) - itself cannot be erased. +3. Removes the value the JSON pointer `ptr` refers to, relative to [`root()`](root.md). `ptr` must not be empty + ([`root()`](root.md) itself cannot be erased), and the *parent* of the target must already exist. The target is + removed as in 1. if the parent is an object (returns the number of removed members, `#!cpp 0` or more) and as in 2. + if it is an array (always returns `#!cpp 1`). ## Template parameters @@ -71,8 +71,8 @@ has `erase`; calling it on a read-only `basic_json_document` fails to compile (` Overloads 1. and 2. also throw [`invalid_iterator.202`](../../home/exceptions.md#jsonexceptioninvalid_iterator202) ("view does not belong to this document") if `object`/`array` is a [discarded](../basic_json_view/is_discarded.md) -view or a view of a *different* document. Overload 3. takes no view: it always starts from this document's own -[`root()`](root.md). +view or a view of a *different* document. Overload 3. never throws `invalid_iterator.202`: it takes no view, and it +always starts from this document's own [`root()`](root.md). ## Complexity diff --git a/docs/mkdocs/docs/home/architecture.md b/docs/mkdocs/docs/home/architecture.md index f66982978..5ee7fcc5d 100644 --- a/docs/mkdocs/docs/home/architecture.md +++ b/docs/mkdocs/docs/home/architecture.md @@ -194,8 +194,8 @@ packet-beta | Bytes | Field | Type | Contents | |-------|---------|------------|-------------------------------------------------------------------------------------------------------------------------------| -| 0 | `kind` | `uint8_t` | the type, numbered as [`value_t`](../api/basic_json/value_t.md): 0 null, 1 object, 2 array, 3 string, 4 boolean, 5 signed integer, 6 unsigned integer, 7 float; 10 for a link (see below) | -| 1 | `flags` | `uint8_t` | bits 0-1: where a string's bytes (or a number's token) are (0: the source text, 1: the buffer of decoded strings, for strings with escapes, 2: the edit buffer); bit 2: the value of a boolean; bits 3 and 4: moved and new (see below) | +| 0 | `kind` | `uint8_t` | the type, numbered as [`value_t`](../api/basic_json/value_t.md): 0 null, 1 object, 2 array, 3 string, 4 boolean, 5 signed integer, 6 unsigned integer, 7 float; 10 for a link (see below). There is no node for a discarded value: a [discarded](../api/basic_json_view/is_discarded.md) view has none | +| 1 | `flags` | `uint8_t` | bits 0-1: where a string's bytes (or a number's token) are (0: the source text, 1: the buffer of decoded strings, for strings with escapes, 2: the edit buffer); bit 2: the value of a boolean; bit 3: moved, bit 4: new, bit 5: linked (see below) | | 2-3 | `extra` | `uint16_t` | numbers: the number of integer digits (low byte) and fraction digits (high byte), 255 for more; objects: the number of their key hash index (1-based), or 0; otherwise 0 | | 4-7 | `off` | `uint32_t` | where the value starts: the first byte after a string's opening quote (or its position in the buffer of decoded strings), the first byte of a number or literal, the bracket of an array or object | | 8-11 | `len` | `uint32_t` | strings: the length after decoding; floats and literals: the length of the token; arrays and objects: the number of elements | @@ -257,6 +257,8 @@ edited: sequence (a header node, then the entries), whose number is in `off`. The entries are links (`kind` 10), whose bytes 8-15 hold the address of the value's node, so values never move. - A node written by an edit gets the flag *new* (bit 4): it has no position in the source text. +- A value that an entry of such a sequence links to gets the flag *linked* (bit 5): its extent in the parsed layout + (`next`) no longer matters, so a scalar can replace an array or object there in place. - Views of read-only documents compile without any of this: how views walk the index is a template parameter (`navigation`). diff --git a/include/nlohmann/detail/view/document_data.hpp b/include/nlohmann/detail/view/document_data.hpp index c36f2e835..e31400a1e 100644 --- a/include/nlohmann/detail/view/document_data.hpp +++ b/include/nlohmann/detail/view/document_data.hpp @@ -58,10 +58,12 @@ struct document_data bool discarded = true; /// The storage of edits (editable documents only; see edit_storage.hpp). - /// Edits never move or resize the parsed index, so views stay valid: an - /// array/object whose elements change gets node_flags::moved, and its - /// elements then live in a separate sequence (a header node, then the - /// entries), whose entries link to the values. + /// No edit moves or resizes the parsed index, and the nodes of values + /// written by edits never move either, so a view stays valid whenever it + /// was taken, before the first edit or between later ones: an array/object + /// whose elements change gets node_flags::moved, and its elements then live + /// in a separate sequence (a header node, then the entries), whose entries + /// link to the values. struct edit_state { std::vector moved{}; ///< element sequences of moved arrays/objects (header node first) // NOLINT(readability-redundant-member-init) diff --git a/include/nlohmann/detail/view/edit_storage.hpp b/include/nlohmann/detail/view/edit_storage.hpp index a4de44112..614a0fb0d 100644 --- a/include/nlohmann/detail/view/edit_storage.hpp +++ b/include/nlohmann/detail/view/edit_storage.hpp @@ -50,7 +50,19 @@ inline node* alloc_nodes(document_data& d, std::size_t k) document_data::edit_state& e = edit_state_of(d); if (NLOHMANN_VIEW_UNLIKELY(static_cast(e.chunk_end - e.chunk_cur) < k)) { - const std::size_t count = (std::max)(k, e.chunk_next); + if (k >= e.chunk_next / 2) + { + // a large request (half of the next chunk or more) gets a chunk of + // its own, so that the rest of the current chunk stays available + // for smaller ones + std::unique_ptr own(new node[k]()); // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays) + e.chunks.push_back(std::move(own)); + e.bytes += k * sizeof(node); + return e.chunks.back().get(); + } + // a smaller request leaves the rest of the current chunk unused + // (fewer than k nodes, and k is less than half of the new chunk) + const std::size_t count = e.chunk_next; std::unique_ptr fresh(new node[count]()); // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays) e.chunks.push_back(std::move(fresh)); e.chunk_cur = e.chunks.back().get(); @@ -81,7 +93,8 @@ inline std::size_t text_capacity(std::size_t cap, std::size_t used, std::size_t } /// copy n bytes into the edit arena and return their offset; a new buffer -/// leaves the old one alive, so that string views into it remain valid +/// leaves the old one alive, so that string views into it remain valid; n is +/// 0 for an empty string (written by an edit, or the key of a new member) inline std::uint32_t append_text(document_data& d, const char* s, std::size_t n) { document_data::edit_state& e = edit_state_of(d); @@ -89,7 +102,7 @@ inline std::uint32_t append_text(document_data& d, const char* s, std::size_t n) { const std::size_t cap = text_capacity(e.text_cap, e.text_used, n); std::unique_ptr fresh(new char[cap]); // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays) - if (e.text_used != 0) + if (NLOHMANN_VIEW_LIKELY(e.text_used != 0)) // (0 only for the first buffer, and then texts is empty) { std::memcpy(fresh.get(), e.texts.back().get(), e.text_used); } @@ -127,8 +140,10 @@ inline void reserve_moved(document_data& d) if (e.moved.size() == e.moved.capacity() || e.moved_cap.size() == e.moved_cap.capacity()) { // both grow before either changes, so that the push_backs cannot throw - e.moved.reserve((2 * e.moved.size()) + 16); - e.moved_cap.reserve((2 * e.moved.size()) + 16); + // (both conditions are needed: reserve() may allocate more than asked, + // so the two capacities can differ) + e.moved.reserve((e.moved.size() * 2) + 16); + e.moved_cap.reserve((e.moved.size() * 2) + 16); } } diff --git a/single_include/nlohmann/json_view.hpp b/single_include/nlohmann/json_view.hpp index 0dd1cd1fd..f38ae4413 100644 --- a/single_include/nlohmann/json_view.hpp +++ b/single_include/nlohmann/json_view.hpp @@ -372,10 +372,12 @@ struct document_data bool discarded = true; /// The storage of edits (editable documents only; see edit_storage.hpp). - /// Edits never move or resize the parsed index, so views stay valid: an - /// array/object whose elements change gets node_flags::moved, and its - /// elements then live in a separate sequence (a header node, then the - /// entries), whose entries link to the values. + /// No edit moves or resizes the parsed index, and the nodes of values + /// written by edits never move either, so a view stays valid whenever it + /// was taken, before the first edit or between later ones: an array/object + /// whose elements change gets node_flags::moved, and its elements then live + /// in a separate sequence (a header node, then the entries), whose entries + /// link to the values. struct edit_state { std::vector moved{}; ///< element sequences of moved arrays/objects (header node first) // NOLINT(readability-redundant-member-init) @@ -2874,7 +2876,19 @@ inline node* alloc_nodes(document_data& d, std::size_t k) document_data::edit_state& e = edit_state_of(d); if (NLOHMANN_VIEW_UNLIKELY(static_cast(e.chunk_end - e.chunk_cur) < k)) { - const std::size_t count = (std::max)(k, e.chunk_next); + if (k >= e.chunk_next / 2) + { + // a large request (half of the next chunk or more) gets a chunk of + // its own, so that the rest of the current chunk stays available + // for smaller ones + std::unique_ptr own(new node[k]()); // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays) + e.chunks.push_back(std::move(own)); + e.bytes += k * sizeof(node); + return e.chunks.back().get(); + } + // a smaller request leaves the rest of the current chunk unused + // (fewer than k nodes, and k is less than half of the new chunk) + const std::size_t count = e.chunk_next; std::unique_ptr fresh(new node[count]()); // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays) e.chunks.push_back(std::move(fresh)); e.chunk_cur = e.chunks.back().get(); @@ -2905,7 +2919,8 @@ inline std::size_t text_capacity(std::size_t cap, std::size_t used, std::size_t } /// copy n bytes into the edit arena and return their offset; a new buffer -/// leaves the old one alive, so that string views into it remain valid +/// leaves the old one alive, so that string views into it remain valid; n is +/// 0 for an empty string (written by an edit, or the key of a new member) inline std::uint32_t append_text(document_data& d, const char* s, std::size_t n) { document_data::edit_state& e = edit_state_of(d); @@ -2913,7 +2928,7 @@ inline std::uint32_t append_text(document_data& d, const char* s, std::size_t n) { const std::size_t cap = text_capacity(e.text_cap, e.text_used, n); std::unique_ptr fresh(new char[cap]); // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays) - if (e.text_used != 0) + if (NLOHMANN_VIEW_LIKELY(e.text_used != 0)) // (0 only for the first buffer, and then texts is empty) { std::memcpy(fresh.get(), e.texts.back().get(), e.text_used); } @@ -2951,8 +2966,10 @@ inline void reserve_moved(document_data& d) if (e.moved.size() == e.moved.capacity() || e.moved_cap.size() == e.moved_cap.capacity()) { // both grow before either changes, so that the push_backs cannot throw - e.moved.reserve((2 * e.moved.size()) + 16); - e.moved_cap.reserve((2 * e.moved.size()) + 16); + // (both conditions are needed: reserve() may allocate more than asked, + // so the two capacities can differ) + e.moved.reserve((e.moved.size() * 2) + 16); + e.moved_cap.reserve((e.moved.size() * 2) + 16); } } diff --git a/tests/src/unit-json_view_edit.cpp b/tests/src/unit-json_view_edit.cpp index d47e18dae..7ed70fb88 100644 --- a/tests/src/unit-json_view_edit.cpp +++ b/tests/src/unit-json_view_edit.cpp @@ -26,6 +26,7 @@ using ptr_t = ordered_json::json_pointer; #include #include #include +#include #include #include #include @@ -915,6 +916,29 @@ TEST_CASE("json_view edits: the size of a text arena") } #endif +TEST_CASE("json_view edits: the chunks of new nodes") +{ + using nlohmann::detail::view::alloc_nodes; + using nlohmann::detail::view::document_data; + const std::unique_ptr d(document_data::create(0)); + // the first chunk has room for 64 nodes, the next one for 128 + const nlohmann::detail::view::node* const first = alloc_nodes(*d, 10); + CHECK(d->edits->chunks.size() == 1); + // a request of half of the next chunk (128 nodes) or more gets a chunk of + // its own + const nlohmann::detail::view::node* const large = alloc_nodes(*d, 100); + CHECK(d->edits->chunks.size() == 2); + CHECK(large == d->edits->chunks.back().get()); + // and the rest of the first chunk is still used + CHECK(alloc_nodes(*d, 50) == first + 10); + CHECK(d->edits->chunks.size() == 2); + // a smaller request that does not fit starts a new (doubled) chunk + CHECK(alloc_nodes(*d, 10) == d->edits->chunks.back().get()); + CHECK(d->edits->chunks.size() == 3); + CHECK(d->edits->chunk_end - d->edits->chunk_cur == 128 - 10); + CHECK(d->edits->bytes == (64 + 100 + 128) * sizeof(nlohmann::detail::view::node)); +} + TEST_CASE("json_view edits: replacing arrays and objects by scalars") { // the first assignment switches the parent to links; the later ones do