From 33bdd841c17edf19c626cc1989cd0f8a887ee075 Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Mon, 30 Oct 2023 18:40:04 +0100 Subject: [PATCH 1/3] Fix entity server memory leak and improve performance --- libraries/entities/src/EntityTree.cpp | 10 ++-------- libraries/entities/src/EntityTree.h | 3 +++ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 5d23cf704b..f947f6707a 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -2553,9 +2553,7 @@ bool EntityTree::writeToMap(QVariantMap& entityDescription, OctreeElementPointer } entityDescription["DataVersion"] = _persistDataVersion; entityDescription["Id"] = _persistID; - // V8TODO: Creating new script engine each time is very inefficient - ScriptEnginePointer engine = newScriptEngine(); - RecurseOctreeToMapOperator theOperator(entityDescription, element, engine.get(), skipDefaultValues, + RecurseOctreeToMapOperator theOperator(entityDescription, element, scriptEngine.get(), skipDefaultValues, skipThoseWithBadParents, _myAvatar); withReadLock([&] { recurseTreeWithOperator(&theOperator); @@ -2704,8 +2702,6 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) { // to a ScriptValue, and then to EntityItemProperties. These properties are used // to add the new entity to the EntityTree. QVariantList entitiesQList = map["Entities"].toList(); - // V8TODO: Creating new script engine each time is very inefficient - ScriptEnginePointer scriptEngine = newScriptEngine(); if (entitiesQList.length() == 0) { qCDebug(entities) << "EntityTree::readFromMap: entitiesQList.length() == 0, Empty map or invalidly formed file"; @@ -2881,9 +2877,7 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) { } bool EntityTree::writeToJSON(QString& jsonString, const OctreeElementPointer& element) { - // V8TODO: Creating new script engine each time is very inefficient - ScriptEnginePointer engine = newScriptEngine(); - RecurseOctreeToJSONOperator theOperator(element, engine.get(), jsonString); + RecurseOctreeToJSONOperator theOperator(element, scriptEngine.get(), jsonString); withReadLock([&] { recurseTreeWithOperator(&theOperator); }); diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index 78fab3af6f..5b7e5691c3 100644 --- a/libraries/entities/src/EntityTree.h +++ b/libraries/entities/src/EntityTree.h @@ -385,6 +385,9 @@ private: // Return an AACube containing object and all its entity descendants AACube updateEntityQueryAACubeWorker(SpatiallyNestablePointer object, EntityEditPacketSender* packetSender, MovingEntitiesOperator& moveOperator, bool force, bool tellServer); + + // Script engine for writing entity tree data to and from JSON + ScriptEnginePointer scriptEngine{ newScriptEngine() }; }; void convertGrabUserDataToProperties(EntityItemProperties& properties); From 7481130e612345a554df446b5df3101d2b139d4e Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Mon, 30 Oct 2023 19:15:42 +0100 Subject: [PATCH 2/3] Add a mutex to entity server script engine --- libraries/entities/src/EntityTree.cpp | 6 ++++++ libraries/entities/src/EntityTree.h | 1 + 2 files changed, 7 insertions(+) diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index f947f6707a..be8f577090 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -2553,11 +2553,13 @@ bool EntityTree::writeToMap(QVariantMap& entityDescription, OctreeElementPointer } entityDescription["DataVersion"] = _persistDataVersion; entityDescription["Id"] = _persistID; + scriptEngineMutex.lock(); RecurseOctreeToMapOperator theOperator(entityDescription, element, scriptEngine.get(), skipDefaultValues, skipThoseWithBadParents, _myAvatar); withReadLock([&] { recurseTreeWithOperator(&theOperator); }); + scriptEngineMutex.unlock(); return true; } @@ -2726,9 +2728,11 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) { " mapped it to parentJointIndex " << entityMap["parentJointIndex"].toInt(); } + scriptEngineMutex.lock(); ScriptValue entityScriptValue = variantMapToScriptValue(entityMap, *scriptEngine); EntityItemProperties properties; EntityItemPropertiesFromScriptValueIgnoreReadOnly(entityScriptValue, properties); + scriptEngineMutex.unlock(); EntityItemID entityItemID; if (entityMap.contains("id")) { @@ -2877,10 +2881,12 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) { } bool EntityTree::writeToJSON(QString& jsonString, const OctreeElementPointer& element) { + scriptEngineMutex.lock(); RecurseOctreeToJSONOperator theOperator(element, scriptEngine.get(), jsonString); withReadLock([&] { recurseTreeWithOperator(&theOperator); }); + scriptEngineMutex.unlock(); jsonString = theOperator.getJson(); return true; diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index 5b7e5691c3..d5c3a74eb7 100644 --- a/libraries/entities/src/EntityTree.h +++ b/libraries/entities/src/EntityTree.h @@ -387,6 +387,7 @@ private: MovingEntitiesOperator& moveOperator, bool force, bool tellServer); // Script engine for writing entity tree data to and from JSON + std::mutex scriptEngineMutex; ScriptEnginePointer scriptEngine{ newScriptEngine() }; }; From 6645763117fbe4fa8bdc4a17e60061e591788eb3 Mon Sep 17 00:00:00 2001 From: ksuprynowicz Date: Tue, 31 Oct 2023 18:37:05 +0100 Subject: [PATCH 3/3] Replaced mutex locking with lock guard --- libraries/entities/src/EntityTree.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index be8f577090..fb7fbe6499 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -2553,13 +2553,12 @@ bool EntityTree::writeToMap(QVariantMap& entityDescription, OctreeElementPointer } entityDescription["DataVersion"] = _persistDataVersion; entityDescription["Id"] = _persistID; - scriptEngineMutex.lock(); + const std::lock_guard scriptLock(scriptEngineMutex); RecurseOctreeToMapOperator theOperator(entityDescription, element, scriptEngine.get(), skipDefaultValues, skipThoseWithBadParents, _myAvatar); withReadLock([&] { recurseTreeWithOperator(&theOperator); }); - scriptEngineMutex.unlock(); return true; } @@ -2728,11 +2727,12 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) { " mapped it to parentJointIndex " << entityMap["parentJointIndex"].toInt(); } - scriptEngineMutex.lock(); - ScriptValue entityScriptValue = variantMapToScriptValue(entityMap, *scriptEngine); EntityItemProperties properties; - EntityItemPropertiesFromScriptValueIgnoreReadOnly(entityScriptValue, properties); - scriptEngineMutex.unlock(); + { + const std::lock_guard scriptLock(scriptEngineMutex); + ScriptValue entityScriptValue = variantMapToScriptValue(entityMap, *scriptEngine); + EntityItemPropertiesFromScriptValueIgnoreReadOnly(entityScriptValue, properties); + } EntityItemID entityItemID; if (entityMap.contains("id")) { @@ -2881,12 +2881,11 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) { } bool EntityTree::writeToJSON(QString& jsonString, const OctreeElementPointer& element) { - scriptEngineMutex.lock(); + const std::lock_guard scriptLock(scriptEngineMutex); RecurseOctreeToJSONOperator theOperator(element, scriptEngine.get(), jsonString); withReadLock([&] { recurseTreeWithOperator(&theOperator); }); - scriptEngineMutex.unlock(); jsonString = theOperator.getJson(); return true;