ObjectTracker now uses the string heap instead of a StringBuffer

10 files changed, 62 insertions, 46 deletions

diff --git a/src/GCodes/GCodeBuffer/ExpressionParser.h b/src/GCodes/GCodeBuffer/ExpressionParser.h
index 3dd5f236..c965b605 100644
--- a/src/GCodes/GCodeBuffer/ExpressionParser.h
+++ b/src/GCodes/GCodeBuffer/ExpressionParser.h
@@ -9,7 +9,6 @@
 #define SRC_GCODES_GCODEBUFFER_EXPRESSIONPARSER_H_
 
 #include <RepRapFirmware.h>
-#include <General/StringBuffer.h>
 #include <ObjectModel/ObjectModel.h>
 #include <GCodes/GCodeException.h>
 
diff --git a/src/GCodes/GCodeBuffer/StringParser.cpp b/src/GCodes/GCodeBuffer/StringParser.cpp
index e4abb59e..bdd6ead2 100644
--- a/src/GCodes/GCodeBuffer/StringParser.cpp
+++ b/src/GCodes/GCodeBuffer/StringParser.cpp
@@ -14,7 +14,6 @@
 #include <GCodes/GCodes.h>
 #include <Platform/Platform.h>
 #include <Platform/RepRap.h>
-#include <General/StringBuffer.h>
 #include <Networking/NetworkDefs.h>
 
 // Replace the default definition of THROW_INTERNAL_ERROR by one that gives line information
diff --git a/src/GCodes/GCodeBuffer/StringParser.h b/src/GCodes/GCodeBuffer/StringParser.h
index fa090b70..ed20be62 100644
--- a/src/GCodes/GCodeBuffer/StringParser.h
+++ b/src/GCodes/GCodeBuffer/StringParser.h
@@ -19,7 +19,6 @@
 class GCodeBuffer;
 class IPAddress;
 class MacAddress;
-class StringBuffer;
 class VariableSet;
 
 class StringParser
diff --git a/src/GCodes/ObjectTracker.cpp b/src/GCodes/ObjectTracker.cpp
index 69a25f3f..b3597d0a 100644
--- a/src/GCodes/ObjectTracker.cpp
+++ b/src/GCodes/ObjectTracker.cpp
@@ -47,10 +47,11 @@ constexpr ObjectModelTableEntry ObjectTracker::objectModelTable[] =
 	{ "m486Names",		OBJECT_MODEL_FUNC(self->usingM486Naming),							ObjectModelEntryFlags::none },
 	{ "m486Numbers",	OBJECT_MODEL_FUNC(self->usingM486Labelling),						ObjectModelEntryFlags::none },
 	{ "objects",		OBJECT_MODEL_FUNC_NOSELF(&objectsArrayDescriptor),					ObjectModelEntryFlags::none },
+
 #if TRACK_OBJECT_NAMES
 	// 1. ObjectDirectoryEntry root
 	{ "cancelled",	OBJECT_MODEL_FUNC(self->IsCancelled(context.GetLastIndex())),			ObjectModelEntryFlags::none },
-	{ "name",		OBJECT_MODEL_FUNC(self->objectDirectory[context.GetLastIndex()].name),	ObjectModelEntryFlags::none },
+	{ "name",		OBJECT_MODEL_FUNC(self->objectDirectory[context.GetLastIndex()].name.IncreaseRefCount()),	ObjectModelEntryFlags::none },
 	{ "x",			OBJECT_MODEL_FUNC_NOSELF(&xArrayDescriptor),							ObjectModelEntryFlags::none },
 	{ "y",			OBJECT_MODEL_FUNC_NOSELF(&yArrayDescriptor),							ObjectModelEntryFlags::none },
 #endif
@@ -81,7 +82,6 @@ void ObjectTracker::Init() noexcept
 	{
 		objectDirectory[i].Init("");
 	}
-	objectNames.Reset();
 	usingM486Naming = false;
 #endif
 }
@@ -137,10 +137,9 @@ GCodeResult ObjectTracker::HandleM486(GCodeBuffer &gb, const StringRef &reply, O
 			{
 				CreateObject(num, objectName.c_str());
 			}
-			else if (strcmp(objectName.c_str(), objectDirectory[num].name) != 0)
+			else if (strcmp(objectName.c_str(), objectDirectory[num].name.Get().Ptr()) != 0)
 			{
-				objectNames.FinishedUsing(objectDirectory[num].name);
-				SetObjectName(num, objectName.c_str());
+				objectDirectory[num].SetName(objectName.c_str());
 				reprap.JobUpdated();
 			}
 		}
@@ -225,7 +224,7 @@ GCodeResult ObjectTracker::HandleM486(GCodeBuffer &gb, const StringRef &reply, O
 							(objectsCancelled.IsBitSet(i) ? " (cancelled)" : ""),
 							(int)obj.x[0], (int)obj.x[1],
 							(int)obj.y[0], (int)obj.y[1],
-							obj.name);
+							obj.name.Get().Ptr());
 			}
 			if (numObjects > MaxTrackedObjects)
 			{
@@ -278,7 +277,7 @@ bool ObjectTracker::WriteObjectDirectory(FileStore *f) const noexcept
 	for (size_t i = 0; ok && i < min<unsigned int>(numObjects, MaxTrackedObjects); ++i)
 	{
 		String<StringLength100> buf;
-		buf.printf("M486 S%u A\"%s\"\n", i, objectDirectory[i].name);
+		buf.printf("M486 S%u A\"%s\"\n", i, objectDirectory[i].name.Get().Ptr());
 		ok = f->Write(buf.c_str());
 	}
 #else
@@ -318,7 +317,7 @@ bool ObjectTracker::WriteObjectDirectory(FileStore *f) const noexcept
 // Create a new entry in the object directory
 void ObjectDirectoryEntry::Init(const char *label) noexcept
 {
-	name = label;
+	name.Assign(label);
 	x[0] = x[1] = y[0] = y[1] = std::numeric_limits<int16_t>::min();
 }
 
@@ -369,6 +368,11 @@ bool ObjectDirectoryEntry::UpdateObjectCoordinates(const float coords[], AxesBit
 	return updated;
 }
 
+void ObjectDirectoryEntry::SetName(const char *label) noexcept
+{
+	name.Assign(label);
+}
+
 // Update the min and max object coordinates to include the coordinates passed
 // We could pass both the start and end coordinates of the printing move, however it is simpler just to pass the end coordinates.
 // This is OK because it is very unlikely that there won't be a subsequent extruding move that ends close to the original one.
@@ -383,14 +387,6 @@ void ObjectTracker::UpdateObjectCoordinates(const float coords[], AxesBitmap axe
 	}
 }
 
-void ObjectTracker::SetObjectName(unsigned int number, const char *label) noexcept
-{
-	bool err = objectNames.GetRef().copy(label);
-	const char * const name = objectNames.LatestCStr();
-	err = err || objectNames.Fix();
-	objectDirectory[number].name = ((err) ? "" : name);
-}
-
 // This is called when we need to create a new named object
 void ObjectTracker::CreateObject(unsigned int number, const char *label) noexcept
 {
@@ -401,7 +397,7 @@ void ObjectTracker::CreateObject(unsigned int number, const char *label) noexcep
 			objectDirectory[numObjects].Init("");
 			++numObjects;
 		}
-		SetObjectName(number, label);
+		objectDirectory[number].SetName(label);
 		reprap.JobUpdated();
 	}
 }
@@ -413,7 +409,7 @@ void ObjectTracker::StartObject(GCodeBuffer& gb, const char *label) noexcept
 	{
 		for (size_t i = 0; i < numObjects; ++i)
 		{
-			if (strcmp(objectDirectory[i].name, label) == 0)
+			if (strcmp(objectDirectory[i].name.Get().Ptr(), label) == 0)
 			{
 				ChangeToObject(gb, i);
 				return;
diff --git a/src/GCodes/ObjectTracker.h b/src/GCodes/ObjectTracker.h
index 0b0e253b..efcf4eac 100644
--- a/src/GCodes/ObjectTracker.h
+++ b/src/GCodes/ObjectTracker.h
@@ -12,17 +12,18 @@
 #include "GCodeResult.h"
 #include "RestorePoint.h"
 #include <ObjectModel/ObjectModel.h>
-#include <General/StringBuffer.h>
+#include <Platform/Heap.h>
 
 #if TRACK_OBJECT_NAMES
 
 struct ObjectDirectoryEntry
 {
-	const char *name;					// pointer to the object name within the string buffer
-	int16_t x[2], y[2];					// lowest and highest extrusion coordinates
+	AutoStringHandle name;					// pointer to the object name within the string buffer
+	int16_t x[2], y[2];						// lowest and highest extrusion coordinates
 
 	void Init(const char *label) noexcept;
 	bool UpdateObjectCoordinates(const float coords[], AxesBitmap axes) noexcept;
+	void SetName(const char *label) noexcept;
 };
 
 #endif
@@ -35,9 +36,6 @@ class ObjectTracker
 public:
 	ObjectTracker() noexcept
 		: numObjects(0)
-#if TRACK_OBJECT_NAMES
-		  , objectNames(stringBufferStorage, ARRAY_SIZE(stringBufferStorage))
-#endif
 	{ }
 
 	void Init() noexcept;
@@ -82,7 +80,6 @@ private:
 
 #if TRACK_OBJECT_NAMES
 	void CreateObject(unsigned int number, const char *label) noexcept;
-	void SetObjectName(unsigned int number, const char *label) noexcept;
 	ExpressionValue GetXCoordinate(const ObjectExplorationContext& context) const noexcept;
 	ExpressionValue GetYCoordinate(const ObjectExplorationContext& context) const noexcept;
 #endif
@@ -95,8 +92,6 @@ private:
 
 #if TRACK_OBJECT_NAMES
 	ObjectDirectoryEntry objectDirectory[MaxTrackedObjects];
-	StringBuffer objectNames;
-	char stringBufferStorage[ObjectNamesStringSpace];
 	bool usingM486Naming;
 #endif
 
diff --git a/src/ObjectModel/ObjectModel.cpp b/src/ObjectModel/ObjectModel.cpp
index 157c5edd..ed59b5fa 100644
--- a/src/ObjectModel/ObjectModel.cpp
+++ b/src/ObjectModel/ObjectModel.cpp
@@ -138,6 +138,14 @@ ExpressionValue::ExpressionValue(const ExpressionValue& other) noexcept
 	}
 }
 
+ExpressionValue::ExpressionValue(ExpressionValue&& other) noexcept
+{
+	type = other.type;
+	param = other.param;
+	whole = other.whole;
+	other.type = (uint32_t)TypeCode::None;
+}
+
 ExpressionValue::~ExpressionValue()
 {
 	Release();
diff --git a/src/ObjectModel/ObjectModel.h b/src/ObjectModel/ObjectModel.h
index 427fbe91..e391ddb9 100644
--- a/src/ObjectModel/ObjectModel.h
+++ b/src/ObjectModel/ObjectModel.h
@@ -135,6 +135,7 @@ struct ExpressionValue
 #endif
 
 	ExpressionValue(const ExpressionValue& other) noexcept;
+	ExpressionValue(ExpressionValue&& other) noexcept;
 	~ExpressionValue();
 	ExpressionValue& operator=(const ExpressionValue& other) noexcept;
 	void Release() noexcept;					// release any associated storage
diff --git a/src/Platform/Heap.cpp b/src/Platform/Heap.cpp
index 2f74b8b9..480617a4 100644
--- a/src/Platform/Heap.cpp
+++ b/src/Platform/Heap.cpp
@@ -64,9 +64,11 @@ struct HeapBlock
 ReadWriteLock StringHandle::heapLock;
 IndexBlock *StringHandle::indexRoot = nullptr;
 HeapBlock *StringHandle::heapRoot = nullptr;
-std::atomic<size_t> StringHandle::spaceToRecycle = 0;
-size_t StringHandle::totalIndexSpace = 0;
-size_t StringHandle::totalHeapSpace = 0;
+size_t StringHandle::handlesAllocated = 0;
+std::atomic<size_t> StringHandle::handlesUsed = 0;
+size_t StringHandle::heapAllocated = 0;
+size_t StringHandle::heapUsed = 0;
+std::atomic<size_t> StringHandle::heapToRecycle = 0;
 unsigned int StringHandle::gcCyclesDone = 0;
 
 /*static*/ void StringHandle::GarbageCollect() noexcept
@@ -81,6 +83,7 @@ unsigned int StringHandle::gcCyclesDone = 0;
 	RRF_ASSERT(heapLock.GetWriteLockOwner() == TaskBase::GetCallerTaskHandle());
 #endif
 
+	heapUsed = 0;
 	for (HeapBlock *currentBlock = heapRoot; currentBlock != nullptr; )
 	{
 		// Skip any used blocks at the start because they won't be moved
@@ -142,9 +145,11 @@ unsigned int StringHandle::gcCyclesDone = 0;
 			}
 		}
 
+		heapUsed += currentBlock->allocated;
 		currentBlock = currentBlock->next;
 	}
-	spaceToRecycle = 0;
+
+	heapToRecycle = 0;
 	++gcCyclesDone;
 }
 
@@ -262,6 +267,7 @@ unsigned int StringHandle::gcCyclesDone = 0;
 			if (curBlock->slots[i].storage == nullptr)
 			{
 				curBlock->slots[i].refCount = 0;
+				++handlesUsed;
 				return &curBlock->slots[i];
 			}
 		}
@@ -271,7 +277,7 @@ unsigned int StringHandle::gcCyclesDone = 0;
 
 	// If we get here then we didn't find a free handle entry
 	IndexBlock * const newIndexBlock = new IndexBlock;
-	totalIndexSpace += sizeof(IndexBlock);
+	handlesAllocated += IndexBlockSlots;
 
 	if (prevIndexBlock == nullptr)
 	{
@@ -282,6 +288,7 @@ unsigned int StringHandle::gcCyclesDone = 0;
 		prevIndexBlock->next = newIndexBlock;
 	}
 
+	++handlesUsed;
 	return &newIndexBlock->slots[0];
 }
 
@@ -304,12 +311,13 @@ unsigned int StringHandle::gcCyclesDone = 0;
 				StorageSpace * const ret = reinterpret_cast<StorageSpace*>(currentBlock->data + currentBlock->allocated);
 				ret->length = length;
 				currentBlock->allocated += length + sizeof(StorageSpace::length);
+				heapUsed += length + sizeof(StorageSpace::length);
 				return ret;
 			}
 		}
 
 		// There is no space in any existing heap block. Decide whether to garbage collect and try again, or allocate a new block.
-		if (collected || spaceToRecycle < length * 4)
+		if (collected || heapToRecycle < length * 4)
 		{
 			break;
 		}
@@ -319,7 +327,7 @@ unsigned int StringHandle::gcCyclesDone = 0;
 
 	// Create a new heap block
 	heapRoot = new HeapBlock(heapRoot);
-	totalHeapSpace += sizeof(HeapBlock);
+	heapAllocated += HeapBlockSize - (length + sizeof(StorageSpace::length));
 	StorageSpace * const ret2 = reinterpret_cast<StorageSpace*>(heapRoot->data);
 	ret2->length = length;
 	heapRoot->allocated = length + sizeof(StorageSpace::length);
@@ -330,7 +338,7 @@ unsigned int StringHandle::gcCyclesDone = 0;
 {
 	if (ptr != nullptr)
 	{
-		spaceToRecycle += ptr->length;
+		heapToRecycle += ptr->length;
 		ptr->length |= 1;									// flag the space as unused
 	}
 }
@@ -385,12 +393,13 @@ void StringHandle::Delete() noexcept
 	}
 }
 
-void StringHandle::IncreaseRefCount() noexcept
+const StringHandle& StringHandle::IncreaseRefCount() const noexcept
 {
 	if (slotPtr != nullptr)
 	{
 		++slotPtr->refCount;
 	}
+	return *this;
 }
 
 // Caller must have at least a read lock when calling this
@@ -401,6 +410,7 @@ void StringHandle::InternalDelete() noexcept
 	{
 		ReleaseSpace(slotPtr->storage);						// release the space
 		slotPtr->storage = nullptr;							// release the handle entry
+		--handlesUsed;
 	}
 	slotPtr = nullptr;										// clear the pointer to the handle entry
 }
@@ -469,7 +479,8 @@ size_t StringHandle::GetLength() const noexcept
 	{
 		temp.copy("Heap OK");
 	}
-	temp.catf(", index memory %u, heap memory %u, reclaimable space %u, gc cycles %u\n", totalIndexSpace, totalHeapSpace, (unsigned int)spaceToRecycle, gcCyclesDone);
+	temp.catf(", handles allocated/used %u/%u, heap memory allocated/used/recyclable %u/%u/%u, gc cycles %u\n",
+					handlesAllocated, (unsigned int)handlesUsed, heapAllocated, heapUsed, (unsigned int)heapToRecycle, gcCyclesDone);
 	reprap.GetPlatform().Message(mt, temp.c_str());
 }
 
@@ -480,6 +491,11 @@ AutoStringHandle::AutoStringHandle(const AutoStringHandle& other) noexcept
 	IncreaseRefCount();
 }
 
+AutoStringHandle::AutoStringHandle(AutoStringHandle&& other) noexcept
+{
+	other.slotPtr = nullptr;
+}
+
 AutoStringHandle& AutoStringHandle::operator=(const AutoStringHandle& other) noexcept
 {
 	if (slotPtr != other.slotPtr)
diff --git a/src/Platform/Heap.h b/src/Platform/Heap.h
index fc465d94..4952af6b 100644
--- a/src/Platform/Heap.h
+++ b/src/Platform/Heap.h
@@ -30,7 +30,7 @@ public:
 	ReadLockedPointer<const char> Get() const noexcept;
 	size_t GetLength() const noexcept;
 	void Delete() noexcept;
-	void IncreaseRefCount() noexcept;
+	const StringHandle& IncreaseRefCount() const noexcept;
 	bool IsNull() const noexcept { return slotPtr == nullptr; }
 	void Assign(const char *s) noexcept;
 
@@ -56,9 +56,11 @@ protected:
 	static ReadWriteLock heapLock;
 	static IndexBlock *indexRoot;
 	static HeapBlock *heapRoot;
-	static std::atomic<size_t> spaceToRecycle;
-	static size_t totalIndexSpace;
-	static size_t totalHeapSpace;
+	static size_t handlesAllocated;
+	static std::atomic<size_t> handlesUsed;
+	static size_t heapAllocated;
+	static size_t heapUsed;
+	static std::atomic<size_t> heapToRecycle;
 	static unsigned int gcCyclesDone;
 };
 
@@ -70,6 +72,7 @@ public:
 	AutoStringHandle(const char *s) noexcept : StringHandle(s) { }
 	AutoStringHandle(const char *s, size_t len) noexcept : StringHandle(s, len) { }
 	AutoStringHandle(const AutoStringHandle& other) noexcept;
+	AutoStringHandle(AutoStringHandle&& other) noexcept;
 	AutoStringHandle& operator=(const AutoStringHandle& other) noexcept;
 	~AutoStringHandle();
 };
diff --git a/src/Platform/Platform.cpp b/src/Platform/Platform.cpp
index f515ecfb..53d0efaa 100644
--- a/src/Platform/Platform.cpp
+++ b/src/Platform/Platform.cpp
@@ -1824,6 +1824,8 @@ void Platform::Diagnostics(MessageType mtype) noexcept
 	lowestV12 = highestV12 = currentV12;
 #endif
 
+	StringHandle::Diagnostics(mtype);
+
 	// Show the motor position and stall status
 	for (size_t drive = 0; drive < NumDirectDrivers; ++drive)
 	{
@@ -1860,8 +1862,6 @@ void Platform::Diagnostics(MessageType mtype) noexcept
 
 	reprap.Timing(mtype);
 
-	StringHandle::Diagnostics(mtype);
-
 #if 0
 	// Debugging temperature readings
 	const uint32_t div = ThermistorAveragingFilter::NumAveraged() >> 2;		// 2 oversample bits