diff --git a/libraries/entities/src/AddEntityOperator.cpp b/libraries/entities/src/AddEntityOperator.cpp index e86e70dd80..78d986f538 100644 --- a/libraries/entities/src/AddEntityOperator.cpp +++ b/libraries/entities/src/AddEntityOperator.cpp @@ -46,10 +46,8 @@ bool AddEntityOperator::preRecursion(const OctreeElementPointer& element) { // If this element is the best fit for the new entity properties, then add/or update it if (entityTreeElement->bestFitBounds(_newEntityBox)) { - + _tree->addEntityMapEntry(_newEntity); entityTreeElement->addEntityItem(_newEntity); - _tree->setContainingElement(_newEntity->getEntityItemID(), entityTreeElement); - _foundNew = true; keepSearching = false; } else { diff --git a/libraries/entities/src/DeleteEntityOperator.cpp b/libraries/entities/src/DeleteEntityOperator.cpp index 709c281341..cbd0ad7839 100644 --- a/libraries/entities/src/DeleteEntityOperator.cpp +++ b/libraries/entities/src/DeleteEntityOperator.cpp @@ -96,7 +96,7 @@ bool DeleteEntityOperator::preRecursion(const OctreeElementPointer& element) { bool entityDeleted = entityTreeElement->removeEntityItem(theEntity); // remove it from the element assert(entityDeleted); (void)entityDeleted; // quite warning - _tree->setContainingElement(details.entity->getEntityItemID(), NULL); // update or id to element lookup + _tree->clearEntityMapEntry(details.entity->getEntityItemID()); _foundCount++; } } diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 23ce097cc2..5996327e87 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -89,7 +89,8 @@ EntityItem::EntityItem(const EntityItemID& entityItemID) : EntityItem::~EntityItem() { // clear out any left-over actions - EntityTreePointer entityTree = _element ? _element->getTree() : nullptr; + EntityTreeElementPointer element = _element; // use local copy of _element for logic below + EntityTreePointer entityTree = element ? element->getTree() : nullptr; EntitySimulationPointer simulation = entityTree ? entityTree->getSimulation() : nullptr; if (simulation) { clearActions(simulation); @@ -880,8 +881,9 @@ int EntityItem::readEntityDataFromBuffer(const unsigned char* data, int bytesLef // Tracking for editing roundtrips here. We will tell our EntityTree that we just got incoming data about // and entity that was edited at some time in the past. The tree will determine how it wants to track this // information. - if (_element && _element->getTree()) { - _element->getTree()->trackIncomingEntityLastEdited(lastEditedFromBufferAdjusted, bytesRead); + EntityTreeElementPointer element = _element; // use local copy of _element for logic below + if (element && element->getTree()) { + element->getTree()->trackIncomingEntityLastEdited(lastEditedFromBufferAdjusted, bytesRead); } @@ -2056,7 +2058,8 @@ bool EntityItem::removeActionInternal(const QUuid& actionID, EntitySimulationPoi _previouslyDeletedActions.insert(actionID, usecTimestampNow()); if (_objectActions.contains(actionID)) { if (!simulation) { - EntityTreePointer entityTree = _element ? _element->getTree() : nullptr; + EntityTreeElementPointer element = _element; // use local copy of _element for logic below + EntityTreePointer entityTree = element ? element->getTree() : nullptr; simulation = entityTree ? entityTree->getSimulation() : nullptr; } diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 4773f45af7..5e58736477 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -90,13 +90,17 @@ void EntityTree::eraseAllOctreeElements(bool createNewRoot) { if (_simulation) { _simulation->clearEntities(); } - { - QWriteLocker locker(&_entityToElementLock); - foreach(EntityTreeElementPointer element, _entityToElementMap) { - element->cleanupEntities(); + QHash localMap; + localMap.swap(_entityMap); + this->withWriteLock([&] { + foreach(EntityItemPointer entity, localMap) { + EntityTreeElementPointer element = entity->getElement(); + if (element) { + element->cleanupEntities(); + } } - _entityToElementMap.clear(); - } + }); + localMap.clear(); Octree::eraseAllOctreeElements(createNewRoot); resetClientEditStats(); @@ -136,29 +140,24 @@ void EntityTree::postAddEntity(EntityItemPointer entity) { } bool EntityTree::updateEntity(const EntityItemID& entityID, const EntityItemProperties& properties, const SharedNodePointer& senderNode) { - EntityTreeElementPointer containingElement = getContainingElement(entityID); + EntityItemPointer entity; + { + QReadLocker locker(&_entityMapLock); + entity = _entityMap.value(entityID); + } + if (!entity) { + return false; + } + return updateEntity(entity, properties, senderNode); +} + +bool EntityTree::updateEntity(EntityItemPointer entity, const EntityItemProperties& origProperties, + const SharedNodePointer& senderNode) { + EntityTreeElementPointer containingElement = entity->getElement(); if (!containingElement) { return false; } - EntityItemPointer existingEntity = containingElement->getEntityWithEntityItemID(entityID); - if (!existingEntity) { - return false; - } - - return updateEntityWithElement(existingEntity, properties, containingElement, senderNode); -} - -bool EntityTree::updateEntity(EntityItemPointer entity, const EntityItemProperties& properties, const SharedNodePointer& senderNode) { - EntityTreeElementPointer containingElement = getContainingElement(entity->getEntityItemID()); - if (!containingElement) { - return false; - } - return updateEntityWithElement(entity, properties, containingElement, senderNode); -} - -bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityItemProperties& origProperties, - EntityTreeElementPointer containingElement, const SharedNodePointer& senderNode) { EntityItemProperties properties = origProperties; bool allowLockChange; @@ -190,7 +189,7 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI bool success; AACube queryCube = entity->getQueryAACube(success); if (!success) { - qCDebug(entities) << "Warning -- failed to get query-cube for" << entity->getID(); + qCWarning(entities) << "failed to get query-cube for" << entity->getID(); } UpdateEntityOperator theOperator(getThisPointer(), containingElement, entity, queryCube); recurseTreeWithOperator(&theOperator); @@ -331,9 +330,8 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI } // TODO: this final containingElement check should eventually be removed (or wrapped in an #ifdef DEBUG). - containingElement = getContainingElement(entity->getEntityItemID()); - if (!containingElement) { - qCDebug(entities) << "UNEXPECTED!!!! after updateEntity() we no longer have a containing element??? entityID=" + if (!entity->getElement()) { + qCWarning(entities) << "EntityTree::updateEntity() we no longer have a containing element for entityID=" << entity->getEntityItemID(); return false; } @@ -366,7 +364,7 @@ EntityItemPointer EntityTree::addEntity(const EntityItemID& entityID, const Enti // You should not call this on existing entities that are already part of the tree! Call updateEntity() EntityTreeElementPointer containingElement = getContainingElement(entityID); if (containingElement) { - qCDebug(entities) << "UNEXPECTED!!! ----- don't call addEntity() on existing entity items. entityID=" << entityID + qCWarning(entities) << "EntityTree::addEntity() on existing entity item with entityID=" << entityID << "containingElement=" << containingElement.get(); return result; } @@ -422,7 +420,7 @@ void EntityTree::deleteEntity(const EntityItemID& entityID, bool force, bool ign EntityTreeElementPointer containingElement = getContainingElement(entityID); if (!containingElement) { if (!ignoreWarnings) { - qCDebug(entities) << "UNEXPECTED!!!! EntityTree::deleteEntity() entityID doesn't exist!!! entityID=" << entityID; + qCWarning(entities) << "EntityTree::deleteEntity() on non-existent entityID=" << entityID; } return; } @@ -430,8 +428,7 @@ void EntityTree::deleteEntity(const EntityItemID& entityID, bool force, bool ign EntityItemPointer existingEntity = containingElement->getEntityWithEntityItemID(entityID); if (!existingEntity) { if (!ignoreWarnings) { - qCDebug(entities) << "UNEXPECTED!!!! don't call EntityTree::deleteEntity() on entity items that don't exist. " - "entityID=" << entityID; + qCWarning(entities) << "EntityTree::deleteEntity() on non-existant entity item with entityID=" << entityID; } return; } @@ -478,7 +475,7 @@ void EntityTree::deleteEntities(QSet entityIDs, bool force, bool i EntityTreeElementPointer containingElement = getContainingElement(entityID); if (!containingElement) { if (!ignoreWarnings) { - qCDebug(entities) << "UNEXPECTED!!!! EntityTree::deleteEntities() entityID doesn't exist!!! entityID=" << entityID; + qCWarning(entities) << "EntityTree::deleteEntities() on non-existent entityID=" << entityID; } continue; } @@ -486,8 +483,7 @@ void EntityTree::deleteEntities(QSet entityIDs, bool force, bool i EntityItemPointer existingEntity = containingElement->getEntityWithEntityItemID(entityID); if (!existingEntity) { if (!ignoreWarnings) { - qCDebug(entities) << "UNEXPECTED!!!! don't call EntityTree::deleteEntities() on entity items that don't exist. " - "entityID=" << entityID; + qCWarning(entities) << "EntityTree::deleteEntities() on non-existent entity item with entityID=" << entityID; } continue; } @@ -975,7 +971,7 @@ int EntityTree::processEditPacketData(ReceivedMessage& message, const unsigned c const SharedNodePointer& senderNode) { if (!getIsServer()) { - qCDebug(entities) << "UNEXPECTED!!! processEditPacketData() should only be called on a server tree."; + qCWarning(entities) << "EntityTree::processEditPacketData() should only be called on a server tree."; return 0; } @@ -1502,27 +1498,43 @@ int EntityTree::processEraseMessageDetails(const QByteArray& dataByteArray, cons } EntityTreeElementPointer EntityTree::getContainingElement(const EntityItemID& entityItemID) /*const*/ { - QReadLocker locker(&_entityToElementLock); - EntityTreeElementPointer element = _entityToElementMap.value(entityItemID); - return element; + EntityItemPointer entity; + { + QReadLocker locker(&_entityMapLock); + entity = _entityMap.value(entityItemID); + } + if (entity) { + return entity->getElement(); + } + return EntityTreeElementPointer(nullptr); } -void EntityTree::setContainingElement(const EntityItemID& entityItemID, EntityTreeElementPointer element) { - QWriteLocker locker(&_entityToElementLock); - if (element) { - _entityToElementMap[entityItemID] = element; - } else { - _entityToElementMap.remove(entityItemID); +void EntityTree::addEntityMapEntry(EntityItemPointer entity) { + EntityItemID id = entity->getEntityItemID(); + QWriteLocker locker(&_entityMapLock); + EntityItemPointer otherEntity = _entityMap.value(id); + if (otherEntity) { + qCWarning(entities) << "EntityTree::addEntityMapEntry() found pre-existing id " << id; + assert(false); + return; } + _entityMap.insert(id, entity); +} + +void EntityTree::clearEntityMapEntry(const EntityItemID& id) { + QWriteLocker locker(&_entityMapLock); + _entityMap.remove(id); } void EntityTree::debugDumpMap() { + // QHash's are implicitly shared, so we make a shared copy and use that instead. + // This way we might be able to avoid both a lock and a true copy. + QHash localMap(_entityMap); qCDebug(entities) << "EntityTree::debugDumpMap() --------------------------"; - QReadLocker locker(&_entityToElementLock); - QHashIterator i(_entityToElementMap); + QHashIterator i(localMap); while (i.hasNext()) { i.next(); - qCDebug(entities) << i.key() << ": " << i.value().get(); + qCDebug(entities) << i.key() << ": " << i.value()->getElement().get(); } qCDebug(entities) << "-----------------------------------------------------"; } diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index 24e6c364b1..efb8cf81ba 100644 --- a/libraries/entities/src/EntityTree.h +++ b/libraries/entities/src/EntityTree.h @@ -119,9 +119,6 @@ public: // use this method if you only know the entityID bool updateEntity(const EntityItemID& entityID, const EntityItemProperties& properties, const SharedNodePointer& senderNode = SharedNodePointer(nullptr)); - // use this method if you have a pointer to the entity (avoid an extra entity lookup) - bool updateEntity(EntityItemPointer entity, const EntityItemProperties& properties, const SharedNodePointer& senderNode = SharedNodePointer(nullptr)); - // check if the avatar is a child of this entity, If so set the avatar parentID to null void unhookChildAvatar(const EntityItemID entityID); void deleteEntity(const EntityItemID& entityID, bool force = false, bool ignoreWarnings = true); @@ -183,7 +180,8 @@ public: int processEraseMessageDetails(const QByteArray& buffer, const SharedNodePointer& sourceNode); EntityTreeElementPointer getContainingElement(const EntityItemID& entityItemID) /*const*/; - void setContainingElement(const EntityItemID& entityItemID, EntityTreeElementPointer element); + void addEntityMapEntry(EntityItemPointer entity); + void clearEntityMapEntry(const EntityItemID& id); void debugDumpMap(); virtual void dumpTree() override; virtual void pruneTree() override; @@ -275,9 +273,8 @@ signals: protected: void processRemovedEntities(const DeleteEntityOperator& theOperator); - bool updateEntityWithElement(EntityItemPointer entity, const EntityItemProperties& properties, - EntityTreeElementPointer containingElement, - const SharedNodePointer& senderNode = SharedNodePointer(nullptr)); + bool updateEntity(EntityItemPointer entity, const EntityItemProperties& properties, + const SharedNodePointer& senderNode = SharedNodePointer(nullptr)); static bool findNearPointOperation(const OctreeElementPointer& element, void* extraData); static bool findInSphereOperation(const OctreeElementPointer& element, void* extraData); static bool findInCubeOperation(const OctreeElementPointer& element, void* extraData); @@ -309,8 +306,8 @@ protected: _deletedEntityItemIDs << id; } - mutable QReadWriteLock _entityToElementLock; - QHash _entityToElementMap; + mutable QReadWriteLock _entityMapLock; + QHash _entityMap; EntitySimulationPointer _simulation; diff --git a/libraries/entities/src/EntityTreeElement.cpp b/libraries/entities/src/EntityTreeElement.cpp index cce7ee006f..108cb39222 100644 --- a/libraries/entities/src/EntityTreeElement.cpp +++ b/libraries/entities/src/EntityTreeElement.cpp @@ -885,10 +885,10 @@ EntityItemPointer EntityTreeElement::getEntityWithEntityItemID(const EntityItemI void EntityTreeElement::cleanupEntities() { withWriteLock([&] { foreach(EntityItemPointer entity, _entityItems) { + // NOTE: only EntityTreeElement should ever be changing the value of entity->_element // NOTE: We explicitly don't delete the EntityItem here because since we only // access it by smart pointers, when we remove it from the _entityItems // we know that it will be deleted. - //delete entity; entity->_element = NULL; } _entityItems.clear(); @@ -903,6 +903,7 @@ bool EntityTreeElement::removeEntityWithEntityItemID(const EntityItemID& id) { EntityItemPointer& entity = _entityItems[i]; if (entity->getEntityItemID() == id) { foundEntity = true; + // NOTE: only EntityTreeElement should ever be changing the value of entity->_element entity->_element = NULL; _entityItems.removeAt(i); break; @@ -918,6 +919,7 @@ bool EntityTreeElement::removeEntityItem(EntityItemPointer entity) { numEntries = _entityItems.removeAll(entity); }); if (numEntries > 0) { + // NOTE: only EntityTreeElement should ever be changing the value of entity->_element assert(entity->_element.get() == this); entity->_element = NULL; return true; @@ -1001,7 +1003,6 @@ int EntityTreeElement::readElementDataFromBuffer(const unsigned char* data, int if (currentContainingElement.get() != this) { currentContainingElement->removeEntityItem(entityItem); addEntityItem(entityItem); - _myTree->setContainingElement(entityItemID, getThisPointer()); } } } @@ -1032,9 +1033,9 @@ int EntityTreeElement::readElementDataFromBuffer(const unsigned char* data, int // don't add if we've recently deleted.... if (!_myTree->isDeletedEntity(entityItem->getID())) { + _myTree->addEntityMapEntry(entityItem); addEntityItem(entityItem); // add this new entity to this elements entities entityItemID = entityItem->getEntityItemID(); - _myTree->setContainingElement(entityItemID, getThisPointer()); _myTree->postAddEntity(entityItem); if (entityItem->getCreated() == UNKNOWN_CREATED_TIME) { entityItem->recordCreationTime(); diff --git a/libraries/entities/src/MovingEntitiesOperator.cpp b/libraries/entities/src/MovingEntitiesOperator.cpp index ab97c67aa2..42e5a2ece5 100644 --- a/libraries/entities/src/MovingEntitiesOperator.cpp +++ b/libraries/entities/src/MovingEntitiesOperator.cpp @@ -51,7 +51,7 @@ MovingEntitiesOperator::~MovingEntitiesOperator() { void MovingEntitiesOperator::addEntityToMoveList(EntityItemPointer entity, const AACube& newCube) { - EntityTreeElementPointer oldContainingElement = _tree->getContainingElement(entity->getEntityItemID()); + EntityTreeElementPointer oldContainingElement = entity->getElement(); AABox newCubeClamped = newCube.clamp((float)-HALF_TREE_SCALE, (float)HALF_TREE_SCALE); if (_wantDebug) { @@ -193,7 +193,6 @@ bool MovingEntitiesOperator::preRecursion(const OctreeElementPointer& 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(); // remove from the old before adding EntityTreeElementPointer oldElement = details.entity->getElement(); if (oldElement != entityTreeElement) { @@ -201,7 +200,6 @@ bool MovingEntitiesOperator::preRecursion(const OctreeElementPointer& element) { oldElement->removeEntityItem(details.entity); } entityTreeElement->addEntityItem(details.entity); - _tree->setContainingElement(entityItemID, entityTreeElement); } _foundNewCount++; //details.newFound = true; // TODO: would be nice to add this optimization diff --git a/libraries/entities/src/UpdateEntityOperator.cpp b/libraries/entities/src/UpdateEntityOperator.cpp index ec6051af04..7a5c87187a 100644 --- a/libraries/entities/src/UpdateEntityOperator.cpp +++ b/libraries/entities/src/UpdateEntityOperator.cpp @@ -138,8 +138,8 @@ bool UpdateEntityOperator::preRecursion(const OctreeElementPointer& element) { qCDebug(entities) << " _foundNew=" << _foundNew; } - // If we haven't yet found the old entity, and this subTreeContains our old - // entity, then we need to keep searching. + // If we haven't yet found the old element, and this subTreeContains our old element, + // then we need to keep searching. if (!_foundOld && subtreeContainsOld) { if (_wantDebug) { @@ -169,7 +169,6 @@ bool UpdateEntityOperator::preRecursion(const OctreeElementPointer& element) { // NOTE: we know we haven't yet added it to its new element because _removeOld is true EntityTreeElementPointer oldElement = _existingEntity->getElement(); oldElement->removeEntityItem(_existingEntity); - _tree->setContainingElement(_entityItemID, NULL); if (oldElement != _containingElement) { qCDebug(entities) << "WARNING entity moved during UpdateEntityOperator recursion"; @@ -187,8 +186,8 @@ bool UpdateEntityOperator::preRecursion(const OctreeElementPointer& element) { } } - // If we haven't yet found the new entity, and this subTreeContains our new - // entity, then we need to keep searching. + // If we haven't yet found the new element, and this subTreeContains our new element, + // then we need to keep searching. if (!_foundNew && subtreeContainsNew) { if (_wantDebug) { @@ -221,7 +220,6 @@ bool UpdateEntityOperator::preRecursion(const OctreeElementPointer& element) { } } entityTreeElement->addEntityItem(_existingEntity); - _tree->setContainingElement(_entityItemID, entityTreeElement); } _foundNew = true; // we found the new element _removeOld = false; // and it has already been removed from the old diff --git a/scripts/developer/tests/entityLookupCostMeasurement.js b/scripts/developer/tests/entityLookupCostMeasurement.js new file mode 100644 index 0000000000..15697fe13d --- /dev/null +++ b/scripts/developer/tests/entityLookupCostMeasurement.js @@ -0,0 +1,104 @@ +// Creates a large number of entities on the cardinal planes of the octree (all +// objects will live in the root octree element). Measures how long it takes +// to update the properties of the first and last entity. The difference +// between the two measurements shows how the cost of lookup changes as a +// function of the number of entities. For best results run this in an +// otherwise empty domain. + +var firstId; +var lastId; +var NUM_ENTITIES_ON_SIDE = 25; + +// create the objects +createObjects = function () { + var STRIDE = 0.75; + var WIDTH = 0.5; + var DIMENSIONS = { x: WIDTH, y: WIDTH, z: WIDTH }; + var LIFETIME = 20; + + var properties = { + name: "", + type : "Box", + dimensions : DIMENSIONS, + position : { x: 0, y: 0, z: 0}, + lifetime : LIFETIME, + color : { red:255, green: 64, blue: 255 } + }; + + // xy + var planeName = "xy"; + for (var i = 0; i < NUM_ENTITIES_ON_SIDE; ++i) { + for (var j = 0; j < NUM_ENTITIES_ON_SIDE; ++j) { + properties.name = "Box-" + planeName + "-" + i + "." + j; + properties.position = { x: i * STRIDE, y: j * STRIDE, z: 0 }; + var red = i * 255 / NUM_ENTITIES_ON_SIDE; + var green = j * 255 / NUM_ENTITIES_ON_SIDE; + var blue = 0; + properties.color = { red: red, green: green, blue: blue }; + if (i == 0 && j == 0) { + firstId = Entities.addEntity(properties); + } else { + Entities.addEntity(properties); + } + } + } + + // yz + var planeName = "yz"; + for (var i = 0; i < NUM_ENTITIES_ON_SIDE; ++i) { + for (var j = 0; j < NUM_ENTITIES_ON_SIDE; ++j) { + properties.name = "Box-" + planeName + "-" + i + "." + j; + properties.position = { x: 0, y: i * STRIDE, z: j * STRIDE }; + var red = 0; + var green = i * 255 / NUM_ENTITIES_ON_SIDE; + var blue = j * 255 / NUM_ENTITIES_ON_SIDE; + properties.color = { red: red, green: green, blue: blue }; + Entities.addEntity(properties); + } + } + + // zx + var planeName = "zx"; + for (var i = 0; i < NUM_ENTITIES_ON_SIDE; ++i) { + for (var j = 0; j < NUM_ENTITIES_ON_SIDE; ++j) { + properties.name = "Box-" + planeName + "-" + i + "." + j; + properties.position = { x: j * STRIDE, y: 0, z: i * STRIDE }; + var red = j * 255 / NUM_ENTITIES_ON_SIDE; + var green = 0; + var blue = i * 255 / NUM_ENTITIES_ON_SIDE; + properties.color = { red: red, green: green, blue: blue }; + lastId = Entities.addEntity(properties); + } + } +}; + +createObjects(); + +// measure the time it takes to edit the first and last entities many times +// (requires a lookup by entityId each time) +changeProperties = function (id) { + var newProperties = { color : { red: 255, green: 255, blue: 255 } }; + Entities.editEntity(id, newProperties); +} + +// first +var NUM_CHANGES = 10000; +var firstStart = Date.now(); +for (var k = 0; k < NUM_CHANGES; ++k) { + changeProperties(firstId); +} +var firstEnd = Date.now(); +var firstDt = firstEnd - firstStart; + +// last +var lastStart = Date.now(); +for (var k = 0; k < NUM_CHANGES; ++k) { + changeProperties(lastId); +} +var lastEnd = Date.now(); +var lastDt = lastEnd - lastStart; + +// print the results +var numEntities = 3 * NUM_ENTITIES_ON_SIDE * NUM_ENTITIES_ON_SIDE; +print("numEntities = " + numEntities + " numEdits = " + NUM_CHANGES + " firstDt = " + firstDt + " lastDt = " + lastDt); +