From 7d9efe4c091521e8c3a0362a8cd673410a072263 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 28 Feb 2019 14:20:11 -0800 Subject: [PATCH 1/4] avoid mem-leak for serverless domains --- interface/src/Application.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 83b287b7ae..9913e0d128 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -3773,9 +3773,12 @@ std::map Application::prepareServerlessDomainContents(QUrl dom tmpTree->reaverageOctreeElements(); tmpTree->sendEntities(&_entityEditSender, getEntities()->getTree(), 0, 0, 0); } + std::map namedPaths = tmpTree->getNamedPaths(); - return tmpTree->getNamedPaths(); + // we must manually eraseAllOctreeElements(false) else the tmpTree will mem-leak + tmpTree->eraseAllOctreeElements(false); + return namedPaths; } void Application::loadServerlessDomain(QUrl domainURL) { From ed9ebf84c971ed040612d6cc7075ae6d9cd521bd Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 28 Feb 2019 14:21:14 -0800 Subject: [PATCH 2/4] remove unuseful log entry --- libraries/entities-renderer/src/EntityTreeRenderer.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index efd1399f30..e54258fc3e 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -232,8 +232,6 @@ void EntityTreeRenderer::clearNonLocalEntities() { } } scene->enqueueTransaction(transaction); - } else { - qCWarning(entitiesrenderer) << "EntitityTreeRenderer::clear(), Unexpected null scene, possibly during application shutdown"; } _renderablesToUpdate = savedEntities; From 33d6e2ce3b2e562bdac33368d7a7750113e3bd3b Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 28 Feb 2019 14:21:35 -0800 Subject: [PATCH 3/4] avoid mem-leaked entities when clearing tree --- libraries/entities/src/EntityTree.cpp | 40 +++++++++++++++++++-------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 53b4fb4fe4..8855bc28a7 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -52,7 +52,15 @@ EntityTree::EntityTree(bool shouldReaverage) : } EntityTree::~EntityTree() { - eraseAllOctreeElements(false); + // NOTE: to eraseAllOctreeElements() in this context is useless because + // any OctreeElements in the tree still have shared backpointers to this Tree + // which means the dtor never would have been called in the first place! + // + // I'm keeping this useless commented-out line to remind us: + // we don't need shared pointer overhead for EntityTrees. + // TODO: EntityTreeElement::_tree should be raw back pointer. + // AND: EntityItem::_element should be a raw back pointer. + //eraseAllOctreeElements(false); // KEEP THIS } void EntityTree::setEntityScriptSourceWhitelist(const QString& entityScriptSourceWhitelist) { @@ -74,16 +82,15 @@ void EntityTree::eraseNonLocalEntities() { emit clearingEntities(); if (_simulation) { - // This will clear all entities host types including local entities, because local entities - // are not in the physics simulation + // local entities are not in the simulation, so we clear ALL _simulation->clearEntities(); } - _staleProxies.clear(); - QHash localMap; - localMap.swap(_entityMap); - QHash savedEntities; this->withWriteLock([&] { - foreach(EntityItemPointer entity, localMap) { + QHash savedEntities; + // NOTE: lock the Tree first, then lock the _entityMap. + // It should never be done the other way around. + QReadLocker locker(&_entityMapLock); + foreach(EntityItemPointer entity, _entityMap) { EntityTreeElementPointer element = entity->getElement(); if (element) { element->cleanupNonLocalEntities(); @@ -91,11 +98,16 @@ void EntityTree::eraseNonLocalEntities() { if (entity->isLocalEntity()) { savedEntities[entity->getEntityItemID()] = entity; + } else { + int32_t spaceIndex = entity->getSpaceIndex(); + if (spaceIndex != -1) { + // stale spaceIndices will be freed later + _staleProxies.push_back(spaceIndex); + } } } + _entityMap.swap(savedEntities); }); - localMap.clear(); - _entityMap = savedEntities; resetClientEditStats(); clearDeletedEntities(); @@ -113,13 +125,13 @@ void EntityTree::eraseNonLocalEntities() { _needsParentFixup = localEntitiesNeedsParentFixup; } } + void EntityTree::eraseAllOctreeElements(bool createNewRoot) { emit clearingEntities(); if (_simulation) { _simulation->clearEntities(); } - _staleProxies.clear(); QHash localMap; localMap.swap(_entityMap); this->withWriteLock([&] { @@ -128,6 +140,11 @@ void EntityTree::eraseAllOctreeElements(bool createNewRoot) { if (element) { element->cleanupEntities(); } + int32_t spaceIndex = entity->getSpaceIndex(); + if (spaceIndex != -1) { + // assume stale spaceIndices will be freed later + _staleProxies.push_back(spaceIndex); + } } }); localMap.clear(); @@ -767,6 +784,7 @@ void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator) // keep a record of valid stale spaceIndices so they can be removed from the Space int32_t spaceIndex = theEntity->getSpaceIndex(); if (spaceIndex != -1) { + // stale spaceIndices will be freed later _staleProxies.push_back(spaceIndex); } } From 8a7853a701a2b4790f77b05d49e94b7e7fbd48d6 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 28 Feb 2019 14:38:13 -0800 Subject: [PATCH 4/4] remove crufty comment --- libraries/entities/src/EntityTree.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 8855bc28a7..6e404ce690 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -781,7 +781,6 @@ void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator) _simulation->prepareEntityForDelete(theEntity); } - // keep a record of valid stale spaceIndices so they can be removed from the Space int32_t spaceIndex = theEntity->getSpaceIndex(); if (spaceIndex != -1) { // stale spaceIndices will be freed later