Skip to content

Allocate dump() indent buffer only for pretty-print - #5430

Closed
22elix3r wants to merge 1 commit into
nlohmann:developfrom
22elix3r:fix/5413-dump-lazy-indent-string
Closed

22elix3r wants to merge 1 commit into
nlohmann:developfrom
22elix3r:fix/5413-dump-lazy-indent-string

Conversation

@22elix3r

Copy link
Copy Markdown
Contributor

Summary

Every dump() constructed a 512-byte indent_string even for compact output (indent = -1), where the buffer is never read. That per-call heap allocation showed up as ~17% overhead on dump-many-small-values.

Related Issue

Fixes #5413

Changes Made

  • Default-construct indent_string empty
  • Grow it on demand in ensure_indent() using indent_char (the previous doubling path filled with ' ', which was wrong for a custom indent character)
  • Tests for compact dump, dump(2), and dump(1, '\\t')

The make_shared output-adapter allocation from the issue is left for a follow-up; it is a larger adapter API change.

Testing

Commands executed:

  • python3 tools/amalgamate/amalgamate.py -c tools/amalgamate/config_json.json -s .
  • cmake -S . -B build -DJSON_BuildTests=ON -DCMAKE_BUILD_TYPE=Debug
  • cmake --build build --target test-serialization_cpp11
  • ./tests/test-serialization_cpp11 --no-skip -tce='*downloaded*'
  • ./tests/test-serialization_cpp11 --no-skip -tce='*downloaded*' -tc='*5413*'

Results:

Notes

Pretty-print still allocates a 512-byte (or larger) indent buffer the first time a nested indent is needed. Compact dump allocates none.

  • 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.

serializer always constructed a 512-byte indent_string, including
compact dump() which never reads it. Grow the buffer on first
pretty-print instead, using indent_char rather than a hardcoded space.

Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
@22elix3r
22elix3r requested a review from nlohmann as a code owner August 27, 2026 02:26
Copilot AI lite review requested due to automatic review settings August 27, 2026 02:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gregmarr

Copy link
Copy Markdown
Contributor

FYI, there is an open PR that removes this buffer entirely.

@22elix3r

Copy link
Copy Markdown
Contributor Author

Thanks — that is #5285 (and the related overflow work in #5186). This PR is a smaller, independent change: skip the 512-byte indent_string allocation on compact dump(). If #5285 lands first this becomes redundant and can be closed; until then it still helps compact dump() on current develop.

@github-actions

Copy link
Copy Markdown

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated and/or formatted correctly.

📎 A ready-to-apply patch is attached to the failed workflow run as the amalgamation-patch artifact. Download it, then apply it locally from the repository root with:

git apply amalgamation.patch

This does not require installing astyle yourself.

@nlohmann

nlohmann commented Sep 2, 2026

Copy link
Copy Markdown
Owner

@22elix3r CI is red: the build fails to compile with serializer.hpp:992:27: error: no member named 'assign' in 'alt_string' (the custom-string-type test), and Fuzzing, CodeQL, and the amalgamation check are also failing.

@nlohmann

nlohmann commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Thanks for this — and good catch on the indent-buffer regrowth using a hardcoded space fill instead of the configured indent_char past 512 bytes.

This turns out to be superseded by already-in-progress maintainer work: #5285 (part of an active, currently-green stack) removes the indent_string allocation entirely rather than just deferring it, replacing it with a stack-resident write buffer — so the bug this PR fixes can't recur there, and we don't need a separate fix for it. We closed our own equivalent PR (#5483) for the same reason and are moving forward with #5285 instead. Appreciate you tracking down the fill-character issue regardless.

— closed by Claude Code on behalf of @nlohmann

@nlohmann nlohmann closed this Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

4 participants