From 8e3286aac8e8384540d901fb1db483527baf812d Mon Sep 17 00:00:00 2001 From: Benoit Blanchon Date: Mon, 24 Feb 2025 15:35:09 +0100 Subject: [PATCH] Store static strings in a dedicated pool Because a slot id is smaller than a pointer, this change will ultimately allow reducing the slot size. --- CHANGELOG.md | 37 +++++++++++++++ extras/tests/Deprecated/BasicJsonDocument.cpp | 2 +- extras/tests/Helpers/Allocators.hpp | 6 +++ extras/tests/JsonArray/add.cpp | 1 + .../JsonDeserializer/destination_types.cpp | 4 +- extras/tests/JsonDeserializer/filter.cpp | 8 ++-- extras/tests/JsonDocument/ElementProxy.cpp | 1 + extras/tests/JsonDocument/MemberProxy.cpp | 9 +++- extras/tests/JsonDocument/add.cpp | 1 + extras/tests/JsonDocument/constructor.cpp | 2 + extras/tests/JsonDocument/remove.cpp | 4 +- extras/tests/JsonDocument/set.cpp | 4 +- extras/tests/JsonDocument/shrinkToFit.cpp | 19 ++++++-- extras/tests/JsonDocument/subscript.cpp | 3 +- extras/tests/JsonObject/set.cpp | 22 ++------- extras/tests/JsonObject/subscript.cpp | 18 ++++--- extras/tests/JsonVariant/copy.cpp | 6 ++- extras/tests/JsonVariant/set.cpp | 22 ++++++++- extras/tests/JsonVariantConst/subscript.cpp | 2 +- extras/tests/Misc/StringAdapters.cpp | 2 +- .../MsgPackDeserializer/destination_types.cpp | 4 +- extras/tests/ResourceManager/CMakeLists.txt | 1 + extras/tests/ResourceManager/StringBuffer.cpp | 8 ++-- .../tests/ResourceManager/StringBuilder.cpp | 27 ++++++----- .../ResourceManager/saveStaticString.cpp | 47 +++++++++++++++++++ src/ArduinoJson/Memory/MemoryPool.hpp | 13 +++++ src/ArduinoJson/Memory/MemoryPoolList.hpp | 9 ++++ src/ArduinoJson/Memory/ResourceManager.hpp | 25 +++++++++- src/ArduinoJson/Object/JsonPair.hpp | 4 +- src/ArduinoJson/Object/ObjectImpl.hpp | 2 +- .../Strings/Adapters/RamString.hpp | 5 +- src/ArduinoJson/Strings/JsonString.hpp | 3 +- src/ArduinoJson/Variant/ConverterImpl.hpp | 4 +- src/ArduinoJson/Variant/VariantContent.hpp | 3 -- src/ArduinoJson/Variant/VariantData.hpp | 19 ++++---- src/ArduinoJson/Variant/VariantImpl.hpp | 26 ++++++++-- 36 files changed, 282 insertions(+), 91 deletions(-) create mode 100644 extras/tests/ResourceManager/saveStaticString.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index a0a7a6f6..3751c13b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,43 @@ ArduinoJson: change log ======================= +HEAD +---- + +* Optimize storage of static strings + +> ### BREAKING CHANGES +> +> Static string cannot contain NUL characters anymore (they could since 7.3.0). +> This is an extremely rare case, so you probably won't be affected. +> +> For example, the following code produces different output in 7.3 and 7.4: +> +> ```cpp +> JsonDocument doc; +> doc["a\0b"] = "c\0d"; +> serializeJson(doc, Serial); +> // With Arduino 7.3 -> {"a\u0000b":"c\u0000d"} +> // With Arduino 7.4 -> {"a":"c"} +> ``` +> +> `JsonString` contructor now only accepts two arguments, not three. +> If your code uses `JsonString` to store a string as a pointer, you must remove the size argument. +> +> For example, if you have something like this: +> +> ```cpp +> doc["key"] = JsonString(str.c_str(), str.size(), true); +> ``` +> +> You must replace with either: +> +> ```cpp +> doc["key"] = JsonString(str.c_str(), true); // store as pointer, cannot contain NUL characters +> doc["key"] = JsonString(str.c_str(), str.size()); // store by copy, NUL characters allowed +> doc["key"] = str; // same as previous line for supported string classes (`String`, `std::string`, etc.) +> ``` + v7.4.2 (2025-06-20) ------ diff --git a/extras/tests/Deprecated/BasicJsonDocument.cpp b/extras/tests/Deprecated/BasicJsonDocument.cpp index a3509779..a5d9f54d 100644 --- a/extras/tests/Deprecated/BasicJsonDocument.cpp +++ b/extras/tests/Deprecated/BasicJsonDocument.cpp @@ -54,7 +54,7 @@ TEST_CASE("BasicJsonDocument") { doc["hello"] = "world"; auto copy = doc; REQUIRE(copy.as() == "{\"hello\":\"world\"}"); - REQUIRE(allocatorLog == "AA"); + REQUIRE(allocatorLog == "AAAA"); } SECTION("capacity") { diff --git a/extras/tests/Helpers/Allocators.hpp b/extras/tests/Helpers/Allocators.hpp index 17e05cab..6b65ad01 100644 --- a/extras/tests/Helpers/Allocators.hpp +++ b/extras/tests/Helpers/Allocators.hpp @@ -275,6 +275,12 @@ inline size_t sizeofPool( return MemoryPool::slotsToBytes(n); } +inline size_t sizeofStaticStringPool( + ArduinoJson::detail::SlotCount n = ARDUINOJSON_POOL_CAPACITY) { + using namespace ArduinoJson::detail; + return MemoryPool::slotsToBytes(n); +} + inline size_t sizeofStringBuffer(size_t iteration = 1) { // returns 31, 63, 127, 255, etc. auto capacity = ArduinoJson::detail::StringBuilder::initialCapacity; diff --git a/extras/tests/JsonArray/add.cpp b/extras/tests/JsonArray/add.cpp index 0983e3bd..bb64ef06 100644 --- a/extras/tests/JsonArray/add.cpp +++ b/extras/tests/JsonArray/add.cpp @@ -56,6 +56,7 @@ TEST_CASE("JsonArray::add(T)") { REQUIRE(array[0].is() == false); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } diff --git a/extras/tests/JsonDeserializer/destination_types.cpp b/extras/tests/JsonDeserializer/destination_types.cpp index 0f96b23e..2a51c411 100644 --- a/extras/tests/JsonDeserializer/destination_types.cpp +++ b/extras/tests/JsonDeserializer/destination_types.cpp @@ -104,6 +104,8 @@ TEST_CASE("deserializeJson(MemberProxy)") { REQUIRE(err == DeserializationError::Ok); REQUIRE(doc.as() == "{\"hello\":\"world\",\"value\":[42]}"); - REQUIRE(spy.log() == AllocatorLog{}); + REQUIRE(spy.log() == AllocatorLog{ + Allocate(sizeofStaticStringPool()), + }); } } diff --git a/extras/tests/JsonDeserializer/filter.cpp b/extras/tests/JsonDeserializer/filter.cpp index 4f9d9b9b..d57ea962 100644 --- a/extras/tests/JsonDeserializer/filter.cpp +++ b/extras/tests/JsonDeserializer/filter.cpp @@ -825,7 +825,9 @@ TEST_CASE("shrink filter") { deserializeJson(doc, "{}", DeserializationOption::Filter(filter)); - REQUIRE(spy.log() == AllocatorLog{ - Reallocate(sizeofPool(), sizeofObject(1)), - }); + REQUIRE(spy.log() == + AllocatorLog{ + Reallocate(sizeofPool(), sizeofObject(1)), + Reallocate(sizeofStaticStringPool(), sizeofStaticStringPool(1)), + }); } diff --git a/extras/tests/JsonDocument/ElementProxy.cpp b/extras/tests/JsonDocument/ElementProxy.cpp index 9cf88551..1da97c7d 100644 --- a/extras/tests/JsonDocument/ElementProxy.cpp +++ b/extras/tests/JsonDocument/ElementProxy.cpp @@ -31,6 +31,7 @@ TEST_CASE("ElementProxy::add()") { REQUIRE(doc.as() == "[[\"world\"]]"); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } diff --git a/extras/tests/JsonDocument/MemberProxy.cpp b/extras/tests/JsonDocument/MemberProxy.cpp index c42edcc1..816054f4 100644 --- a/extras/tests/JsonDocument/MemberProxy.cpp +++ b/extras/tests/JsonDocument/MemberProxy.cpp @@ -25,6 +25,7 @@ TEST_CASE("MemberProxy::add()") { REQUIRE(doc.as() == "{\"hello\":[42]}"); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } @@ -34,6 +35,7 @@ TEST_CASE("MemberProxy::add()") { REQUIRE(doc.as() == "{\"hello\":[\"world\"]}"); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } @@ -44,6 +46,7 @@ TEST_CASE("MemberProxy::add()") { REQUIRE(doc.as() == "{\"hello\":[\"world\"]}"); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), Allocate(sizeofString("world")), }); } @@ -55,8 +58,8 @@ TEST_CASE("MemberProxy::add()") { REQUIRE(doc.as() == "{\"hello\":[\"world\"]}"); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), Allocate(sizeofString("world")), - }); } @@ -71,6 +74,7 @@ TEST_CASE("MemberProxy::add()") { REQUIRE(doc.as() == "{\"hello\":[\"world\"]}"); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), Allocate(sizeofString("world")), }); } @@ -399,7 +403,7 @@ TEST_CASE("MemberProxy under memory constraints") { } SECTION("value slot allocation fails") { - timebomb.setCountdown(1); + timebomb.setCountdown(2); // fill the pool entirely, but leave one slot for the key doc["foo"][ARDUINOJSON_POOL_CAPACITY - 4] = 1; @@ -412,6 +416,7 @@ TEST_CASE("MemberProxy under memory constraints") { REQUIRE(doc.overflowed() == true); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), AllocateFail(sizeofPool()), }); } diff --git a/extras/tests/JsonDocument/add.cpp b/extras/tests/JsonDocument/add.cpp index da898e66..b7f442a9 100644 --- a/extras/tests/JsonDocument/add.cpp +++ b/extras/tests/JsonDocument/add.cpp @@ -32,6 +32,7 @@ TEST_CASE("JsonDocument::add(T)") { REQUIRE(doc.as() == "[\"hello\"]"); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } diff --git a/extras/tests/JsonDocument/constructor.cpp b/extras/tests/JsonDocument/constructor.cpp index 371a2561..5dd6bf82 100644 --- a/extras/tests/JsonDocument/constructor.cpp +++ b/extras/tests/JsonDocument/constructor.cpp @@ -64,6 +64,7 @@ TEST_CASE("JsonDocument constructor") { REQUIRE(doc2.as() == "{\"hello\":\"world\"}"); REQUIRE(spyingAllocator.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } @@ -87,6 +88,7 @@ TEST_CASE("JsonDocument constructor") { REQUIRE(doc2.as() == "[\"hello\"]"); REQUIRE(spyingAllocator.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } diff --git a/extras/tests/JsonDocument/remove.cpp b/extras/tests/JsonDocument/remove.cpp index f017bb9a..3533cd68 100644 --- a/extras/tests/JsonDocument/remove.cpp +++ b/extras/tests/JsonDocument/remove.cpp @@ -22,10 +22,10 @@ TEST_CASE("JsonDocument::remove()") { SECTION("string literal") { doc["a"] = 1; - doc["a\0b"_s] = 2; + doc["x"] = 2; doc["b"] = 3; - doc.remove("a\0b"); + doc.remove("x"); REQUIRE(doc.as() == "{\"a\":1,\"b\":3}"); } diff --git a/extras/tests/JsonDocument/set.cpp b/extras/tests/JsonDocument/set.cpp index 88c33bbc..b6ccef37 100644 --- a/extras/tests/JsonDocument/set.cpp +++ b/extras/tests/JsonDocument/set.cpp @@ -37,7 +37,9 @@ TEST_CASE("JsonDocument::set()") { doc.set("example"); REQUIRE(doc.as() == "example"_s); - REQUIRE(spy.log() == AllocatorLog{}); + REQUIRE(spy.log() == AllocatorLog{ + Allocate(sizeofStaticStringPool()), + }); } SECTION("const char*") { diff --git a/extras/tests/JsonDocument/shrinkToFit.cpp b/extras/tests/JsonDocument/shrinkToFit.cpp index 148b552d..1718dadf 100644 --- a/extras/tests/JsonDocument/shrinkToFit.cpp +++ b/extras/tests/JsonDocument/shrinkToFit.cpp @@ -75,7 +75,11 @@ TEST_CASE("JsonDocument::shrinkToFit()") { doc.shrinkToFit(); REQUIRE(doc.as() == "hello"); - REQUIRE(spyingAllocator.log() == AllocatorLog{}); + REQUIRE(spyingAllocator.log() == + AllocatorLog{ + Allocate(sizeofStaticStringPool()), + Reallocate(sizeofStaticStringPool(), sizeofStaticStringPool(1)), + }); } SECTION("owned string") { @@ -110,7 +114,9 @@ TEST_CASE("JsonDocument::shrinkToFit()") { REQUIRE(spyingAllocator.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), Reallocate(sizeofPool(), sizeofObject(1)), + Reallocate(sizeofStaticStringPool(), sizeofStaticStringPool(1)), }); } @@ -137,7 +143,9 @@ TEST_CASE("JsonDocument::shrinkToFit()") { REQUIRE(spyingAllocator.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), Reallocate(sizeofPool(), sizeofArray(1)), + Reallocate(sizeofStaticStringPool(), sizeofStaticStringPool(1)), }); } @@ -164,20 +172,23 @@ TEST_CASE("JsonDocument::shrinkToFit()") { REQUIRE(spyingAllocator.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), Reallocate(sizeofPool(), sizeofObject(1)), + Reallocate(sizeofStaticStringPool(), sizeofStaticStringPool(2)), }); } SECTION("owned string in object") { - doc["key"] = "abcdefg"_s; + doc["key1"_s] = "value"_s; doc.shrinkToFit(); - REQUIRE(doc.as() == "{\"key\":\"abcdefg\"}"); + REQUIRE(doc.as() == "{\"key1\":\"value\"}"); REQUIRE(spyingAllocator.log() == AllocatorLog{ Allocate(sizeofPool()), - Allocate(sizeofString("abcdefg")), + Allocate(sizeofString("key1")), + Allocate(sizeofString("value")), Reallocate(sizeofPool(), sizeofPool(2)), }); } diff --git a/extras/tests/JsonDocument/subscript.cpp b/extras/tests/JsonDocument/subscript.cpp index 17bdcc2e..aa53e551 100644 --- a/extras/tests/JsonDocument/subscript.cpp +++ b/extras/tests/JsonDocument/subscript.cpp @@ -25,8 +25,6 @@ TEST_CASE("JsonDocument::operator[]") { SECTION("string literal") { REQUIRE(doc["abc"] == "ABC"); REQUIRE(cdoc["abc"] == "ABC"); - REQUIRE(doc["abc\0d"] == "ABCD"); - REQUIRE(cdoc["abc\0d"] == "ABCD"); } SECTION("std::string") { @@ -114,6 +112,7 @@ TEST_CASE("JsonDocument::operator[] key storage") { REQUIRE(doc.as() == "{\"hello\":0}"); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } diff --git a/extras/tests/JsonObject/set.cpp b/extras/tests/JsonObject/set.cpp index e5c3044d..54febfca 100644 --- a/extras/tests/JsonObject/set.cpp +++ b/extras/tests/JsonObject/set.cpp @@ -26,25 +26,12 @@ TEST_CASE("JsonObject::set()") { REQUIRE(obj2["hello"] == "world"_s); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } - SECTION("copy local string value") { - obj1["hello"] = "world"_s; - spy.clearLog(); - - bool success = obj2.set(obj1); - - REQUIRE(success == true); - REQUIRE(obj2["hello"] == "world"_s); - REQUIRE(spy.log() == AllocatorLog{ - Allocate(sizeofPool()), - Allocate(sizeofString("world")), - }); - } - - SECTION("copy local key") { - obj1["hello"_s] = "world"; + SECTION("copy local string key and value") { + obj1["hello"_s] = "world"_s; spy.clearLog(); bool success = obj2.set(obj1); @@ -54,6 +41,7 @@ TEST_CASE("JsonObject::set()") { REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), Allocate(sizeofString("hello")), + Allocate(sizeofString("world")), }); } @@ -110,7 +98,7 @@ TEST_CASE("JsonObject::set()") { } SECTION("copy fails in the middle of an array") { - TimebombAllocator timebomb(1); + TimebombAllocator timebomb(2); JsonDocument doc3(&timebomb); JsonObject obj3 = doc3.to(); diff --git a/extras/tests/JsonObject/subscript.cpp b/extras/tests/JsonObject/subscript.cpp index bdf900f9..a7548f94 100644 --- a/extras/tests/JsonObject/subscript.cpp +++ b/extras/tests/JsonObject/subscript.cpp @@ -102,21 +102,25 @@ TEST_CASE("JsonObject::operator[]") { REQUIRE(42 == obj[key]); } - SECTION("should not duplicate const char*") { + SECTION("string literals") { obj["hello"] = "world"; - REQUIRE(spy.log() == AllocatorLog{Allocate(sizeofPool())}); + REQUIRE(spy.log() == AllocatorLog{ + Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), + }); } SECTION("should duplicate char* value") { obj["hello"] = const_cast("world"); REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), Allocate(sizeofString("world")), }); } SECTION("should duplicate char* key") { - obj[const_cast("hello")] = "world"; + obj[const_cast("hello")] = 42; REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), Allocate(sizeofString("hello")), @@ -136,12 +140,13 @@ TEST_CASE("JsonObject::operator[]") { obj["hello"] = "world"_s; REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), Allocate(sizeofString("world")), }); } SECTION("should duplicate std::string key") { - obj["hello"_s] = "world"; + obj["hello"_s] = 42; REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), Allocate(sizeofString("hello")), @@ -158,7 +163,7 @@ TEST_CASE("JsonObject::operator[]") { } SECTION("should duplicate a non-static JsonString key") { - obj[JsonString("hello", false)] = "world"; + obj[JsonString("hello", false)] = 42; REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), Allocate(sizeofString("hello")), @@ -166,9 +171,10 @@ TEST_CASE("JsonObject::operator[]") { } SECTION("should not duplicate a static JsonString key") { - obj[JsonString("hello", true)] = "world"; + obj[JsonString("hello", true)] = 42; REQUIRE(spy.log() == AllocatorLog{ Allocate(sizeofPool()), + Allocate(sizeofStaticStringPool()), }); } diff --git a/extras/tests/JsonVariant/copy.cpp b/extras/tests/JsonVariant/copy.cpp index b5da71f5..89fe9ce4 100644 --- a/extras/tests/JsonVariant/copy.cpp +++ b/extras/tests/JsonVariant/copy.cpp @@ -38,13 +38,15 @@ TEST_CASE("JsonVariant::set(JsonVariant)") { REQUIRE(var1.as() == "{\"value\":[42]}"); } - SECTION("stores const char* by reference") { + SECTION("stores string literals by pointer") { var1.set("hello!!"); spyingAllocator.clearLog(); var2.set(var1); - REQUIRE(spyingAllocator.log() == AllocatorLog{}); + REQUIRE(spyingAllocator.log() == AllocatorLog{ + Allocate(sizeofStaticStringPool()), + }); } SECTION("stores char* by copy") { diff --git a/extras/tests/JsonVariant/set.cpp b/extras/tests/JsonVariant/set.cpp index 7d56818b..04ffeddc 100644 --- a/extras/tests/JsonVariant/set.cpp +++ b/extras/tests/JsonVariant/set.cpp @@ -23,7 +23,9 @@ TEST_CASE("JsonVariant::set() when there is enough memory") { REQUIRE(result == true); CHECK(variant == "hello"_s); // linked string cannot contain '\0' at the moment - CHECK(spy.log() == AllocatorLog{}); + CHECK(spy.log() == AllocatorLog{ + Allocate(sizeofStaticStringPool()), + }); } SECTION("const char*") { @@ -149,7 +151,9 @@ TEST_CASE("JsonVariant::set() when there is enough memory") { REQUIRE(result == true); REQUIRE(variant == "world"); // stores by pointer - REQUIRE(spy.log() == AllocatorLog{}); + REQUIRE(spy.log() == AllocatorLog{ + Allocate(sizeofStaticStringPool()), + }); } SECTION("non-static JsonString") { @@ -265,6 +269,20 @@ TEST_CASE("JsonVariant::set() with not enough memory") { JsonVariant v = doc.to(); + SECTION("string literal") { + bool result = v.set("hello world"); + + REQUIRE(result == false); + REQUIRE(v.isNull()); + } + + SECTION("static JsonString") { + bool result = v.set(JsonString("hello world", true)); + + REQUIRE(result == false); + REQUIRE(v.isNull()); + } + SECTION("std::string") { bool result = v.set("hello world!!"_s); diff --git a/extras/tests/JsonVariantConst/subscript.cpp b/extras/tests/JsonVariantConst/subscript.cpp index 281e3d95..0e9e86e4 100644 --- a/extras/tests/JsonVariantConst/subscript.cpp +++ b/extras/tests/JsonVariantConst/subscript.cpp @@ -57,7 +57,7 @@ TEST_CASE("JsonVariantConst::operator[]") { SECTION("string literal") { REQUIRE(var["ab"] == "AB"_s); REQUIRE(var["abc"] == "ABC"_s); - REQUIRE(var["abc\0d"] == "ABCD"_s); + REQUIRE(var["abc\0d"] == "ABC"_s); REQUIRE(var["def"].isNull()); REQUIRE(var[0].isNull()); } diff --git a/extras/tests/Misc/StringAdapters.cpp b/extras/tests/Misc/StringAdapters.cpp index 99178975..e3c0373c 100644 --- a/extras/tests/Misc/StringAdapters.cpp +++ b/extras/tests/Misc/StringAdapters.cpp @@ -21,7 +21,7 @@ TEST_CASE("adaptString()") { auto s = adaptString("bravo\0alpha"); CHECK(s.isNull() == false); - CHECK(s.size() == 11); + CHECK(s.size() == 5); CHECK(s.isStatic() == true); } diff --git a/extras/tests/MsgPackDeserializer/destination_types.cpp b/extras/tests/MsgPackDeserializer/destination_types.cpp index 6b437ec7..362a9651 100644 --- a/extras/tests/MsgPackDeserializer/destination_types.cpp +++ b/extras/tests/MsgPackDeserializer/destination_types.cpp @@ -104,6 +104,8 @@ TEST_CASE("deserializeMsgPack(MemberProxy)") { REQUIRE(err == DeserializationError::Ok); REQUIRE(doc.as() == "{\"hello\":\"world\",\"value\":[42]}"); - REQUIRE(spy.log() == AllocatorLog{}); + REQUIRE(spy.log() == AllocatorLog{ + Allocate(sizeofStaticStringPool()), + }); } } diff --git a/extras/tests/ResourceManager/CMakeLists.txt b/extras/tests/ResourceManager/CMakeLists.txt index 3a0908af..05be9e96 100644 --- a/extras/tests/ResourceManager/CMakeLists.txt +++ b/extras/tests/ResourceManager/CMakeLists.txt @@ -5,6 +5,7 @@ add_executable(ResourceManagerTests allocVariant.cpp clear.cpp + saveStaticString.cpp saveString.cpp shrinkToFit.cpp size.cpp diff --git a/extras/tests/ResourceManager/StringBuffer.cpp b/extras/tests/ResourceManager/StringBuffer.cpp index 1fb2b01b..b39b60e7 100644 --- a/extras/tests/ResourceManager/StringBuffer.cpp +++ b/extras/tests/ResourceManager/StringBuffer.cpp @@ -2,7 +2,7 @@ // Copyright © 2014-2025, Benoit BLANCHON // MIT License -#include +#include #include #include "Allocators.hpp" @@ -22,7 +22,7 @@ TEST_CASE("StringBuffer") { sb.save(&variant); REQUIRE(variant.type() == VariantType::TinyString); - REQUIRE(variant.asString() == "hi!"); + REQUIRE(variant.asString(&resources) == "hi!"); } SECTION("Tiny string can't contain NUL") { @@ -32,7 +32,7 @@ TEST_CASE("StringBuffer") { REQUIRE(variant.type() == VariantType::OwnedString); - auto str = variant.asString(); + auto str = variant.asString(&resources); REQUIRE(str.size() == 3); REQUIRE(str.c_str()[0] == 'a'); REQUIRE(str.c_str()[1] == 0); @@ -45,6 +45,6 @@ TEST_CASE("StringBuffer") { sb.save(&variant); REQUIRE(variant.type() == VariantType::OwnedString); - REQUIRE(variant.asString() == "alfa"); + REQUIRE(variant.asString(&resources) == "alfa"); } } diff --git a/extras/tests/ResourceManager/StringBuilder.cpp b/extras/tests/ResourceManager/StringBuilder.cpp index 5c6dd220..ec3c2ab4 100644 --- a/extras/tests/ResourceManager/StringBuilder.cpp +++ b/extras/tests/ResourceManager/StringBuilder.cpp @@ -2,7 +2,7 @@ // Copyright © 2014-2025, Benoit BLANCHON // MIT License -#include +#include #include #include "Allocators.hpp" @@ -46,7 +46,7 @@ TEST_CASE("StringBuilder") { REQUIRE(resources.overflowed() == false); REQUIRE(data.type() == VariantType::TinyString); - REQUIRE(data.asString() == "url"); + REQUIRE(data.asString(&resources) == "url"); } SECTION("Short string fits in first allocation") { @@ -134,9 +134,10 @@ TEST_CASE("StringBuilder::save() deduplicates strings") { auto s2 = saveString(builder, "world"); auto s3 = saveString(builder, "hello"); - REQUIRE(s1.asString() == "hello"); - REQUIRE(s2.asString() == "world"); - REQUIRE(+s1.asString().c_str() == +s3.asString().c_str()); // same address + REQUIRE(s1.asString(&resources) == "hello"); + REQUIRE(s2.asString(&resources) == "world"); + REQUIRE(+s1.asString(&resources).c_str() == + +s3.asString(&resources).c_str()); // same address REQUIRE(spy.log() == AllocatorLog{ @@ -152,10 +153,10 @@ TEST_CASE("StringBuilder::save() deduplicates strings") { auto s1 = saveString(builder, "hello world"); auto s2 = saveString(builder, "hello"); - REQUIRE(s1.asString() == "hello world"); - REQUIRE(s2.asString() == "hello"); - REQUIRE(+s2.asString().c_str() != - +s1.asString().c_str()); // different address + REQUIRE(s1.asString(&resources) == "hello world"); + REQUIRE(s2.asString(&resources) == "hello"); + REQUIRE(+s2.asString(&resources).c_str() != + +s1.asString(&resources).c_str()); // different address REQUIRE(spy.log() == AllocatorLog{ @@ -170,10 +171,10 @@ TEST_CASE("StringBuilder::save() deduplicates strings") { auto s1 = saveString(builder, "hello world"); auto s2 = saveString(builder, "worl"); - REQUIRE(s1.asString() == "hello world"); - REQUIRE(s2.asString() == "worl"); - REQUIRE(s2.asString().c_str() != - s1.asString().c_str()); // different address + REQUIRE(s1.asString(&resources) == "hello world"); + REQUIRE(s2.asString(&resources) == "worl"); + REQUIRE(s2.asString(&resources).c_str() != + s1.asString(&resources).c_str()); // different address REQUIRE(spy.log() == AllocatorLog{ diff --git a/extras/tests/ResourceManager/saveStaticString.cpp b/extras/tests/ResourceManager/saveStaticString.cpp new file mode 100644 index 00000000..588f2072 --- /dev/null +++ b/extras/tests/ResourceManager/saveStaticString.cpp @@ -0,0 +1,47 @@ +// ArduinoJson - https://arduinojson.org +// Copyright © 2014-2025, Benoit BLANCHON +// MIT License + +#include +#include +#include + +#include "Allocators.hpp" + +using namespace ArduinoJson::detail; + +TEST_CASE("ResourceManager::saveStaticString() deduplicates strings") { + SpyingAllocator spy; + ResourceManager resources(&spy); + + auto str1 = "hello"; + auto str2 = "world"; + + auto id1 = resources.saveStaticString(str1); + auto id2 = resources.saveStaticString(str2); + REQUIRE(id1 != id2); + + auto id3 = resources.saveStaticString(str1); + REQUIRE(id1 == id3); + + resources.shrinkToFit(); + REQUIRE(spy.log() == + AllocatorLog{ + Allocate(sizeofStaticStringPool()), + Reallocate(sizeofStaticStringPool(), sizeofStaticStringPool(2)), + }); + REQUIRE(resources.overflowed() == false); +} + +TEST_CASE("ResourceManager::saveStaticString() when allocation fails") { + SpyingAllocator spy(FailingAllocator::instance()); + ResourceManager resources(&spy); + + auto slotId = resources.saveStaticString("hello"); + + REQUIRE(slotId == NULL_SLOT); + REQUIRE(resources.overflowed() == true); + REQUIRE(spy.log() == AllocatorLog{ + AllocateFail(sizeofStaticStringPool()), + }); +} diff --git a/src/ArduinoJson/Memory/MemoryPool.hpp b/src/ArduinoJson/Memory/MemoryPool.hpp index 4663a61e..79f1bf53 100644 --- a/src/ArduinoJson/Memory/MemoryPool.hpp +++ b/src/ArduinoJson/Memory/MemoryPool.hpp @@ -34,6 +34,11 @@ class Slot { return ptr_; } + T& operator*() const { + ARDUINOJSON_ASSERT(ptr_ != nullptr); + return *ptr_; + } + T* operator->() const { ARDUINOJSON_ASSERT(ptr_ != nullptr); return ptr_; @@ -76,6 +81,14 @@ class MemoryPool { return slots_ + id; } + SlotId find(const T& value) const { + for (SlotId i = 0; i < usage_; i++) { + if (slots_[i] == value) + return i; + } + return NULL_SLOT; + } + void clear() { usage_ = 0; } diff --git a/src/ArduinoJson/Memory/MemoryPoolList.hpp b/src/ArduinoJson/Memory/MemoryPoolList.hpp index 7da08063..14b668c0 100644 --- a/src/ArduinoJson/Memory/MemoryPoolList.hpp +++ b/src/ArduinoJson/Memory/MemoryPoolList.hpp @@ -114,6 +114,15 @@ class MemoryPoolList { return pools_[poolIndex].getSlot(indexInPool); } + SlotId find(const T& value) const { + for (PoolCount i = 0; i < count_; i++) { + SlotId id = pools_[i].find(value); + if (id != NULL_SLOT) + return SlotId(i * ARDUINOJSON_POOL_CAPACITY + id); + } + return NULL_SLOT; + } + void clear(Allocator* allocator) { for (PoolCount i = 0; i < count_; i++) pools_[i].destroy(allocator); diff --git a/src/ArduinoJson/Memory/ResourceManager.hpp b/src/ArduinoJson/Memory/ResourceManager.hpp index f74c91bc..12a379c8 100644 --- a/src/ArduinoJson/Memory/ResourceManager.hpp +++ b/src/ArduinoJson/Memory/ResourceManager.hpp @@ -34,6 +34,7 @@ class ResourceManager { ~ResourceManager() { stringPool_.clear(allocator_); variantPools_.clear(allocator_); + staticStringsPools_.clear(allocator_); } ResourceManager(const ResourceManager&) = delete; @@ -42,6 +43,7 @@ class ResourceManager { friend void swap(ResourceManager& a, ResourceManager& b) { swap(a.stringPool_, b.stringPool_); swap(a.variantPools_, b.variantPools_); + swap(a.staticStringsPools_, b.staticStringsPools_); swap_(a.allocator_, b.allocator_); swap_(a.overflowed_, b.overflowed_); } @@ -111,14 +113,34 @@ class ResourceManager { stringPool_.dereference(s, allocator_); } + SlotId saveStaticString(const char* s) { + auto existingSlotId = staticStringsPools_.find(s); + if (existingSlotId != NULL_SLOT) + return existingSlotId; + + auto slot = staticStringsPools_.allocSlot(allocator_); + if (slot) + *slot = s; + else + overflowed_ = true; + + return slot.id(); + } + + const char* getStaticString(SlotId id) const { + return *staticStringsPools_.getSlot(id); + } + void clear() { - variantPools_.clear(allocator_); overflowed_ = false; + variantPools_.clear(allocator_); stringPool_.clear(allocator_); + staticStringsPools_.clear(allocator_); } void shrinkToFit() { variantPools_.shrinkToFit(allocator_); + staticStringsPools_.shrinkToFit(allocator_); } private: @@ -126,6 +148,7 @@ class ResourceManager { bool overflowed_; StringPool stringPool_; MemoryPoolList variantPools_; + MemoryPoolList staticStringsPools_; }; ARDUINOJSON_END_PRIVATE_NAMESPACE diff --git a/src/ArduinoJson/Object/JsonPair.hpp b/src/ArduinoJson/Object/JsonPair.hpp index f0dcb50a..9b2d4072 100644 --- a/src/ArduinoJson/Object/JsonPair.hpp +++ b/src/ArduinoJson/Object/JsonPair.hpp @@ -18,7 +18,7 @@ class JsonPair { JsonPair(detail::ObjectData::iterator iterator, detail::ResourceManager* resources) { if (!iterator.done()) { - key_ = iterator->asString(); + key_ = iterator->asString(resources); iterator.next(resources); value_ = JsonVariant(iterator.data(), resources); } @@ -46,7 +46,7 @@ class JsonPairConst { JsonPairConst(detail::ObjectData::iterator iterator, const detail::ResourceManager* resources) { if (!iterator.done()) { - key_ = iterator->asString(); + key_ = iterator->asString(resources); iterator.next(resources); value_ = JsonVariantConst(iterator.data(), resources); } diff --git a/src/ArduinoJson/Object/ObjectImpl.hpp b/src/ArduinoJson/Object/ObjectImpl.hpp index 67677d74..34da0dab 100644 --- a/src/ArduinoJson/Object/ObjectImpl.hpp +++ b/src/ArduinoJson/Object/ObjectImpl.hpp @@ -36,7 +36,7 @@ inline ObjectData::iterator ObjectData::findKey( return iterator(); bool isKey = true; for (auto it = createIterator(resources); !it.done(); it.next(resources)) { - if (isKey && stringEquals(key, adaptString(it->asString()))) + if (isKey && stringEquals(key, adaptString(it->asString(resources)))) return it; isKey = !isKey; } diff --git a/src/ArduinoJson/Strings/Adapters/RamString.hpp b/src/ArduinoJson/Strings/Adapters/RamString.hpp index c269ffb7..59b4f287 100644 --- a/src/ArduinoJson/Strings/Adapters/RamString.hpp +++ b/src/ArduinoJson/Strings/Adapters/RamString.hpp @@ -90,8 +90,9 @@ template struct StringAdapter { using AdaptedString = RamString; - static AdaptedString adapt(const char (&p)[N]) { - return RamString(p, N - 1, true); + static AdaptedString adapt(const char* p) { + ARDUINOJSON_ASSERT(p); + return RamString(p, ::strlen(p), true); } }; diff --git a/src/ArduinoJson/Strings/JsonString.hpp b/src/ArduinoJson/Strings/JsonString.hpp index 98bae43f..d7e9818b 100644 --- a/src/ArduinoJson/Strings/JsonString.hpp +++ b/src/ArduinoJson/Strings/JsonString.hpp @@ -28,8 +28,7 @@ class JsonString { detail::enable_if_t::value && !detail::is_same::value, int> = 0> - JsonString(const char* data, TSize sz, bool isStatic = false) - : str_(data, size_t(sz), isStatic) {} + JsonString(const char* data, TSize sz) : str_(data, size_t(sz), false) {} // Returns a pointer to the characters. const char* c_str() const { diff --git a/src/ArduinoJson/Variant/ConverterImpl.hpp b/src/ArduinoJson/Variant/ConverterImpl.hpp index 6c5fcda8..627fb6b0 100644 --- a/src/ArduinoJson/Variant/ConverterImpl.hpp +++ b/src/ArduinoJson/Variant/ConverterImpl.hpp @@ -160,7 +160,7 @@ struct Converter : private detail::VariantAttorney { static const char* fromJson(JsonVariantConst src) { auto data = getData(src); - return data ? data->asString().c_str() : 0; + return data ? data->asString(getResourceManager(src)).c_str() : 0; } static bool checkJson(JsonVariantConst src) { @@ -178,7 +178,7 @@ struct Converter : private detail::VariantAttorney { static JsonString fromJson(JsonVariantConst src) { auto data = getData(src); - return data ? data->asString() : JsonString(); + return data ? data->asString(getResourceManager(src)) : JsonString(); } static bool checkJson(JsonVariantConst src) { diff --git a/src/ArduinoJson/Variant/VariantContent.hpp b/src/ArduinoJson/Variant/VariantContent.hpp index d6ee6c78..9c0b820f 100644 --- a/src/ArduinoJson/Variant/VariantContent.hpp +++ b/src/ArduinoJson/Variant/VariantContent.hpp @@ -56,13 +56,10 @@ union VariantContent { bool asBoolean; uint32_t asUint32; int32_t asInt32; -#if ARDUINOJSON_USE_EXTENSIONS SlotId asSlotId; -#endif ArrayData asArray; ObjectData asObject; CollectionData asCollection; - const char* asLinkedString; struct StringNode* asOwnedString; char asTinyString[tinyStringMaxLength + 1]; }; diff --git a/src/ArduinoJson/Variant/VariantData.hpp b/src/ArduinoJson/Variant/VariantData.hpp index 6b0d474b..e9d7429d 100644 --- a/src/ArduinoJson/Variant/VariantData.hpp +++ b/src/ArduinoJson/Variant/VariantData.hpp @@ -77,7 +77,7 @@ class VariantData { return visit.visit(JsonString(content_.asTinyString)); case VariantType::LinkedString: - return visit.visit(JsonString(content_.asLinkedString, true)); + return visit.visit(JsonString(asLinkedString(resources), true)); case VariantType::OwnedString: return visit.visit(JsonString(content_.asOwnedString->data, @@ -216,7 +216,7 @@ class VariantData { str = content_.asTinyString; break; case VariantType::LinkedString: - str = content_.asLinkedString; + str = asLinkedString(resources); break; case VariantType::OwnedString: str = content_.asOwnedString->data; @@ -261,7 +261,7 @@ class VariantData { str = content_.asTinyString; break; case VariantType::LinkedString: - str = content_.asLinkedString; + str = asLinkedString(resources); break; case VariantType::OwnedString: str = content_.asOwnedString->data; @@ -298,12 +298,14 @@ class VariantData { } } - JsonString asString() const { + const char* asLinkedString(const ResourceManager* resources) const; + + JsonString asString(const ResourceManager* resources) const { switch (type_) { case VariantType::TinyString: return JsonString(content_.asTinyString); case VariantType::LinkedString: - return JsonString(content_.asLinkedString, true); + return JsonString(asLinkedString(resources), true); case VariantType::OwnedString: return JsonString(content_.asOwnedString->data, content_.asOwnedString->length); @@ -519,12 +521,7 @@ class VariantData { var->setString(value, resources); } - void setLinkedString(const char* s) { - ARDUINOJSON_ASSERT(type_ == VariantType::Null); // must call clear() first - ARDUINOJSON_ASSERT(s); - type_ = VariantType::LinkedString; - content_.asLinkedString = s; - } + bool setLinkedString(const char* s, ResourceManager* resources); template void setTinyString(const TAdaptedString& s) { diff --git a/src/ArduinoJson/Variant/VariantImpl.hpp b/src/ArduinoJson/Variant/VariantImpl.hpp index 98289ed0..1c5ec3a3 100644 --- a/src/ArduinoJson/Variant/VariantImpl.hpp +++ b/src/ArduinoJson/Variant/VariantImpl.hpp @@ -18,6 +18,20 @@ inline void VariantData::setRawString(SerializedValue value, setRawString(dup); } +inline bool VariantData::setLinkedString(const char* s, + ResourceManager* resources) { + ARDUINOJSON_ASSERT(type_ == VariantType::Null); // must call clear() first + ARDUINOJSON_ASSERT(s); + + auto slotId = resources->saveStaticString(s); + if (slotId == NULL_SLOT) + return false; + + type_ = VariantType::LinkedString; + content_.asSlotId = slotId; + return true; +} + template inline bool VariantData::setString(TAdaptedString value, ResourceManager* resources) { @@ -26,10 +40,8 @@ inline bool VariantData::setString(TAdaptedString value, if (value.isNull()) return false; - if (value.isStatic()) { - setLinkedString(value.data()); - return true; - } + if (value.isStatic()) + return setLinkedString(value.data(), resources); if (isTinyString(value, value.size())) { setTinyString(value); @@ -70,6 +82,12 @@ inline const VariantExtension* VariantData::getExtension( } #endif +inline const char* VariantData::asLinkedString( + const ResourceManager* resources) const { + ARDUINOJSON_ASSERT(type_ == VariantType::LinkedString); + return resources->getStaticString(content_.asSlotId); +} + template enable_if_t VariantData::setFloat( T value, ResourceManager* resources) {