From 6d67b8e20c54505a61164cd5bf5f107a5bfc7447 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Fri, 9 Jan 2015 15:32:08 -0800 Subject: [PATCH 1/3] Don't call delete later unless you have to --- libraries/shared/src/DependencyManager.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libraries/shared/src/DependencyManager.h b/libraries/shared/src/DependencyManager.h index fdf8030199..2ccbe18dd8 100644 --- a/libraries/shared/src/DependencyManager.h +++ b/libraries/shared/src/DependencyManager.h @@ -13,6 +13,7 @@ #define hifi_DependencyManager_h #include +#include #include @@ -22,10 +23,12 @@ public:\ private:\ void customDeleter() {\ QObject* thisObject = dynamic_cast(this);\ - if (thisObject) {\ + if (thisObject && thisObject->parent()) {\ thisObject->deleteLater();\ + qDebug() << "Delete later:" << #T;\ } else {\ delete this;\ + qDebug() << "Deleted:" << #T;\ }\ }\ friend class DependencyManager; From 58b965c9a2b97eba1d8f3e6b022f1d4c9a1b673b Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 9 Jan 2015 16:12:43 -0800 Subject: [PATCH 2/3] lock Octree higher when extracting things from bag --- assignment-client/src/octree/OctreeSendThread.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/assignment-client/src/octree/OctreeSendThread.cpp b/assignment-client/src/octree/OctreeSendThread.cpp index ae34d54198..f0b4170cc6 100644 --- a/assignment-client/src/octree/OctreeSendThread.cpp +++ b/assignment-client/src/octree/OctreeSendThread.cpp @@ -404,6 +404,13 @@ int OctreeSendThread::packetDistributor(OctreeQueryNode* nodeData, bool viewFrus bool lastNodeDidntFit = false; // assume each node fits if (!nodeData->elementBag.isEmpty()) { + + quint64 lockWaitStart = usecTimestampNow(); + _myServer->getOctree()->lockForRead(); + quint64 lockWaitEnd = usecTimestampNow(); + lockWaitElapsedUsec = (float)(lockWaitEnd - lockWaitStart); + quint64 encodeStart = usecTimestampNow(); + OctreeElement* subTree = nodeData->elementBag.extract(); /* TODO: Looking for a way to prevent locking and encoding a tree that is not @@ -447,12 +454,6 @@ int OctreeSendThread::packetDistributor(OctreeQueryNode* nodeData, bool viewFrus // it seems like it may be a good idea to include the lock time as part of the encode time // are reported to client. Since you can encode without the lock nodeData->stats.encodeStarted(); - - quint64 lockWaitStart = usecTimestampNow(); - _myServer->getOctree()->lockForRead(); - quint64 lockWaitEnd = usecTimestampNow(); - lockWaitElapsedUsec = (float)(lockWaitEnd - lockWaitStart); - quint64 encodeStart = usecTimestampNow(); bytesWritten = _myServer->getOctree()->encodeTreeBitstream(subTree, &_packetData, nodeData->elementBag, params); From 99e1fdd46e227cde5412a21496db593ef6219648 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Sat, 10 Jan 2015 07:44:26 -0800 Subject: [PATCH 3/3] fix for EntityServer crash adding EntityItem::_element backpointer for easier add/remove logic --- .../src/EntityTreeRenderer.cpp | 2 + libraries/entities/src/EntityItem.cpp | 3 + libraries/entities/src/EntityItem.h | 4 ++ libraries/entities/src/EntityTreeElement.cpp | 13 ++++- .../entities/src/MovingEntitiesOperator.cpp | 15 +++-- .../entities/src/UpdateEntityOperator.cpp | 57 ++++++++----------- 6 files changed, 55 insertions(+), 39 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 47e9237ddc..3518e7b75c 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -98,7 +98,9 @@ void EntityTreeRenderer::init() { } QScriptValue EntityTreeRenderer::loadEntityScript(const EntityItemID& entityItemID) { + _tree->lockForRead(); EntityItem* entity = static_cast(_tree)->findEntityByEntityItemID(entityItemID); + _tree->unlock(); return loadEntityScript(entity); } diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index a744d39f05..6917dc1e7d 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -95,6 +95,7 @@ EntityItem::EntityItem(const EntityItemID& entityItemID) { _physicsInfo = NULL; _dirtyFlags = 0; _changedOnServer = 0; + _element = NULL; initFromEntityItemID(entityItemID); } @@ -110,6 +111,7 @@ EntityItem::EntityItem(const EntityItemID& entityItemID, const EntityItemPropert _physicsInfo = NULL; _dirtyFlags = 0; _changedOnServer = 0; + _element = NULL; initFromEntityItemID(entityItemID); setProperties(properties); } @@ -117,6 +119,7 @@ EntityItem::EntityItem(const EntityItemID& entityItemID, const EntityItemPropert EntityItem::~EntityItem() { // be sure to clean up _physicsInfo before calling this dtor assert(_physicsInfo == NULL); + assert(_element == NULL); } EntityPropertyFlags EntityItem::getEntityProperties(EncodeBitstreamParams& params) const { diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index e52be1707a..6d422ab257 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -39,6 +39,7 @@ class EntityTreeElementExtraEncodeData; /// to all other entity types. In particular: postion, size, rotation, age, lifetime, velocity, gravity. You can not instantiate /// one directly, instead you must only construct one of it's derived classes with additional features. class EntityItem { + friend class EntityTreeElement; public: enum EntityDirtyFlags { @@ -301,6 +302,7 @@ public: void* getPhysicsInfo() const { return _physicsInfo; } void setPhysicsInfo(void* data) { _physicsInfo = data; } + EntityTreeElement* getElement() const { return _element; } protected: virtual void initFromEntityItemID(const EntityItemID& entityItemID); // maybe useful to allow subclasses to init @@ -362,6 +364,8 @@ protected: // DirtyFlags are set whenever a property changes that the EntitySimulation needs to know about. uint32_t _dirtyFlags; // things that have changed from EXTERNAL changes (via script or packet) but NOT from simulation + + EntityTreeElement* _element; // back pointer to containing Element }; diff --git a/libraries/entities/src/EntityTreeElement.cpp b/libraries/entities/src/EntityTreeElement.cpp index 5f072164a0..4ced5fe844 100644 --- a/libraries/entities/src/EntityTreeElement.cpp +++ b/libraries/entities/src/EntityTreeElement.cpp @@ -682,6 +682,7 @@ void EntityTreeElement::cleanupEntities() { uint16_t numberOfEntities = _entityItems->size(); for (uint16_t i = 0; i < numberOfEntities; i++) { EntityItem* entity = (*_entityItems)[i]; + entity->_element = NULL; delete entity; } _entityItems->clear(); @@ -693,6 +694,7 @@ bool EntityTreeElement::removeEntityWithEntityItemID(const EntityItemID& id) { for (uint16_t i = 0; i < numberOfEntities; i++) { if ((*_entityItems)[i]->getEntityItemID() == id) { foundEntity = true; + (*_entityItems)[i]->_element = NULL; _entityItems->removeAt(i); break; } @@ -701,7 +703,13 @@ bool EntityTreeElement::removeEntityWithEntityItemID(const EntityItemID& id) { } bool EntityTreeElement::removeEntityItem(EntityItem* entity) { - return _entityItems->removeAll(entity) > 0; + int numEntries = _entityItems->removeAll(entity); + if (numEntries > 0) { + assert(entity->_element == this); + entity->_element = NULL; + return true; + } + return false; } @@ -808,7 +816,10 @@ int EntityTreeElement::readElementDataFromBuffer(const unsigned char* data, int } void EntityTreeElement::addEntityItem(EntityItem* entity) { + assert(entity); + assert(entity->_element == NULL); _entityItems->push_back(entity); + entity->_element = this; } // will average a "common reduced LOD view" from the the child elements... diff --git a/libraries/entities/src/MovingEntitiesOperator.cpp b/libraries/entities/src/MovingEntitiesOperator.cpp index 1ec67967b6..48ba8e4ec2 100644 --- a/libraries/entities/src/MovingEntitiesOperator.cpp +++ b/libraries/entities/src/MovingEntitiesOperator.cpp @@ -179,7 +179,7 @@ bool MovingEntitiesOperator::preRecursion(OctreeElement* element) { // If this is one of the old elements we're looking for, then ask it to remove the old entity if (!details.oldFound && entityTreeElement == details.oldContainingElement) { - entityTreeElement->removeEntityItem(details.entity); + // DO NOT remove the entity here. It will be removed when added to the destination element. _foundOldCount++; //details.oldFound = true; // TODO: would be nice to add this optimization if (_wantDebug) { @@ -193,8 +193,15 @@ bool MovingEntitiesOperator::preRecursion(OctreeElement* element) { // If this element is the best fit for the new bounds of this entity then add the entity to the element if (!details.newFound && entityTreeElement->bestFitBounds(details.newCube)) { EntityItemID entityItemID = details.entity->getEntityItemID(); - entityTreeElement->addEntityItem(details.entity); - _tree->setContainingElement(entityItemID, entityTreeElement); + // remove from the old before adding + EntityTreeElement* oldElement = details.entity->getElement(); + if (oldElement != entityTreeElement) { + if (oldElement) { + oldElement->removeEntityItem(details.entity); + } + entityTreeElement->addEntityItem(details.entity); + _tree->setContainingElement(entityItemID, entityTreeElement); + } _foundNewCount++; //details.newFound = true; // TODO: would be nice to add this optimization if (_wantDebug) { @@ -227,7 +234,7 @@ bool MovingEntitiesOperator::postRecursion(OctreeElement* element) { - // It's not OK to prune if we have the potential of deleting the original containig element. + // It's not OK to prune if we have the potential of deleting the original containing element // because if we prune the containing element then new might end up reallocating the same memory later // and that will confuse our logic. // diff --git a/libraries/entities/src/UpdateEntityOperator.cpp b/libraries/entities/src/UpdateEntityOperator.cpp index 9cee11d73c..c85c4235de 100644 --- a/libraries/entities/src/UpdateEntityOperator.cpp +++ b/libraries/entities/src/UpdateEntityOperator.cpp @@ -231,18 +231,19 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) { qDebug() << " *** REMOVING from ELEMENT ***"; } - entityTreeElement->removeEntityItem(_existingEntity); // NOTE: only removes the entity, doesn't delete it + // the entity knows what element it's in, so we remove it from that one + // NOTE: we know we haven't yet added it to its new element because _removeOld is true + EntityTreeElement* oldElement = _existingEntity->getElement(); + oldElement->removeEntityItem(_existingEntity); + _tree->setContainingElement(_entityItemID, NULL); - // If we haven't yet found the new location, then we need to - // make sure to remove our entity to element map, because for - // now we're not in that map - if (!_foundNew) { - _tree->setContainingElement(_entityItemID, NULL); - - if (_wantDebug) { - qDebug() << " *** REMOVING from MAP ***"; - } + if (oldElement != _containingElement) { + qDebug() << "WARNING entity moved during UpdateEntityOperator recursion"; + assert(! _containingElement->removeEntityItem(_existingEntity)); + } + if (_wantDebug) { + qDebug() << " *** REMOVING from MAP ***"; } } _foundOld = true; @@ -263,7 +264,6 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) { qDebug() << " entityTreeElement->bestFitBounds(_newEntityBox)=" << entityTreeElement->bestFitBounds(_newEntityBox); } - // If this element is the best fit for the new entity properties, then add/or update it if (entityTreeElement->bestFitBounds(_newEntityBox)) { @@ -271,33 +271,14 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) { qDebug() << " *** THIS ELEMENT IS BEST FIT ***"; } + EntityTreeElement* oldElement = _existingEntity->getElement(); // if we are the existing containing element, then we can just do the update of the entity properties - if (entityTreeElement == _containingElement) { + if (entityTreeElement == oldElement) { if (_wantDebug) { qDebug() << " *** This is the same OLD ELEMENT ***"; } - // TODO: We shouldn't be in a remove old case and also be the new best fit. This indicates that - // we have some kind of a logic error in this operator. But, it can handle it properly by setting - // the new properties for the entity and moving on. Still going to output a warning that if we - // see consistently we will want to address this. - if (_removeOld) { - qDebug() << "UNEXPECTED - UpdateEntityOperator - " - "we thought we needed to removeOld, but the old entity is our best fit."; - _removeOld = false; - - // if we thought we were supposed to remove the old item, and we already did, then we need - // to repair this case. - if (_foundOld) { - if (_wantDebug) { - qDebug() << " *** REPAIRING PREVIOUS REMOVAL from ELEMENT and MAP ***"; - } - entityTreeElement->addEntityItem(_existingEntity); - _tree->setContainingElement(_entityItemID, entityTreeElement); - } - } - // set the entity properties and mark our element as changed. _existingEntity->setProperties(_properties); if (_wantDebug) { @@ -305,14 +286,22 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) { } } else { // otherwise, this is an add case. + if (oldElement) { + oldElement->removeEntityItem(_existingEntity); + if (oldElement != _containingElement) { + qDebug() << "WARNING entity moved during UpdateEntityOperator recursion"; + } + } entityTreeElement->addEntityItem(_existingEntity); - _existingEntity->setProperties(_properties); // still need to update the properties! _tree->setContainingElement(_entityItemID, entityTreeElement); + + _existingEntity->setProperties(_properties); // still need to update the properties! if (_wantDebug) { qDebug() << " *** ADDING ENTITY to ELEMENT and MAP and SETTING PROPERTIES ***"; } } - _foundNew = true; // we found the new item! + _foundNew = true; // we found the new element + _removeOld = false; // and it has already been removed from the old } else { keepSearching = true; }