Reference-count shared strings

bblanchon · bblanchon · commit 003087406c98 · 2023-04-21T16:11:54.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,3 +12,4 @@ HEAD
 * Remove `ARDUINOJSON_ENABLE_STRING_DEDUPLICATION` (string deduplication cannot be enabled anymore)
 * Remove `JsonDocument::capacity()`
 * Store the strings in the heap
+* Reference-count shared strings
diff --git a/extras/tests/JsonDocument/garbageCollect.cpp b/extras/tests/JsonDocument/garbageCollect.cpp
@@ -32,7 +32,6 @@ TEST_CASE("JsonDocument::garbageCollect()") {
             AllocatorLog() << AllocatorLog::Allocate(4096)
                            << AllocatorLog::Allocate(sizeofString(7))
                            << AllocatorLog::Deallocate(sizeofString(7))
-                           << AllocatorLog::Deallocate(sizeofString(7))
                            << AllocatorLog::Deallocate(4096));
   }
 
@@ -46,7 +45,7 @@ TEST_CASE("JsonDocument::garbageCollect()") {
     bool result = doc.garbageCollect();
 
     REQUIRE(result == false);
-    REQUIRE(doc.memoryUsage() == sizeofObject(2) + 2 * sizeofString(7));
+    REQUIRE(doc.memoryUsage() == sizeofObject(2) + sizeofString(7));
     REQUIRE(doc.as<std::string>() == "{\"dancing\":2}");
 
     REQUIRE(spyingAllocator.log() == AllocatorLog()
diff --git a/extras/tests/JsonVariant/clear.cpp b/extras/tests/JsonVariant/clear.cpp
@@ -6,6 +6,9 @@
 #include <stdint.h>
 #include <catch.hpp>
 
+using ArduinoJson::detail::sizeofArray;
+using ArduinoJson::detail::sizeofString;
+
 TEST_CASE("JsonVariant::clear()") {
   JsonDocument doc(4096);
   JsonVariant var = doc.to<JsonVariant>();
@@ -23,4 +26,12 @@ TEST_CASE("JsonVariant::clear()") {
 
     REQUIRE(var.isNull() == true);
   }
+
+  SECTION("releases owned string") {
+    var.set(std::string("hello"));
+    REQUIRE(doc.memoryUsage() == sizeofString(5));
+
+    var.clear();
+    REQUIRE(doc.memoryUsage() == 0);
+  }
 }
diff --git a/extras/tests/JsonVariant/remove.cpp b/extras/tests/JsonVariant/remove.cpp
@@ -6,35 +6,67 @@
 #include <stdint.h>
 #include <catch.hpp>
 
-TEST_CASE("JsonVariant::remove()") {
+using ArduinoJson::detail::sizeofArray;
+using ArduinoJson::detail::sizeofString;
+
+TEST_CASE("JsonVariant::remove(int)") {
   JsonDocument doc(4096);
-  JsonVariant var = doc.to<JsonVariant>();
 
-  SECTION("remove(int)") {
-    var.add(1);
-    var.add(2);
-    var.add(3);
+  SECTION("release top level strings") {
+    doc.add(std::string("hello"));
+    doc.add(std::string("hello"));
+    doc.add(std::string("world"));
+
+    JsonVariant var = doc.as<JsonVariant>();
+    REQUIRE(var.as<std::string>() == "[\"hello\",\"hello\",\"world\"]");
+    REQUIRE(doc.memoryUsage() == sizeofArray(3) + 2 * sizeofString(5));
+
+    var.remove(1);
+    REQUIRE(var.as<std::string>() == "[\"hello\",\"world\"]");
+    REQUIRE(doc.memoryUsage() == sizeofArray(3) + 2 * sizeofString(5));
 
     var.remove(1);
+    REQUIRE(var.as<std::string>() == "[\"hello\"]");
+    REQUIRE(doc.memoryUsage() == sizeofArray(3) + 1 * sizeofString(5));
 
-    REQUIRE(var.as<std::string>() == "[1,3]");
+    var.remove(0);
+    REQUIRE(var.as<std::string>() == "[]");
+    REQUIRE(doc.memoryUsage() == sizeofArray(3));
   }
 
-  SECTION("remove(const char *)") {
-    var["a"] = 1;
-    var["b"] = 2;
+  SECTION("release strings in nested array") {
+    doc[0][0] = std::string("hello");
 
-    var.remove("a");
+    JsonVariant var = doc.as<JsonVariant>();
+    REQUIRE(var.as<std::string>() == "[[\"hello\"]]");
+    REQUIRE(doc.memoryUsage() == 2 * sizeofArray(1) + sizeofString(5));
 
-    REQUIRE(var.as<std::string>() == "{\"b\":2}");
+    var.remove(0);
+    REQUIRE(var.as<std::string>() == "[]");
+    REQUIRE(doc.memoryUsage() == 2 * sizeofArray(1));
   }
+}
 
-  SECTION("remove(std::string)") {
-    var["a"] = 1;
-    var["b"] = 2;
+TEST_CASE("JsonVariant::remove(const char *)") {
+  JsonDocument doc(4096);
+  JsonVariant var = doc.to<JsonVariant>();
 
-    var.remove(std::string("b"));
+  var["a"] = 1;
+  var["b"] = 2;
 
-    REQUIRE(var.as<std::string>() == "{\"a\":1}");
-  }
+  var.remove("a");
+
+  REQUIRE(var.as<std::string>() == "{\"b\":2}");
+}
+
+TEST_CASE("JsonVariant::remove(std::string)") {
+  JsonDocument doc(4096);
+  JsonVariant var = doc.to<JsonVariant>();
+
+  var["a"] = 1;
+  var["b"] = 2;
+
+  var.remove(std::string("b"));
+
+  REQUIRE(var.as<std::string>() == "{\"a\":1}");
 }
diff --git a/extras/tests/JsonVariant/set.cpp b/extras/tests/JsonVariant/set.cpp
@@ -7,6 +7,9 @@
 
 #include "Allocators.hpp"
 
+using ArduinoJson::detail::sizeofObject;
+using ArduinoJson::detail::sizeofString;
+
 enum ErrorCode { ERROR_01 = 1, ERROR_10 = 10 };
 
 TEST_CASE("JsonVariant::set() when there is enough memory") {
@@ -172,3 +175,36 @@ TEST_CASE("JsonVariant::set(JsonDocument)") {
   serializeJson(doc2, json);
   REQUIRE(json == "{\"hello\":\"world\"}");
 }
+
+TEST_CASE("JsonVariant::set() releases the previous value") {
+  JsonDocument doc(1024);
+  doc["hello"] = std::string("world");
+  REQUIRE(doc.memoryUsage() == sizeofObject(1) + sizeofString(5));
+
+  JsonVariant v = doc["hello"];
+
+  SECTION("int") {
+    v.set(42);
+    REQUIRE(doc.memoryUsage() == sizeofObject(1));
+  }
+
+  SECTION("bool") {
+    v.set(false);
+    REQUIRE(doc.memoryUsage() == sizeofObject(1));
+  }
+
+  SECTION("const char*") {
+    v.set("hello");
+    REQUIRE(doc.memoryUsage() == sizeofObject(1));
+  }
+
+  SECTION("float") {
+    v.set(1.2);
+    REQUIRE(doc.memoryUsage() == sizeofObject(1));
+  }
+
+  SECTION("Serialized<const char*>") {
+    v.set(serialized("[]"));
+    REQUIRE(doc.memoryUsage() == sizeofObject(1));
+  }
+}
diff --git a/src/ArduinoJson/Memory/MemoryPool.hpp b/src/ArduinoJson/Memory/MemoryPool.hpp
@@ -28,6 +28,7 @@ constexpr size_t sizeofObject(size_t n) {
 struct StringNode {
   struct StringNode* next;
   uint16_t length;
+  uint16_t references;
   char data[1];
 };
 
@@ -112,6 +113,7 @@ class MemoryPool {
 
     auto node = findString(str);
     if (node) {
+      node->references++;
       return node->data;
     }
 
@@ -147,6 +149,7 @@ class MemoryPool {
         _allocator->allocate(sizeofString(length)));
     if (node) {
       node->length = uint16_t(length);
+      node->references = 1;
     } else {
       _overflowed = true;
     }
@@ -170,6 +173,23 @@ class MemoryPool {
     _allocator->deallocate(node);
   }
 
+  void dereferenceString(const char* s) {
+    StringNode* prev = nullptr;
+    for (auto node = _strings; node; node = node->next) {
+      if (node->data == s) {
+        if (--node->references == 0) {
+          if (prev)
+            prev->next = node->next;
+          else
+            _strings = node->next;
+          _allocator->deallocate(node);
+        }
+        return;
+      }
+      prev = node;
+    }
+  }
+
   void clear() {
     _right = _end;
     _overflowed = false;
diff --git a/src/ArduinoJson/StringStorage/StringCopier.hpp b/src/ArduinoJson/StringStorage/StringCopier.hpp
@@ -33,6 +33,8 @@ class StringCopier {
       node = _pool->reallocString(_node, _size);
       _pool->addStringToList(node);
       _node = nullptr;  // next time we need a new string
+    } else {
+      node->references++;
     }
     return JsonString(node->data, node->length, JsonString::Copied);
   }
diff --git a/src/ArduinoJson/Variant/SlotFunctions.hpp b/src/ArduinoJson/Variant/SlotFunctions.hpp
@@ -24,6 +24,8 @@ inline VariantData* slotData(VariantSlot* slot) {
 
 inline void slotRelease(const VariantSlot* slot, MemoryPool* pool) {
   ARDUINOJSON_ASSERT(slot != nullptr);
+  if (slot->ownsKey())
+    pool->dereferenceString(slot->key());
   variantRelease(slot->data(), pool);
 }
 
diff --git a/src/ArduinoJson/Variant/VariantFunctions.hpp b/src/ArduinoJson/Variant/VariantFunctions.hpp
@@ -33,6 +33,10 @@ inline typename TVisitor::result_type variantAccept(const VariantData* var,
 
 inline void variantRelease(const VariantData* var, MemoryPool* pool) {
   ARDUINOJSON_ASSERT(var != nullptr);
+  auto s = var->getOwnedString();
+  if (s)
+    pool->dereferenceString(s);
+
   auto c = var->asCollection();
   if (c) {
     for (auto slot = c->head(); slot; slot = slot->next())

Original file line number	Diff line number	Diff line change
`@@ -33,6 +33,8 @@ class StringCopier {`
`33`	`33`	`node = _pool->reallocString(_node, _size);`
`34`	`34`	`_pool->addStringToList(node);`
`35`	`35`	`_node = nullptr; // next time we need a new string`
	`36`	`+ } else {`
	`37`	`+ node->references++;`
`36`	`38`	`}`
`37`	`39`	`return JsonString(node->data, node->length, JsonString::Copied);`
`38`	`40`	`}`
Original file line number	Diff line number	Diff line change
`@@ -24,6 +24,8 @@ inline VariantData* slotData(VariantSlot* slot) {`
`24`	`24`
`25`	`25`	`inline void slotRelease(const VariantSlot* slot, MemoryPool* pool) {`
`26`	`26`	`ARDUINOJSON_ASSERT(slot != nullptr);`
	`27`	`+ if (slot->ownsKey())`
	`28`	`+ pool->dereferenceString(slot->key());`
`27`	`29`	`variantRelease(slot->data(), pool);`
`28`	`30`	`}`
`29`	`31`