From 45672a6ccab933868acd42010900aceab803e2f8 Mon Sep 17 00:00:00 2001 From: David Crocker Date: Sun, 7 Mar 2021 17:22:07 +0000 Subject: ObjectTracker now uses the string heap instead of a StringBuffer --- src/GCodes/GCodeBuffer/ExpressionParser.h | 1 - src/GCodes/GCodeBuffer/StringParser.cpp | 1 - src/GCodes/GCodeBuffer/StringParser.h | 1 - src/GCodes/ObjectTracker.cpp | 32 ++++++++++++--------------- src/GCodes/ObjectTracker.h | 13 ++++------- src/ObjectModel/ObjectModel.cpp | 8 +++++++ src/ObjectModel/ObjectModel.h | 1 + src/Platform/Heap.cpp | 36 ++++++++++++++++++++++--------- src/Platform/Heap.h | 11 ++++++---- src/Platform/Platform.cpp | 4 ++-- 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 -#include #include #include 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 #include #include -#include #include // 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(numObjects, MaxTrackedObjects); ++i) { String 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::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 -#include +#include #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 StringHandle::spaceToRecycle = 0; -size_t StringHandle::totalIndexSpace = 0; -size_t StringHandle::totalHeapSpace = 0; +size_t StringHandle::handlesAllocated = 0; +std::atomic StringHandle::handlesUsed = 0; +size_t StringHandle::heapAllocated = 0; +size_t StringHandle::heapUsed = 0; +std::atomic 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(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(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 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 spaceToRecycle; - static size_t totalIndexSpace; - static size_t totalHeapSpace; + static size_t handlesAllocated; + static std::atomic handlesUsed; + static size_t heapAllocated; + static size_t heapUsed; + static std::atomic 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 -- cgit v1.2.3