Skip to content

Reserve capacity when converting JSON objects to maps - #5434

Open
22elix3r wants to merge 1 commit into
nlohmann:developfrom
22elix3r:fix/5406-from-json-object-reserve
Open

Reserve capacity when converting JSON objects to maps#5434
22elix3r wants to merge 1 commit into
nlohmann:developfrom
22elix3r:fix/5406-from-json-object-reserve

Conversation

@22elix3r

Copy link
Copy Markdown
Contributor

Summary

`from_json` into `std::unordered_map` (and other reservable object types) never called `reserve()`, so a 5k-entry object rehashed repeatedly. The array conversion path already reserves when `reserve` exists.

Related Issue

Fixes #5406

Changes Made

  • SFINAE `try_reserve` helper used by the constructible-object `from_json` overload
  • Test converting a 200-key object to `unordered_map<string,int>`

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-conversions_cpp11`
  • `./tests/test-conversions_cpp11 --no-skip -tce='downloaded'`

Results:

  • 3 passed, 262 assertions

Notes

`std::map` has no `reserve`; the helper is a no-op there.

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

from_json into unordered_map and other reservable object types
emplaced one entry at a time and rehashed as it grew. The array path
already reserved; do the same when Container::reserve exists.

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

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.

Comment on lines +98 to +110
SECTION("std::unordered_map reserve path with many keys")
{
json big;
for (int i = 0; i < 200; ++i)
{
big[std::to_string(i)] = i;
}
const auto o = big.get<std::unordered_map<std::string, int>>();
CHECK(o.size() == 200);
CHECK(o.at("0") == 0);
CHECK(o.at("199") == 199);
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This test is not really testing the reserving part. Maybe you could explicitly check capacity() here?

Copy link
Copy Markdown
Owner

The reserve works — measured on #5406's own benchmark (5,000-entry object → std::unordered_map), this goes from 5009 to 5001 allocations, i.e. exactly the 8 rehash reallocations the issue predicts. std::map targets correctly fall through to the no-op overload.

One deviation from the ticket. #5406 asks for the object path to mirror the array path:

Add a reserve-detecting overload for the object path, mirroring the array path's priority_tag SFINAE

This instead introduces a new detail::try_reserve with an int / ... ranked-overload pair. Functionally equivalent, but it sits a few hundred lines above from_json_array_impl (from_json.hpp:289), which already solves the same problem with priority_tag — so the file now carries two different idioms for one job, and try_reserve is a fairly generic name to add to detail. Reusing the existing priority_tag pattern would keep it consistent.


Generated by Claude Code

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

from_json into a std::unordered_map (and other reservable object types) never reserves, causing rehashing (~13% slower get<unordered_map>)

3 participants