Skip to content

Return the parsed value by move from from_cbor() and friends - #5501

Merged
nlohmann merged 1 commit into
developfrom
binary-readers-move-result
Sep 10, 2026
Merged

nlohmann merged 1 commit into
developfrom
binary-readers-move-result

Conversation

@nlohmann

@nlohmann nlohmann commented Sep 6, 2026 •

Copy link
Copy Markdown
Owner

Part of the series that makes the binary readers non-recursive (#5104); this one stands on its own.

What

The binary entry points end with

return res ? result : basic_json(value_t::discarded);

The conditional operator's second operand is an lvalue, so this is not a case where the return value can be elided or implicitly moved from. Every successful from_cbor, from_msgpack, from_ubjson, from_bjdata and from_bson call deep-copies the value it just parsed, and then destroys the original.

Why it matters

basic_json's copy constructor walks the whole value, so the cost is proportional to the document. Parsing a 2 MB CBOR document with 60,000 objects, median of 25 runs, clang 17 -O3:

before after
from_cbor 26.99 ms 14.65 ms
from_msgpack 26.82 ms 14.82 ms

There is a second reason. The copy constructor recurses once per nesting level, so it is also a stack-overflow path on the return side, on a value the reader has already accepted. That is currently masked because the readers themselves recurse and overflow first, but it has to be fixed for making them iterative to have any effect — see #5104 and the rest of this stack.

API impact

No breaking changes. No signature, no return type and no observable behaviour changes; the parsed value is simply not copied on its way out. It is not used again after the return expression is evaluated.

Checklist

  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate.

🤖 Generated with Claude Code

Comment thread include/nlohmann/json.hpp Outdated
@nlohmann
nlohmann force-pushed the binary-readers-move-result branch from c3219fd to 755c547 Compare September 7, 2026 05:58
@github-actions github-actions Bot added L and removed M labels Sep 7, 2026
Comment thread include/nlohmann/json.hpp
@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Sep 7, 2026
@nlohmann nlohmann added 🚀 ready to merge Ready to merge - just waiting for CI to complete. and removed review needed It would be great if someone could review the proposed changes. labels Sep 7, 2026
@nlohmann
nlohmann force-pushed the binary-readers-move-result branch from 755c547 to ea02fda Compare September 8, 2026 11:13
@nlohmann nlohmann added this to the Release 3.13.0 milestone Sep 9, 2026
The binary entry points end with

    return res ? result : basic_json(value_t::discarded);

The condition operator's second operand is an lvalue, so this is not a case
where the return value can be elided or implicitly moved from: every
successful from_cbor(), from_msgpack(), from_ubjson(), from_bjdata() and
from_bson() call deep-copies the value it just parsed, and then destroys the
original.

The copy is not cheap, and it is not incidental: basic_json's copy
constructor walks the whole value. Parsing a 2 MB CBOR document with 60,000
objects, median of 25 runs, clang 17 -O3:

    from_cbor      26.99 ms  ->  14.65 ms
    from_msgpack   26.82 ms  ->  14.82 ms

Moving instead of copying is the entire change; the parsed value is not used
again after the return expression is evaluated.

There is a second reason to prefer the move. The copy constructor recurses
once per nesting level, so the copy is also a stack-overflow path on the
return side, on a value the reader has already accepted. That is currently
masked because the readers themselves recurse and overflow first (#5104), but
it has to be fixed for making them iterative to have any effect.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
@nlohmann
nlohmann force-pushed the binary-readers-move-result branch from ea02fda to 79f990d Compare September 9, 2026 08:21
@nlohmann
nlohmann merged commit d69fb86 into develop Sep 10, 2026
158 checks passed
@nlohmann
nlohmann deleted the binary-readers-move-result branch September 10, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aspect: binary formats BSON, CBOR, MessagePack, UBJSON L performance 🚀 ready to merge Ready to merge - just waiting for CI to complete.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants