From ef6d758e7f19084b3d25a507a3aaed0827c86226 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 10 May 2016 10:30:38 -0700 Subject: [PATCH 01/10] Fix JurisdictionMap multithreading issues Make octal code pointers use shared_ptr, add locks around access. --- interface/src/Application.cpp | 14 +- interface/src/ui/OctreeStatsDialog.cpp | 6 +- libraries/octree/src/JurisdictionMap.cpp | 187 ++++++++---------- libraries/octree/src/JurisdictionMap.h | 18 +- libraries/octree/src/OctreeHeadlessViewer.cpp | 8 +- libraries/octree/src/OctreeSceneStats.cpp | 96 +++------ libraries/octree/src/OctreeSceneStats.h | 9 +- libraries/shared/src/OctalCode.cpp | 9 +- libraries/shared/src/OctalCode.h | 7 +- 9 files changed, 138 insertions(+), 216 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 7a40c0b554..4d772d4b19 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -3656,11 +3656,11 @@ void Application::queryOctree(NodeType_t serverType, PacketType packetType, Node } else { const JurisdictionMap& map = (jurisdictions)[nodeUUID]; - unsigned char* rootCode = map.getRootOctalCode(); + auto rootCode = map.getRootOctalCode(); if (rootCode) { VoxelPositionSize rootDetails; - voxelDetailsForCode(rootCode, rootDetails); + voxelDetailsForCode(rootCode.get(), rootDetails); AACube serverBounds(glm::vec3(rootDetails.x * TREE_SCALE, rootDetails.y * TREE_SCALE, rootDetails.z * TREE_SCALE) - glm::vec3(HALF_TREE_SCALE), @@ -3720,11 +3720,11 @@ void Application::queryOctree(NodeType_t serverType, PacketType packetType, Node } else { const JurisdictionMap& map = (jurisdictions)[nodeUUID]; - unsigned char* rootCode = map.getRootOctalCode(); + auto rootCode = map.getRootOctalCode(); if (rootCode) { VoxelPositionSize rootDetails; - voxelDetailsForCode(rootCode, rootDetails); + voxelDetailsForCode(rootCode.get(), rootDetails); AACube serverBounds(glm::vec3(rootDetails.x * TREE_SCALE, rootDetails.y * TREE_SCALE, rootDetails.z * TREE_SCALE) - glm::vec3(HALF_TREE_SCALE), @@ -4251,9 +4251,9 @@ void Application::nodeKilled(SharedNodePointer node) { return; } - unsigned char* rootCode = _entityServerJurisdictions[nodeUUID].getRootOctalCode(); + auto rootCode = _entityServerJurisdictions[nodeUUID].getRootOctalCode(); VoxelPositionSize rootDetails; - voxelDetailsForCode(rootCode, rootDetails); + voxelDetailsForCode(rootCode.get(), rootDetails); qCDebug(interfaceapp, "model server going away...... v[%f, %f, %f, %f]", (double)rootDetails.x, (double)rootDetails.y, (double)rootDetails.z, (double)rootDetails.s); @@ -4378,7 +4378,7 @@ int Application::processOctreeStats(ReceivedMessage& message, SharedNodePointer } VoxelPositionSize rootDetails; - voxelDetailsForCode(octreeStats.getJurisdictionRoot(), rootDetails); + voxelDetailsForCode(octreeStats.getJurisdictionRoot().get(), rootDetails); qCDebug(interfaceapp, "stats from new %s server... [%f, %f, %f, %f]", qPrintable(serverType), diff --git a/interface/src/ui/OctreeStatsDialog.cpp b/interface/src/ui/OctreeStatsDialog.cpp index 6b6d600e20..fc0e6781d7 100644 --- a/interface/src/ui/OctreeStatsDialog.cpp +++ b/interface/src/ui/OctreeStatsDialog.cpp @@ -433,13 +433,13 @@ void OctreeStatsDialog::showOctreeServersOfType(int& serverCount, NodeType_t ser } const JurisdictionMap& map = serverJurisdictions[nodeUUID]; - unsigned char* rootCode = map.getRootOctalCode(); + auto rootCode = map.getRootOctalCode(); if (rootCode) { - QString rootCodeHex = octalCodeToHexString(rootCode); + QString rootCodeHex = octalCodeToHexString(rootCode.get()); VoxelPositionSize rootDetails; - voxelDetailsForCode(rootCode, rootDetails); + voxelDetailsForCode(rootCode.get(), rootDetails); AACube serverBounds(glm::vec3(rootDetails.x, rootDetails.y, rootDetails.z), rootDetails.s); serverDetails << " jurisdiction: " << qPrintable(rootCodeHex) diff --git a/libraries/octree/src/JurisdictionMap.cpp b/libraries/octree/src/JurisdictionMap.cpp index cac2211d29..86de7467d3 100644 --- a/libraries/octree/src/JurisdictionMap.cpp +++ b/libraries/octree/src/JurisdictionMap.cpp @@ -17,90 +17,11 @@ #include #include #include -#include #include "OctreeLogging.h" #include "JurisdictionMap.h" - -// standard assignment -// copy assignment -JurisdictionMap& JurisdictionMap::operator=(const JurisdictionMap& other) { - copyContents(other); - return *this; -} - -// Copy constructor -JurisdictionMap::JurisdictionMap(const JurisdictionMap& other) : _rootOctalCode(NULL) { - copyContents(other); -} - -void JurisdictionMap::copyContents(unsigned char* rootCodeIn, const std::vector& endNodesIn) { - unsigned char* rootCode; - std::vector endNodes; - if (rootCodeIn) { - size_t bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(rootCodeIn)); - rootCode = new unsigned char[bytes]; - memcpy(rootCode, rootCodeIn, bytes); - } else { - rootCode = new unsigned char[1]; - *rootCode = 0; - } - - for (size_t i = 0; i < endNodesIn.size(); i++) { - if (endNodesIn[i]) { - size_t bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(endNodesIn[i])); - unsigned char* endNodeCode = new unsigned char[bytes]; - memcpy(endNodeCode, endNodesIn[i], bytes); - endNodes.push_back(endNodeCode); - } - } - init(rootCode, endNodes); -} - -void JurisdictionMap::copyContents(const JurisdictionMap& other) { - _nodeType = other._nodeType; - copyContents(other._rootOctalCode, other._endNodes); -} - -JurisdictionMap::~JurisdictionMap() { - clear(); -} - -void JurisdictionMap::clear() { - if (_rootOctalCode) { - delete[] _rootOctalCode; - _rootOctalCode = NULL; - } - - for (size_t i = 0; i < _endNodes.size(); i++) { - if (_endNodes[i]) { - delete[] _endNodes[i]; - } - } - _endNodes.clear(); -} - -JurisdictionMap::JurisdictionMap(NodeType_t type) : _rootOctalCode(NULL) { - _nodeType = type; - unsigned char* rootCode = new unsigned char[1]; - *rootCode = 0; - - std::vector emptyEndNodes; - init(rootCode, emptyEndNodes); -} - -JurisdictionMap::JurisdictionMap(const char* filename) : _rootOctalCode(NULL) { - clear(); // clean up our own memory - readFromFile(filename); -} - -JurisdictionMap::JurisdictionMap(unsigned char* rootOctalCode, const std::vector& endNodes) - : _rootOctalCode(NULL) { - init(rootOctalCode, endNodes); -} - -void myDebugoutputBits(unsigned char byte, bool withNewLine) { +void myDebugOutputBits(unsigned char byte, bool withNewLine) { if (isalnum(byte)) { printf("[ %d (%c): ", byte, byte); } else { @@ -117,13 +38,12 @@ void myDebugoutputBits(unsigned char byte, bool withNewLine) { } } - void myDebugPrintOctalCode(const unsigned char* octalCode, bool withNewLine) { if (!octalCode) { - printf("NULL"); + printf("nullptr"); } else { for (size_t i = 0; i < bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(octalCode)); i++) { - myDebugoutputBits(octalCode[i],false); + myDebugOutputBits(octalCode[i], false); } } if (withNewLine) { @@ -131,16 +51,53 @@ void myDebugPrintOctalCode(const unsigned char* octalCode, bool withNewLine) { } } +// standard assignment +// copy assignment +JurisdictionMap& JurisdictionMap::operator=(const JurisdictionMap& other) { + copyContents(other); + return *this; +} + +// Copy constructor +JurisdictionMap::JurisdictionMap(const JurisdictionMap& other) : _rootOctalCode(nullptr) { + copyContents(other); +} + +void JurisdictionMap::copyContents(const OctalCodePtr& rootCodeIn, const std::vector& endNodesIn) { + init(rootCodeIn, endNodesIn); +} + +void JurisdictionMap::copyContents(const JurisdictionMap& other) { + _nodeType = other._nodeType; + + init(other.getRootOctalCode(), other.getEndNodeOctalCodes()); +} + +JurisdictionMap::~JurisdictionMap() { +} + +JurisdictionMap::JurisdictionMap(NodeType_t type) : _rootOctalCode(nullptr) { + _nodeType = type; + auto rootCode = std::shared_ptr(new unsigned char[1], std::default_delete()); + *rootCode = 0; + + std::vector emptyEndNodes; + init(rootCode, emptyEndNodes); +} + +JurisdictionMap::JurisdictionMap(const char* filename) : _rootOctalCode(nullptr) { + readFromFile(filename); +} JurisdictionMap::JurisdictionMap(const char* rootHexCode, const char* endNodesHexCodes) { qCDebug(octree, "JurisdictionMap::JurisdictionMap(const char* rootHexCode=[%p] %s, const char* endNodesHexCodes=[%p] %s)", rootHexCode, rootHexCode, endNodesHexCodes, endNodesHexCodes); - _rootOctalCode = hexStringToOctalCode(QString(rootHexCode)); + _rootOctalCode = std::shared_ptr(hexStringToOctalCode(QString(rootHexCode))); - qCDebug(octree, "JurisdictionMap::JurisdictionMap() _rootOctalCode=%p octalCode=", _rootOctalCode); - myDebugPrintOctalCode(_rootOctalCode, true); + qCDebug(octree, "JurisdictionMap::JurisdictionMap() _rootOctalCode=%p octalCode=", _rootOctalCode.get()); + myDebugPrintOctalCode(_rootOctalCode.get(), true); QString endNodesHexStrings(endNodesHexCodes); QString delimiterPattern(","); @@ -149,7 +106,7 @@ JurisdictionMap::JurisdictionMap(const char* rootHexCode, const char* endNodesHe for (int i = 0; i < endNodeList.size(); i++) { QString endNodeHexString = endNodeList.at(i); - unsigned char* endNodeOctcode = hexStringToOctalCode(endNodeHexString); + auto endNodeOctcode = hexStringToOctalCode(endNodeHexString); qCDebug(octree, "JurisdictionMap::JurisdictionMap() endNodeList(%d)=%s", i, endNodeHexString.toLocal8Bit().constData()); @@ -158,14 +115,14 @@ JurisdictionMap::JurisdictionMap(const char* rootHexCode, const char* endNodesHe _endNodes.push_back(endNodeOctcode); qCDebug(octree, "JurisdictionMap::JurisdictionMap() endNodeOctcode=%p octalCode=", endNodeOctcode); - myDebugPrintOctalCode(endNodeOctcode, true); + myDebugPrintOctalCode(endNodeOctcode.get(), true); } } -void JurisdictionMap::init(unsigned char* rootOctalCode, const std::vector& endNodes) { - clear(); // clean up our own memory +void JurisdictionMap::init(OctalCodePtr rootOctalCode, const std::vector& endNodes) { + std::lock_guard lock(_octalCodeMutex); _rootOctalCode = rootOctalCode; _endNodes = endNodes; } @@ -173,17 +130,19 @@ void JurisdictionMap::init(unsigned char* rootOctalCode, const std::vector lock(_octalCodeMutex); + // if the node is an ancestor of my root, then we return ABOVE - if (isAncestorOf(nodeOctalCode, _rootOctalCode)) { + if (isAncestorOf(nodeOctalCode, _rootOctalCode.get())) { return ABOVE; } // otherwise... - bool isInJurisdiction = isAncestorOf(_rootOctalCode, nodeOctalCode, childIndex); + bool isInJurisdiction = isAncestorOf(_rootOctalCode.get(), nodeOctalCode, childIndex); // if we're under the root, then we can't be under any of the endpoints if (isInJurisdiction) { for (size_t i = 0; i < _endNodes.size(); i++) { - bool isUnderEndNode = isAncestorOf(_endNodes[i], nodeOctalCode); + bool isUnderEndNode = isAncestorOf(_endNodes[i].get(), nodeOctalCode); if (isUnderEndNode) { isInJurisdiction = false; break; @@ -200,8 +159,9 @@ bool JurisdictionMap::readFromFile(const char* filename) { QString rootCode = settings.value("root","00").toString(); qCDebug(octree) << "rootCode=" << rootCode; - _rootOctalCode = hexStringToOctalCode(rootCode); - printOctalCode(_rootOctalCode); + std::lock_guard lock(_octalCodeMutex); + _rootOctalCode = std::shared_ptr(hexStringToOctalCode(rootCode)); + printOctalCode(_rootOctalCode.get()); settings.beginGroup("endNodes"); const QStringList childKeys = settings.childKeys(); @@ -211,8 +171,8 @@ bool JurisdictionMap::readFromFile(const char* filename) { values.insert(childKey, childValue); qCDebug(octree) << childKey << "=" << childValue; - unsigned char* octcode = hexStringToOctalCode(childValue); - printOctalCode(octcode); + auto octcode = hexStringToOctalCode(childValue); + printOctalCode(octcode.get()); _endNodes.push_back(octcode); } @@ -221,30 +181,34 @@ bool JurisdictionMap::readFromFile(const char* filename) { } void JurisdictionMap::displayDebugDetails() const { - QString rootNodeValue = octalCodeToHexString(_rootOctalCode); + std::lock_guard lock(_octalCodeMutex); + + QString rootNodeValue = octalCodeToHexString(_rootOctalCode.get()); qCDebug(octree) << "root:" << rootNodeValue; for (size_t i = 0; i < _endNodes.size(); i++) { - QString value = octalCodeToHexString(_endNodes[i]); + QString value = octalCodeToHexString(_endNodes[i].get()); qCDebug(octree) << "End node[" << i << "]: " << rootNodeValue; } } bool JurisdictionMap::writeToFile(const char* filename) { + std::lock_guard lock(_octalCodeMutex); + QString settingsFile(filename); QSettings settings(settingsFile, QSettings::IniFormat); - QString rootNodeValue = octalCodeToHexString(_rootOctalCode); + QString rootNodeValue = octalCodeToHexString(_rootOctalCode.get()); settings.setValue("root", rootNodeValue); settings.beginGroup("endNodes"); for (size_t i = 0; i < _endNodes.size(); i++) { QString key = QString("endnode%1").arg(i); - QString value = octalCodeToHexString(_endNodes[i]); + QString value = octalCodeToHexString(_endNodes[i].get()); settings.setValue(key, value); } settings.endGroup(); @@ -271,18 +235,19 @@ std::unique_ptr JurisdictionMap::packIntoPacket() { packet->writePrimitive(type); // add the root jurisdiction + std::lock_guard lock(_octalCodeMutex); if (_rootOctalCode) { - size_t bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(_rootOctalCode)); + size_t bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(_rootOctalCode.get())); // No root or end node details to pack! packet->writePrimitive(bytes); - packet->write(reinterpret_cast(_rootOctalCode), bytes); + packet->write(reinterpret_cast(_rootOctalCode.get()), bytes); // if and only if there's a root jurisdiction, also include the end nodes int endNodeCount = (int)_endNodes.size(); packet->writePrimitive(endNodeCount); for (int i=0; i < endNodeCount; i++) { - unsigned char* endNodeCode = _endNodes[i]; + auto endNodeCode = _endNodes[i].get(); size_t bytes = 0; if (endNodeCode) { bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(endNodeCode)); @@ -299,15 +264,17 @@ std::unique_ptr JurisdictionMap::packIntoPacket() { } int JurisdictionMap::unpackFromPacket(ReceivedMessage& message) { - clear(); - // read the root jurisdiction int bytes = 0; message.readPrimitive(&bytes); + std::lock_guard lock(_octalCodeMutex); + _rootOctalCode = nullptr; + _endNodes.clear(); + if (bytes > 0 && bytes <= message.getBytesLeftToRead()) { - _rootOctalCode = new unsigned char[bytes]; - message.read(reinterpret_cast(_rootOctalCode), bytes); + _rootOctalCode = std::shared_ptr(new unsigned char[bytes], std::default_delete()); + message.read(reinterpret_cast(_rootOctalCode.get()), bytes); // if and only if there's a root jurisdiction, also include the end nodes int endNodeCount = 0; @@ -318,8 +285,8 @@ int JurisdictionMap::unpackFromPacket(ReceivedMessage& message) { message.readPrimitive(&bytes); if (bytes <= message.getBytesLeftToRead()) { - unsigned char* endNodeCode = new unsigned char[bytes]; - message.read(reinterpret_cast(endNodeCode), bytes); + auto endNodeCode = std::shared_ptr(new unsigned char[bytes], std::default_delete()); + message.read(reinterpret_cast(endNodeCode.get()), bytes); // if the endNodeCode was 0 length then don't add it if (bytes > 0) { diff --git a/libraries/octree/src/JurisdictionMap.h b/libraries/octree/src/JurisdictionMap.h index fb217e59db..0deedb41e1 100644 --- a/libraries/octree/src/JurisdictionMap.h +++ b/libraries/octree/src/JurisdictionMap.h @@ -23,6 +23,7 @@ #include #include +#include class JurisdictionMap { public: @@ -41,7 +42,7 @@ public: // application constructors JurisdictionMap(const char* filename); - JurisdictionMap(unsigned char* rootOctalCode, const std::vector& endNodes); +// JurisdictionMap(unsigned char* rootOctalCode, const std::vector& endNodes); JurisdictionMap(const char* rootHextString, const char* endNodesHextString); ~JurisdictionMap(); @@ -50,11 +51,10 @@ public: bool writeToFile(const char* filename); bool readFromFile(const char* filename); - unsigned char* getRootOctalCode() const { return _rootOctalCode; } - unsigned char* getEndNodeOctalCode(int index) const { return _endNodes[index]; } - int getEndNodeCount() const { return (int)_endNodes.size(); } + OctalCodePtr getRootOctalCode() const { return _rootOctalCode; } + std::vector getEndNodeOctalCodes() const { return _endNodes; } - void copyContents(unsigned char* rootCodeIn, const std::vector& endNodesIn); + void copyContents(const OctalCodePtr& rootCodeIn, const std::vector& endNodesIn); int unpackFromPacket(ReceivedMessage& message); std::unique_ptr packIntoPacket(); @@ -69,11 +69,11 @@ public: private: void copyContents(const JurisdictionMap& other); // use assignment instead - void clear(); - void init(unsigned char* rootOctalCode, const std::vector& endNodes); + void init(OctalCodePtr rootOctalCode, const std::vector& endNodes); - unsigned char* _rootOctalCode; - std::vector _endNodes; + mutable std::mutex _octalCodeMutex; + OctalCodePtr _rootOctalCode { nullptr }; + std::vector _endNodes; NodeType_t _nodeType; }; diff --git a/libraries/octree/src/OctreeHeadlessViewer.cpp b/libraries/octree/src/OctreeHeadlessViewer.cpp index 4a0a76cd02..28d3794a54 100644 --- a/libraries/octree/src/OctreeHeadlessViewer.cpp +++ b/libraries/octree/src/OctreeHeadlessViewer.cpp @@ -78,12 +78,12 @@ void OctreeHeadlessViewer::queryOctree() { } const JurisdictionMap& map = (jurisdictions)[nodeUUID]; - unsigned char* rootCode = map.getRootOctalCode(); + auto rootCode = map.getRootOctalCode(); if (!rootCode) { return; } - voxelDetailsForCode(rootCode, rootDetails); + voxelDetailsForCode(rootCode.get(), rootDetails); foundRootDetails = true; }); @@ -146,7 +146,7 @@ void OctreeHeadlessViewer::queryOctree() { } const JurisdictionMap& map = (jurisdictions)[nodeUUID]; - unsigned char* rootCode = map.getRootOctalCode(); + auto rootCode = map.getRootOctalCode(); if (!rootCode) { if (wantExtraDebugging) { @@ -154,7 +154,7 @@ void OctreeHeadlessViewer::queryOctree() { } return; } - voxelDetailsForCode(rootCode, rootDetails); + voxelDetailsForCode(rootCode.get(), rootDetails); foundRootDetails = true; }); diff --git a/libraries/octree/src/OctreeSceneStats.cpp b/libraries/octree/src/OctreeSceneStats.cpp index 578d35b687..fd770ccd67 100644 --- a/libraries/octree/src/OctreeSceneStats.cpp +++ b/libraries/octree/src/OctreeSceneStats.cpp @@ -110,29 +110,21 @@ void OctreeSceneStats::copyFromOther(const OctreeSceneStats& other) { _treesRemoved = other._treesRemoved; // before copying the jurisdictions, delete any current values... - if (_jurisdictionRoot) { - delete[] _jurisdictionRoot; - _jurisdictionRoot = NULL; - } - for (size_t i = 0; i < _jurisdictionEndNodes.size(); i++) { - if (_jurisdictionEndNodes[i]) { - delete[] _jurisdictionEndNodes[i]; - } - } + _jurisdictionRoot = nullptr; _jurisdictionEndNodes.clear(); // Now copy the values from the other if (other._jurisdictionRoot) { - auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(other._jurisdictionRoot)); - _jurisdictionRoot = new unsigned char[bytes]; - memcpy(_jurisdictionRoot, other._jurisdictionRoot, bytes); + auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(other._jurisdictionRoot.get())); + _jurisdictionRoot = OctalCodePtr(new unsigned char[bytes]); + memcpy(_jurisdictionRoot.get(), other._jurisdictionRoot.get(), bytes); } for (size_t i = 0; i < other._jurisdictionEndNodes.size(); i++) { - unsigned char* endNodeCode = other._jurisdictionEndNodes[i]; + auto& endNodeCode = other._jurisdictionEndNodes[i]; if (endNodeCode) { - auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(endNodeCode)); - unsigned char* endNodeCodeCopy = new unsigned char[bytes]; - memcpy(endNodeCodeCopy, endNodeCode, bytes); + auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(endNodeCode.get())); + auto endNodeCodeCopy = OctalCodePtr(new unsigned char[bytes]); + memcpy(endNodeCodeCopy.get(), endNodeCode.get(), bytes); _jurisdictionEndNodes.push_back(endNodeCodeCopy); } } @@ -162,37 +154,15 @@ void OctreeSceneStats::sceneStarted(bool isFullScene, bool isMoving, OctreeEleme _isFullScene = isFullScene; _isMoving = isMoving; - if (_jurisdictionRoot) { - delete[] _jurisdictionRoot; - _jurisdictionRoot = NULL; - } + _jurisdictionRoot = nullptr; + // clear existing endNodes before copying new ones... - for (size_t i=0; i < _jurisdictionEndNodes.size(); i++) { - if (_jurisdictionEndNodes[i]) { - delete[] _jurisdictionEndNodes[i]; - } - } _jurisdictionEndNodes.clear(); // setup jurisdictions if (jurisdictionMap) { - unsigned char* jurisdictionRoot = jurisdictionMap->getRootOctalCode(); - if (jurisdictionRoot) { - auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(jurisdictionRoot)); - _jurisdictionRoot = new unsigned char[bytes]; - memcpy(_jurisdictionRoot, jurisdictionRoot, bytes); - } - - // copy new endNodes... - for (int i = 0; i < jurisdictionMap->getEndNodeCount(); i++) { - unsigned char* endNodeCode = jurisdictionMap->getEndNodeOctalCode(i); - if (endNodeCode) { - auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(endNodeCode)); - unsigned char* endNodeCodeCopy = new unsigned char[bytes]; - memcpy(endNodeCodeCopy, endNodeCode, bytes); - _jurisdictionEndNodes.push_back(endNodeCodeCopy); - } - } + _jurisdictionRoot = jurisdictionMap->getRootOctalCode(); + _jurisdictionEndNodes = jurisdictionMap->getEndNodeOctalCodes(); } } @@ -270,15 +240,7 @@ void OctreeSceneStats::reset() { _existsInPacketBitsWritten = 0; _treesRemoved = 0; - if (_jurisdictionRoot) { - delete[] _jurisdictionRoot; - _jurisdictionRoot = NULL; - } - for (size_t i = 0; i < _jurisdictionEndNodes.size(); i++) { - if (_jurisdictionEndNodes[i]) { - delete[] _jurisdictionEndNodes[i]; - } - } + _jurisdictionRoot = nullptr; _jurisdictionEndNodes.clear(); } @@ -418,9 +380,9 @@ int OctreeSceneStats::packIntoPacket() { // add the root jurisdiction if (_jurisdictionRoot) { // copy the - int bytes = (int)bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(_jurisdictionRoot)); + int bytes = (int)bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(_jurisdictionRoot.get())); _statsPacket->writePrimitive(bytes); - _statsPacket->write(reinterpret_cast(_jurisdictionRoot), bytes); + _statsPacket->write(reinterpret_cast(_jurisdictionRoot.get()), bytes); // if and only if there's a root jurisdiction, also include the end elements int endNodeCount = (int)_jurisdictionEndNodes.size(); @@ -428,10 +390,10 @@ int OctreeSceneStats::packIntoPacket() { _statsPacket->writePrimitive(endNodeCount); for (int i=0; i < endNodeCount; i++) { - unsigned char* endNodeCode = _jurisdictionEndNodes[i]; - auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(endNodeCode)); + auto& endNodeCode = _jurisdictionEndNodes[i]; + auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(endNodeCode.get())); _statsPacket->writePrimitive(bytes); - _statsPacket->write(reinterpret_cast(endNodeCode), bytes); + _statsPacket->write(reinterpret_cast(endNodeCode.get()), bytes); } } else { int bytes = 0; @@ -500,17 +462,7 @@ int OctreeSceneStats::unpackFromPacket(ReceivedMessage& packet) { packet.readPrimitive(&_existsInPacketBitsWritten); packet.readPrimitive(&_treesRemoved); // before allocating new juridiction, clean up existing ones - if (_jurisdictionRoot) { - delete[] _jurisdictionRoot; - _jurisdictionRoot = NULL; - } - - // clear existing endNodes before copying new ones... - for (size_t i = 0; i < _jurisdictionEndNodes.size(); i++) { - if (_jurisdictionEndNodes[i]) { - delete[] _jurisdictionEndNodes[i]; - } - } + _jurisdictionRoot = nullptr; _jurisdictionEndNodes.clear(); // read the root jurisdiction @@ -518,11 +470,11 @@ int OctreeSceneStats::unpackFromPacket(ReceivedMessage& packet) { packet.readPrimitive(&bytes); if (bytes == 0) { - _jurisdictionRoot = NULL; + _jurisdictionRoot = nullptr; _jurisdictionEndNodes.clear(); } else { - _jurisdictionRoot = new unsigned char[bytes]; - packet.read(reinterpret_cast(_jurisdictionRoot), bytes); + _jurisdictionRoot = OctalCodePtr(new unsigned char[bytes]); + packet.read(reinterpret_cast(_jurisdictionRoot.get()), bytes); // if and only if there's a root jurisdiction, also include the end elements _jurisdictionEndNodes.clear(); @@ -535,8 +487,8 @@ int OctreeSceneStats::unpackFromPacket(ReceivedMessage& packet) { packet.readPrimitive(&bytes); - unsigned char* endNodeCode = new unsigned char[bytes]; - packet.read(reinterpret_cast(endNodeCode), bytes); + auto endNodeCode = OctalCodePtr(new unsigned char[bytes]); + packet.read(reinterpret_cast(endNodeCode.get()), bytes); _jurisdictionEndNodes.push_back(endNodeCode); } diff --git a/libraries/octree/src/OctreeSceneStats.h b/libraries/octree/src/OctreeSceneStats.h index 11aa0b82b2..a888ad2485 100644 --- a/libraries/octree/src/OctreeSceneStats.h +++ b/libraries/octree/src/OctreeSceneStats.h @@ -20,6 +20,7 @@ #include "JurisdictionMap.h" #include "OctreePacketData.h" #include "SequenceNumberStats.h" +#include "OctalCode.h" #define GREENISH 0x40ff40d0 #define YELLOWISH 0xffef40c0 @@ -143,10 +144,10 @@ public: const char* getItemValue(Item item); /// Returns OctCode for root element of the jurisdiction of this particular octree server - unsigned char* getJurisdictionRoot() const { return _jurisdictionRoot; } + OctalCodePtr getJurisdictionRoot() const { return _jurisdictionRoot; } /// Returns list of OctCodes for end elements of the jurisdiction of this particular octree server - const std::vector& getJurisdictionEndNodes() const { return _jurisdictionEndNodes; } + const std::vector& getJurisdictionEndNodes() const { return _jurisdictionEndNodes; } bool isMoving() const { return _isMoving; } bool isFullScene() const { return _isFullScene; } @@ -277,8 +278,8 @@ private: static const int MAX_ITEM_VALUE_LENGTH = 128; char _itemValueBuffer[MAX_ITEM_VALUE_LENGTH]; - unsigned char* _jurisdictionRoot; - std::vector _jurisdictionEndNodes; + OctalCodePtr _jurisdictionRoot; + std::vector _jurisdictionEndNodes; }; /// Map between element IDs and their reported OctreeSceneStats. Typically used by classes that need to know which elements sent diff --git a/libraries/shared/src/OctalCode.cpp b/libraries/shared/src/OctalCode.cpp index 1fa18903cb..802ddbbb64 100644 --- a/libraries/shared/src/OctalCode.cpp +++ b/libraries/shared/src/OctalCode.cpp @@ -304,14 +304,14 @@ bool isAncestorOf(const unsigned char* possibleAncestor, const unsigned char* po return true; } -unsigned char* hexStringToOctalCode(const QString& input) { +std::shared_ptr hexStringToOctalCode(const QString& input) { const int HEX_NUMBER_BASE = 16; const int HEX_BYTE_SIZE = 2; int stringIndex = 0; int byteArrayIndex = 0; // allocate byte array based on half of string length - unsigned char* bytes = new unsigned char[(input.length()) / HEX_BYTE_SIZE]; + auto bytes = std::shared_ptr(new unsigned char[(input.length()) / HEX_BYTE_SIZE]); // loop through the string - 2 bytes at a time converting // it to decimal equivalent and store in byte array @@ -321,15 +321,14 @@ unsigned char* hexStringToOctalCode(const QString& input) { if (!ok) { break; } - bytes[byteArrayIndex] = (unsigned char)value; + bytes.get()[byteArrayIndex] = (unsigned char)value; stringIndex += HEX_BYTE_SIZE; byteArrayIndex++; } // something went wrong if (!ok) { - delete[] bytes; - return NULL; + return nullptr; } return bytes; } diff --git a/libraries/shared/src/OctalCode.h b/libraries/shared/src/OctalCode.h index 09766b685a..e8892fb40b 100644 --- a/libraries/shared/src/OctalCode.h +++ b/libraries/shared/src/OctalCode.h @@ -12,9 +12,10 @@ #ifndef hifi_OctalCode_h #define hifi_OctalCode_h -#include #include +#include + const int BITS_IN_OCTAL = 3; const int NUMBER_OF_COLORS = 3; // RGB! const int SIZE_OF_COLOR_DATA = NUMBER_OF_COLORS * sizeof(unsigned char); // size in bytes @@ -22,6 +23,8 @@ const int RED_INDEX = 0; const int GREEN_INDEX = 1; const int BLUE_INDEX = 2; +using OctalCodePtr = std::shared_ptr; + void printOctalCode(const unsigned char* octalCode); size_t bytesRequiredForCodeLength(unsigned char threeBitCodes); int branchIndexWithDescendant(const unsigned char* ancestorOctalCode, const unsigned char* descendantOctalCode); @@ -58,6 +61,6 @@ typedef enum { OctalCodeComparison compareOctalCodes(const unsigned char* code1, const unsigned char* code2); QString octalCodeToHexString(const unsigned char* octalCode); -unsigned char* hexStringToOctalCode(const QString& input); +std::shared_ptr hexStringToOctalCode(const QString& input); #endif // hifi_OctalCode_h From e819ab8475bfed2a6365defee8f0b4fa3fb9ecae Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 11 May 2016 09:59:02 -0700 Subject: [PATCH 02/10] Add mutex protection around octal code getters --- libraries/octree/src/JurisdictionMap.cpp | 26 +++++++++++++++++++---- libraries/octree/src/JurisdictionMap.h | 9 ++++---- libraries/octree/src/OctreeSceneStats.cpp | 11 ++++------ libraries/shared/src/OctalCode.cpp | 2 +- libraries/shared/src/OctalCode.h | 3 ++- 5 files changed, 34 insertions(+), 17 deletions(-) diff --git a/libraries/octree/src/JurisdictionMap.cpp b/libraries/octree/src/JurisdictionMap.cpp index 86de7467d3..30a8de1bda 100644 --- a/libraries/octree/src/JurisdictionMap.cpp +++ b/libraries/octree/src/JurisdictionMap.cpp @@ -70,7 +70,12 @@ void JurisdictionMap::copyContents(const OctalCodePtr& rootCodeIn, const std::ve void JurisdictionMap::copyContents(const JurisdictionMap& other) { _nodeType = other._nodeType; - init(other.getRootOctalCode(), other.getEndNodeOctalCodes()); + OctalCodePtr rootOctalCode; + OctalCodePtrList endNodes; + + std::tie(rootOctalCode, endNodes) = other.getRootOctalCodeAndEndNodes(); + + init(rootOctalCode, endNodes); } JurisdictionMap::~JurisdictionMap() { @@ -120,6 +125,20 @@ JurisdictionMap::JurisdictionMap(const char* rootHexCode, const char* endNodesHe } } +std::tuple JurisdictionMap::getRootOctalCodeAndEndNodes() const { + std::lock_guard lock(_octalCodeMutex); + return std::tuple>(_rootOctalCode, _endNodes); +} + +OctalCodePtr JurisdictionMap::getRootOctalCode() const { + std::lock_guard lock(_octalCodeMutex); + return _rootOctalCode; +} + +OctalCodePtrList JurisdictionMap::getEndNodeOctalCodes() const { + std::lock_guard lock(_octalCodeMutex); + return _endNodes; +} void JurisdictionMap::init(OctalCodePtr rootOctalCode, const std::vector& endNodes) { std::lock_guard lock(_octalCodeMutex); @@ -160,7 +179,7 @@ bool JurisdictionMap::readFromFile(const char* filename) { qCDebug(octree) << "rootCode=" << rootCode; std::lock_guard lock(_octalCodeMutex); - _rootOctalCode = std::shared_ptr(hexStringToOctalCode(rootCode)); + _rootOctalCode = hexStringToOctalCode(rootCode); printOctalCode(_rootOctalCode.get()); settings.beginGroup("endNodes"); @@ -195,11 +214,10 @@ void JurisdictionMap::displayDebugDetails() const { bool JurisdictionMap::writeToFile(const char* filename) { - std::lock_guard lock(_octalCodeMutex); - QString settingsFile(filename); QSettings settings(settingsFile, QSettings::IniFormat); + std::lock_guard lock(_octalCodeMutex); QString rootNodeValue = octalCodeToHexString(_rootOctalCode.get()); diff --git a/libraries/octree/src/JurisdictionMap.h b/libraries/octree/src/JurisdictionMap.h index 0deedb41e1..3b08fb221c 100644 --- a/libraries/octree/src/JurisdictionMap.h +++ b/libraries/octree/src/JurisdictionMap.h @@ -51,10 +51,11 @@ public: bool writeToFile(const char* filename); bool readFromFile(const char* filename); - OctalCodePtr getRootOctalCode() const { return _rootOctalCode; } - std::vector getEndNodeOctalCodes() const { return _endNodes; } + std::tuple JurisdictionMap::getRootOctalCodeAndEndNodes() const; + OctalCodePtr getRootOctalCode() const; + OctalCodePtrList getEndNodeOctalCodes() const; - void copyContents(const OctalCodePtr& rootCodeIn, const std::vector& endNodesIn); + void copyContents(const OctalCodePtr& rootCodeIn, const OctalCodePtrList& endNodesIn); int unpackFromPacket(ReceivedMessage& message); std::unique_ptr packIntoPacket(); @@ -73,7 +74,7 @@ private: mutable std::mutex _octalCodeMutex; OctalCodePtr _rootOctalCode { nullptr }; - std::vector _endNodes; + OctalCodePtrList _endNodes; NodeType_t _nodeType; }; diff --git a/libraries/octree/src/OctreeSceneStats.cpp b/libraries/octree/src/OctreeSceneStats.cpp index fd770ccd67..ca2bff32a4 100644 --- a/libraries/octree/src/OctreeSceneStats.cpp +++ b/libraries/octree/src/OctreeSceneStats.cpp @@ -154,15 +154,12 @@ void OctreeSceneStats::sceneStarted(bool isFullScene, bool isMoving, OctreeEleme _isFullScene = isFullScene; _isMoving = isMoving; - _jurisdictionRoot = nullptr; - - // clear existing endNodes before copying new ones... - _jurisdictionEndNodes.clear(); - // setup jurisdictions if (jurisdictionMap) { - _jurisdictionRoot = jurisdictionMap->getRootOctalCode(); - _jurisdictionEndNodes = jurisdictionMap->getEndNodeOctalCodes(); + std::tie(_jurisdictionRoot, _jurisdictionEndNodes) = jurisdictionMap->getRootOctalCodeAndEndNodes(); + } else { + _jurisdictionRoot = nullptr; + _jurisdictionEndNodes.clear(); } } diff --git a/libraries/shared/src/OctalCode.cpp b/libraries/shared/src/OctalCode.cpp index 802ddbbb64..85b6c2b632 100644 --- a/libraries/shared/src/OctalCode.cpp +++ b/libraries/shared/src/OctalCode.cpp @@ -304,7 +304,7 @@ bool isAncestorOf(const unsigned char* possibleAncestor, const unsigned char* po return true; } -std::shared_ptr hexStringToOctalCode(const QString& input) { +OctalCodePtr hexStringToOctalCode(const QString& input) { const int HEX_NUMBER_BASE = 16; const int HEX_BYTE_SIZE = 2; int stringIndex = 0; diff --git a/libraries/shared/src/OctalCode.h b/libraries/shared/src/OctalCode.h index e8892fb40b..7d1424d954 100644 --- a/libraries/shared/src/OctalCode.h +++ b/libraries/shared/src/OctalCode.h @@ -24,6 +24,7 @@ const int GREEN_INDEX = 1; const int BLUE_INDEX = 2; using OctalCodePtr = std::shared_ptr; +using OctalCodePtrList = std::vector; void printOctalCode(const unsigned char* octalCode); size_t bytesRequiredForCodeLength(unsigned char threeBitCodes); @@ -61,6 +62,6 @@ typedef enum { OctalCodeComparison compareOctalCodes(const unsigned char* code1, const unsigned char* code2); QString octalCodeToHexString(const unsigned char* octalCode); -std::shared_ptr hexStringToOctalCode(const QString& input); +OctalCodePtr hexStringToOctalCode(const QString& input); #endif // hifi_OctalCode_h From 35f147f55703987d91678530d0356e348c370680 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 11 May 2016 10:37:54 -0700 Subject: [PATCH 03/10] Cleanup use of OctalCodePtrList and add allocateOctalCodePtr --- libraries/octree/src/JurisdictionMap.cpp | 14 +++++++------- libraries/octree/src/JurisdictionMap.h | 4 ++-- libraries/octree/src/OctreeSceneStats.cpp | 8 ++++---- libraries/octree/src/OctreeSceneStats.h | 2 +- libraries/shared/src/OctalCode.cpp | 6 +++++- libraries/shared/src/OctalCode.h | 1 + 6 files changed, 20 insertions(+), 15 deletions(-) diff --git a/libraries/octree/src/JurisdictionMap.cpp b/libraries/octree/src/JurisdictionMap.cpp index 30a8de1bda..7eb976690b 100644 --- a/libraries/octree/src/JurisdictionMap.cpp +++ b/libraries/octree/src/JurisdictionMap.cpp @@ -63,7 +63,7 @@ JurisdictionMap::JurisdictionMap(const JurisdictionMap& other) : _rootOctalCode( copyContents(other); } -void JurisdictionMap::copyContents(const OctalCodePtr& rootCodeIn, const std::vector& endNodesIn) { +void JurisdictionMap::copyContents(const OctalCodePtr& rootCodeIn, const OctalCodePtrList& endNodesIn) { init(rootCodeIn, endNodesIn); } @@ -83,10 +83,10 @@ JurisdictionMap::~JurisdictionMap() { JurisdictionMap::JurisdictionMap(NodeType_t type) : _rootOctalCode(nullptr) { _nodeType = type; - auto rootCode = std::shared_ptr(new unsigned char[1], std::default_delete()); + OctalCodePtr rootCode = allocateOctalCodePtr(1); *rootCode = 0; - std::vector emptyEndNodes; + OctalCodePtrList emptyEndNodes; init(rootCode, emptyEndNodes); } @@ -127,7 +127,7 @@ JurisdictionMap::JurisdictionMap(const char* rootHexCode, const char* endNodesHe std::tuple JurisdictionMap::getRootOctalCodeAndEndNodes() const { std::lock_guard lock(_octalCodeMutex); - return std::tuple>(_rootOctalCode, _endNodes); + return std::tuple(_rootOctalCode, _endNodes); } OctalCodePtr JurisdictionMap::getRootOctalCode() const { @@ -140,7 +140,7 @@ OctalCodePtrList JurisdictionMap::getEndNodeOctalCodes() const { return _endNodes; } -void JurisdictionMap::init(OctalCodePtr rootOctalCode, const std::vector& endNodes) { +void JurisdictionMap::init(OctalCodePtr rootOctalCode, const OctalCodePtrList& endNodes) { std::lock_guard lock(_octalCodeMutex); _rootOctalCode = rootOctalCode; _endNodes = endNodes; @@ -291,7 +291,7 @@ int JurisdictionMap::unpackFromPacket(ReceivedMessage& message) { _endNodes.clear(); if (bytes > 0 && bytes <= message.getBytesLeftToRead()) { - _rootOctalCode = std::shared_ptr(new unsigned char[bytes], std::default_delete()); + _rootOctalCode = allocateOctalCodePtr(bytes); message.read(reinterpret_cast(_rootOctalCode.get()), bytes); // if and only if there's a root jurisdiction, also include the end nodes @@ -303,7 +303,7 @@ int JurisdictionMap::unpackFromPacket(ReceivedMessage& message) { message.readPrimitive(&bytes); if (bytes <= message.getBytesLeftToRead()) { - auto endNodeCode = std::shared_ptr(new unsigned char[bytes], std::default_delete()); + auto endNodeCode = allocateOctalCodePtr(bytes); message.read(reinterpret_cast(endNodeCode.get()), bytes); // if the endNodeCode was 0 length then don't add it diff --git a/libraries/octree/src/JurisdictionMap.h b/libraries/octree/src/JurisdictionMap.h index 3b08fb221c..995b88e3ff 100644 --- a/libraries/octree/src/JurisdictionMap.h +++ b/libraries/octree/src/JurisdictionMap.h @@ -42,8 +42,8 @@ public: // application constructors JurisdictionMap(const char* filename); -// JurisdictionMap(unsigned char* rootOctalCode, const std::vector& endNodes); JurisdictionMap(const char* rootHextString, const char* endNodesHextString); + ~JurisdictionMap(); Area isMyJurisdiction(const unsigned char* nodeOctalCode, int childIndex) const; @@ -70,7 +70,7 @@ public: private: void copyContents(const JurisdictionMap& other); // use assignment instead - void init(OctalCodePtr rootOctalCode, const std::vector& endNodes); + void init(OctalCodePtr rootOctalCode, const OctalCodePtrList& endNodes); mutable std::mutex _octalCodeMutex; OctalCodePtr _rootOctalCode { nullptr }; diff --git a/libraries/octree/src/OctreeSceneStats.cpp b/libraries/octree/src/OctreeSceneStats.cpp index ca2bff32a4..f164cd2d48 100644 --- a/libraries/octree/src/OctreeSceneStats.cpp +++ b/libraries/octree/src/OctreeSceneStats.cpp @@ -116,14 +116,14 @@ void OctreeSceneStats::copyFromOther(const OctreeSceneStats& other) { // Now copy the values from the other if (other._jurisdictionRoot) { auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(other._jurisdictionRoot.get())); - _jurisdictionRoot = OctalCodePtr(new unsigned char[bytes]); + _jurisdictionRoot = allocateOctalCodePtr(bytes); memcpy(_jurisdictionRoot.get(), other._jurisdictionRoot.get(), bytes); } for (size_t i = 0; i < other._jurisdictionEndNodes.size(); i++) { auto& endNodeCode = other._jurisdictionEndNodes[i]; if (endNodeCode) { auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(endNodeCode.get())); - auto endNodeCodeCopy = OctalCodePtr(new unsigned char[bytes]); + auto endNodeCodeCopy = allocateOctalCodePtr(bytes); memcpy(endNodeCodeCopy.get(), endNodeCode.get(), bytes); _jurisdictionEndNodes.push_back(endNodeCodeCopy); } @@ -470,7 +470,7 @@ int OctreeSceneStats::unpackFromPacket(ReceivedMessage& packet) { _jurisdictionRoot = nullptr; _jurisdictionEndNodes.clear(); } else { - _jurisdictionRoot = OctalCodePtr(new unsigned char[bytes]); + _jurisdictionRoot = allocateOctalCodePtr(bytes); packet.read(reinterpret_cast(_jurisdictionRoot.get()), bytes); // if and only if there's a root jurisdiction, also include the end elements @@ -484,7 +484,7 @@ int OctreeSceneStats::unpackFromPacket(ReceivedMessage& packet) { packet.readPrimitive(&bytes); - auto endNodeCode = OctalCodePtr(new unsigned char[bytes]); + auto endNodeCode = allocateOctalCodePtr(bytes); packet.read(reinterpret_cast(endNodeCode.get()), bytes); _jurisdictionEndNodes.push_back(endNodeCode); diff --git a/libraries/octree/src/OctreeSceneStats.h b/libraries/octree/src/OctreeSceneStats.h index a888ad2485..e7d697f785 100644 --- a/libraries/octree/src/OctreeSceneStats.h +++ b/libraries/octree/src/OctreeSceneStats.h @@ -147,7 +147,7 @@ public: OctalCodePtr getJurisdictionRoot() const { return _jurisdictionRoot; } /// Returns list of OctCodes for end elements of the jurisdiction of this particular octree server - const std::vector& getJurisdictionEndNodes() const { return _jurisdictionEndNodes; } + const OctalCodePtrList& getJurisdictionEndNodes() const { return _jurisdictionEndNodes; } bool isMoving() const { return _isMoving; } bool isFullScene() const { return _isFullScene; } diff --git a/libraries/shared/src/OctalCode.cpp b/libraries/shared/src/OctalCode.cpp index 85b6c2b632..d6b9759619 100644 --- a/libraries/shared/src/OctalCode.cpp +++ b/libraries/shared/src/OctalCode.cpp @@ -304,6 +304,10 @@ bool isAncestorOf(const unsigned char* possibleAncestor, const unsigned char* po return true; } +OctalCodePtr allocateOctalCodePtr(size_t size) { + return OctalCodePtr(new unsigned char[size], std::default_delete()); +} + OctalCodePtr hexStringToOctalCode(const QString& input) { const int HEX_NUMBER_BASE = 16; const int HEX_BYTE_SIZE = 2; @@ -311,7 +315,7 @@ OctalCodePtr hexStringToOctalCode(const QString& input) { int byteArrayIndex = 0; // allocate byte array based on half of string length - auto bytes = std::shared_ptr(new unsigned char[(input.length()) / HEX_BYTE_SIZE]); + auto bytes = allocateOctalCodePtr(input.length() / HEX_BYTE_SIZE); // loop through the string - 2 bytes at a time converting // it to decimal equivalent and store in byte array diff --git a/libraries/shared/src/OctalCode.h b/libraries/shared/src/OctalCode.h index 7d1424d954..50381cb0d7 100644 --- a/libraries/shared/src/OctalCode.h +++ b/libraries/shared/src/OctalCode.h @@ -61,6 +61,7 @@ typedef enum { OctalCodeComparison compareOctalCodes(const unsigned char* code1, const unsigned char* code2); +OctalCodePtr allocateOctalCodePtr(size_t size); QString octalCodeToHexString(const unsigned char* octalCode); OctalCodePtr hexStringToOctalCode(const QString& input); From 5fc18eafda6f4d5614bfbbbeecd04a30b8bc5445 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 11 May 2016 10:43:22 -0700 Subject: [PATCH 04/10] Rename OctalCodePtr related functions --- libraries/octree/src/JurisdictionMap.cpp | 10 +++++----- libraries/octree/src/JurisdictionMap.h | 3 ++- libraries/octree/src/OctreeSceneStats.cpp | 10 +++++----- libraries/shared/src/OctalCode.cpp | 4 ++-- libraries/shared/src/OctalCode.h | 2 +- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/libraries/octree/src/JurisdictionMap.cpp b/libraries/octree/src/JurisdictionMap.cpp index 7eb976690b..1349b48b8d 100644 --- a/libraries/octree/src/JurisdictionMap.cpp +++ b/libraries/octree/src/JurisdictionMap.cpp @@ -73,7 +73,7 @@ void JurisdictionMap::copyContents(const JurisdictionMap& other) { OctalCodePtr rootOctalCode; OctalCodePtrList endNodes; - std::tie(rootOctalCode, endNodes) = other.getRootOctalCodeAndEndNodes(); + std::tie(rootOctalCode, endNodes) = other.getRootAndEndNodeOctalCodes(); init(rootOctalCode, endNodes); } @@ -83,7 +83,7 @@ JurisdictionMap::~JurisdictionMap() { JurisdictionMap::JurisdictionMap(NodeType_t type) : _rootOctalCode(nullptr) { _nodeType = type; - OctalCodePtr rootCode = allocateOctalCodePtr(1); + OctalCodePtr rootCode = createOctalCodePtr(1); *rootCode = 0; OctalCodePtrList emptyEndNodes; @@ -125,7 +125,7 @@ JurisdictionMap::JurisdictionMap(const char* rootHexCode, const char* endNodesHe } } -std::tuple JurisdictionMap::getRootOctalCodeAndEndNodes() const { +std::tuple JurisdictionMap::getRootAndEndNodeOctalCodes() const { std::lock_guard lock(_octalCodeMutex); return std::tuple(_rootOctalCode, _endNodes); } @@ -291,7 +291,7 @@ int JurisdictionMap::unpackFromPacket(ReceivedMessage& message) { _endNodes.clear(); if (bytes > 0 && bytes <= message.getBytesLeftToRead()) { - _rootOctalCode = allocateOctalCodePtr(bytes); + _rootOctalCode = createOctalCodePtr(bytes); message.read(reinterpret_cast(_rootOctalCode.get()), bytes); // if and only if there's a root jurisdiction, also include the end nodes @@ -303,7 +303,7 @@ int JurisdictionMap::unpackFromPacket(ReceivedMessage& message) { message.readPrimitive(&bytes); if (bytes <= message.getBytesLeftToRead()) { - auto endNodeCode = allocateOctalCodePtr(bytes); + auto endNodeCode = createOctalCodePtr(bytes); message.read(reinterpret_cast(endNodeCode.get()), bytes); // if the endNodeCode was 0 length then don't add it diff --git a/libraries/octree/src/JurisdictionMap.h b/libraries/octree/src/JurisdictionMap.h index 995b88e3ff..2e39447eb0 100644 --- a/libraries/octree/src/JurisdictionMap.h +++ b/libraries/octree/src/JurisdictionMap.h @@ -51,7 +51,8 @@ public: bool writeToFile(const char* filename); bool readFromFile(const char* filename); - std::tuple JurisdictionMap::getRootOctalCodeAndEndNodes() const; + // Provide an atomic way to get both the rootOctalCode and endNodeOctalCodes. + std::tuple JurisdictionMap::getRootAndEndNodeOctalCodes() const; OctalCodePtr getRootOctalCode() const; OctalCodePtrList getEndNodeOctalCodes() const; diff --git a/libraries/octree/src/OctreeSceneStats.cpp b/libraries/octree/src/OctreeSceneStats.cpp index f164cd2d48..fdaa6b4928 100644 --- a/libraries/octree/src/OctreeSceneStats.cpp +++ b/libraries/octree/src/OctreeSceneStats.cpp @@ -116,14 +116,14 @@ void OctreeSceneStats::copyFromOther(const OctreeSceneStats& other) { // Now copy the values from the other if (other._jurisdictionRoot) { auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(other._jurisdictionRoot.get())); - _jurisdictionRoot = allocateOctalCodePtr(bytes); + _jurisdictionRoot = createOctalCodePtr(bytes); memcpy(_jurisdictionRoot.get(), other._jurisdictionRoot.get(), bytes); } for (size_t i = 0; i < other._jurisdictionEndNodes.size(); i++) { auto& endNodeCode = other._jurisdictionEndNodes[i]; if (endNodeCode) { auto bytes = bytesRequiredForCodeLength(numberOfThreeBitSectionsInCode(endNodeCode.get())); - auto endNodeCodeCopy = allocateOctalCodePtr(bytes); + auto endNodeCodeCopy = createOctalCodePtr(bytes); memcpy(endNodeCodeCopy.get(), endNodeCode.get(), bytes); _jurisdictionEndNodes.push_back(endNodeCodeCopy); } @@ -156,7 +156,7 @@ void OctreeSceneStats::sceneStarted(bool isFullScene, bool isMoving, OctreeEleme // setup jurisdictions if (jurisdictionMap) { - std::tie(_jurisdictionRoot, _jurisdictionEndNodes) = jurisdictionMap->getRootOctalCodeAndEndNodes(); + std::tie(_jurisdictionRoot, _jurisdictionEndNodes) = jurisdictionMap->getRootAndEndNodeOctalCodes(); } else { _jurisdictionRoot = nullptr; _jurisdictionEndNodes.clear(); @@ -470,7 +470,7 @@ int OctreeSceneStats::unpackFromPacket(ReceivedMessage& packet) { _jurisdictionRoot = nullptr; _jurisdictionEndNodes.clear(); } else { - _jurisdictionRoot = allocateOctalCodePtr(bytes); + _jurisdictionRoot = createOctalCodePtr(bytes); packet.read(reinterpret_cast(_jurisdictionRoot.get()), bytes); // if and only if there's a root jurisdiction, also include the end elements @@ -484,7 +484,7 @@ int OctreeSceneStats::unpackFromPacket(ReceivedMessage& packet) { packet.readPrimitive(&bytes); - auto endNodeCode = allocateOctalCodePtr(bytes); + auto endNodeCode = createOctalCodePtr(bytes); packet.read(reinterpret_cast(endNodeCode.get()), bytes); _jurisdictionEndNodes.push_back(endNodeCode); diff --git a/libraries/shared/src/OctalCode.cpp b/libraries/shared/src/OctalCode.cpp index d6b9759619..1a0a19bf44 100644 --- a/libraries/shared/src/OctalCode.cpp +++ b/libraries/shared/src/OctalCode.cpp @@ -304,7 +304,7 @@ bool isAncestorOf(const unsigned char* possibleAncestor, const unsigned char* po return true; } -OctalCodePtr allocateOctalCodePtr(size_t size) { +OctalCodePtr createOctalCodePtr(size_t size) { return OctalCodePtr(new unsigned char[size], std::default_delete()); } @@ -315,7 +315,7 @@ OctalCodePtr hexStringToOctalCode(const QString& input) { int byteArrayIndex = 0; // allocate byte array based on half of string length - auto bytes = allocateOctalCodePtr(input.length() / HEX_BYTE_SIZE); + auto bytes = createOctalCodePtr(input.length() / HEX_BYTE_SIZE); // loop through the string - 2 bytes at a time converting // it to decimal equivalent and store in byte array diff --git a/libraries/shared/src/OctalCode.h b/libraries/shared/src/OctalCode.h index 50381cb0d7..3fd9a49390 100644 --- a/libraries/shared/src/OctalCode.h +++ b/libraries/shared/src/OctalCode.h @@ -61,7 +61,7 @@ typedef enum { OctalCodeComparison compareOctalCodes(const unsigned char* code1, const unsigned char* code2); -OctalCodePtr allocateOctalCodePtr(size_t size); +OctalCodePtr createOctalCodePtr(size_t size); QString octalCodeToHexString(const unsigned char* octalCode); OctalCodePtr hexStringToOctalCode(const QString& input); From 368eded8eec6bdd897b731bf13c03425483454d2 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 11 May 2016 10:45:38 -0700 Subject: [PATCH 05/10] Remove unnecessary type in Jurisdictionmap --- libraries/octree/src/JurisdictionMap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/octree/src/JurisdictionMap.cpp b/libraries/octree/src/JurisdictionMap.cpp index 1349b48b8d..128a4aa162 100644 --- a/libraries/octree/src/JurisdictionMap.cpp +++ b/libraries/octree/src/JurisdictionMap.cpp @@ -99,7 +99,7 @@ JurisdictionMap::JurisdictionMap(const char* rootHexCode, const char* endNodesHe qCDebug(octree, "JurisdictionMap::JurisdictionMap(const char* rootHexCode=[%p] %s, const char* endNodesHexCodes=[%p] %s)", rootHexCode, rootHexCode, endNodesHexCodes, endNodesHexCodes); - _rootOctalCode = std::shared_ptr(hexStringToOctalCode(QString(rootHexCode))); + _rootOctalCode = hexStringToOctalCode(QString(rootHexCode)); qCDebug(octree, "JurisdictionMap::JurisdictionMap() _rootOctalCode=%p octalCode=", _rootOctalCode.get()); myDebugPrintOctalCode(_rootOctalCode.get(), true); From 1bec38584b52cf7770d991012f2e94d06aa3e03a Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 11 May 2016 11:11:52 -0700 Subject: [PATCH 06/10] Remove class qualification from header file --- libraries/octree/src/JurisdictionMap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/octree/src/JurisdictionMap.h b/libraries/octree/src/JurisdictionMap.h index 2e39447eb0..b5a311c3d9 100644 --- a/libraries/octree/src/JurisdictionMap.h +++ b/libraries/octree/src/JurisdictionMap.h @@ -52,7 +52,7 @@ public: bool readFromFile(const char* filename); // Provide an atomic way to get both the rootOctalCode and endNodeOctalCodes. - std::tuple JurisdictionMap::getRootAndEndNodeOctalCodes() const; + std::tuple getRootAndEndNodeOctalCodes() const; OctalCodePtr getRootOctalCode() const; OctalCodePtrList getEndNodeOctalCodes() const; From 475d881b043a2842806c6f3c29b8186ff829994b Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 11 May 2016 11:31:51 -0700 Subject: [PATCH 07/10] Fix logging of shared_ptr --- libraries/octree/src/JurisdictionMap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/octree/src/JurisdictionMap.cpp b/libraries/octree/src/JurisdictionMap.cpp index 128a4aa162..5d5d50f4ad 100644 --- a/libraries/octree/src/JurisdictionMap.cpp +++ b/libraries/octree/src/JurisdictionMap.cpp @@ -119,7 +119,7 @@ JurisdictionMap::JurisdictionMap(const char* rootHexCode, const char* endNodesHe //printOctalCode(endNodeOctcode); _endNodes.push_back(endNodeOctcode); - qCDebug(octree, "JurisdictionMap::JurisdictionMap() endNodeOctcode=%p octalCode=", endNodeOctcode); + qCDebug(octree, "JurisdictionMap::JurisdictionMap() endNodeOctcode=%p octalCode=", endNodeOctcode.get()); myDebugPrintOctalCode(endNodeOctcode.get(), true); } From 013d902376b332df07a70b4aaee9642ee9d03498 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 11 May 2016 11:50:00 -0700 Subject: [PATCH 08/10] Add vector include to OctalCode.h --- libraries/shared/src/OctalCode.h | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/shared/src/OctalCode.h b/libraries/shared/src/OctalCode.h index 3fd9a49390..a0d86f32d2 100644 --- a/libraries/shared/src/OctalCode.h +++ b/libraries/shared/src/OctalCode.h @@ -12,6 +12,7 @@ #ifndef hifi_OctalCode_h #define hifi_OctalCode_h +#include #include #include From 47d897f66967415d399e3ef5054dc311b522b634 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 17 May 2016 15:12:14 -0700 Subject: [PATCH 09/10] Fix processOctreeStats recreating JurisdictionMap for every stat packet --- interface/src/Application.cpp | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 4d772d4b19..a60adeceb5 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -4372,8 +4372,11 @@ int Application::processOctreeStats(ReceivedMessage& message, SharedNodePointer serverType = "Entity"; } + bool found = false; + jurisdiction->withReadLock([&] { if (jurisdiction->find(nodeUUID) != jurisdiction->end()) { + found = true; return; } @@ -4384,15 +4387,18 @@ int Application::processOctreeStats(ReceivedMessage& message, SharedNodePointer qPrintable(serverType), (double)rootDetails.x, (double)rootDetails.y, (double)rootDetails.z, (double)rootDetails.s); }); - // store jurisdiction details for later use - // This is bit of fiddling is because JurisdictionMap assumes it is the owner of the values used to construct it - // but OctreeSceneStats thinks it's just returning a reference to its contents. So we need to make a copy of the - // details from the OctreeSceneStats to construct the JurisdictionMap - JurisdictionMap jurisdictionMap; - jurisdictionMap.copyContents(octreeStats.getJurisdictionRoot(), octreeStats.getJurisdictionEndNodes()); - jurisdiction->withWriteLock([&] { - (*jurisdiction)[nodeUUID] = jurisdictionMap; - }); + + if (!found) { + // store jurisdiction details for later use + // This is bit of fiddling is because JurisdictionMap assumes it is the owner of the values used to construct it + // but OctreeSceneStats thinks it's just returning a reference to its contents. So we need to make a copy of the + // details from the OctreeSceneStats to construct the JurisdictionMap + JurisdictionMap jurisdictionMap; + jurisdictionMap.copyContents(octreeStats.getJurisdictionRoot(), octreeStats.getJurisdictionEndNodes()); + jurisdiction->withWriteLock([&] { + (*jurisdiction)[nodeUUID] = jurisdictionMap; + }); + } }); return statsMessageLength; From 68665e6b2579fa63e9cf0af6ce293af78afdf1dd Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 17 May 2016 15:12:45 -0700 Subject: [PATCH 10/10] Fix JurisdictionMap not initializing correctly --- libraries/octree/src/JurisdictionMap.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/libraries/octree/src/JurisdictionMap.cpp b/libraries/octree/src/JurisdictionMap.cpp index 5d5d50f4ad..fcbc085339 100644 --- a/libraries/octree/src/JurisdictionMap.cpp +++ b/libraries/octree/src/JurisdictionMap.cpp @@ -64,7 +64,14 @@ JurisdictionMap::JurisdictionMap(const JurisdictionMap& other) : _rootOctalCode( } void JurisdictionMap::copyContents(const OctalCodePtr& rootCodeIn, const OctalCodePtrList& endNodesIn) { - init(rootCodeIn, endNodesIn); + OctalCodePtr rootCode = rootCodeIn; + if (!rootCode) { + rootCode = createOctalCodePtr(1); + *rootCode = 0; + } + + OctalCodePtrList emptyEndNodes; + init(rootCode, endNodesIn); } void JurisdictionMap::copyContents(const JurisdictionMap& other) {