From 93a243d095561eebead01e668a0252a858174dcb Mon Sep 17 00:00:00 2001
From: ksuprynowicz <ksuprynowicz@post.pl>
Date: Mon, 4 Mar 2024 22:33:35 +0100
Subject: [PATCH] Move helper script engines to their own threads
---
.../src/avatars/ScriptableAvatar.cpp | 41 +++++++++++++++----
.../src/avatars/ScriptableAvatar.h | 7 +++-
interface/src/avatar/MyAvatar.cpp | 20 ++++++++-
interface/src/avatar/MyAvatar.h | 4 ++
libraries/baking/src/MaterialBaker.cpp | 21 +++++++++-
libraries/baking/src/MaterialBaker.h | 5 ++-
libraries/entities/src/EntityTree.cpp | 26 +++++++++---
libraries/entities/src/EntityTree.h | 5 ++-
8 files changed, 108 insertions(+), 21 deletions(-)
diff --git a/assignment-client/src/avatars/ScriptableAvatar.cpp b/assignment-client/src/avatars/ScriptableAvatar.cpp
index c919d2a2d7..1d0114d703 100644
--- a/assignment-client/src/avatars/ScriptableAvatar.cpp
+++ b/assignment-client/src/avatars/ScriptableAvatar.cpp
@@ -30,8 +30,26 @@
#include <NetworkingConstants.h>
-ScriptableAvatar::ScriptableAvatar(): _scriptEngine(newScriptEngine()) {
+ScriptableAvatar::ScriptableAvatar() {
_clientTraitsHandler.reset(new ClientTraitsHandler(this));
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ _scriptEngine = newScriptEngine();
+ if (!_scriptEngineThread) {
+ _scriptEngineThread.reset(new QThread());
+ _scriptEngine->setThread(_scriptEngineThread.get());
+ _scriptEngineThread->start();
+ }
+}
+
+ScriptableAvatar::~ScriptableAvatar() {
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ if (_scriptEngine) {
+ if (_scriptEngineThread) {
+ _scriptEngineThread->quit();
+ _scriptEngineThread->wait();
+ }
+ _scriptEngine.reset();
+ }
}
QByteArray ScriptableAvatar::toByteArrayStateful(AvatarDataDetail dataDetail, bool dropFaceTracking) {
@@ -315,7 +333,10 @@ AvatarEntityMap ScriptableAvatar::getAvatarEntityDataInternal(bool allProperties
EntityItemProperties properties = entity->getProperties(desiredProperties);
QByteArray blob;
- EntityItemProperties::propertiesToBlob(*_scriptEngine, sessionID, properties, blob, allProperties);
+ {
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ EntityItemProperties::propertiesToBlob(*_scriptEngine, sessionID, properties, blob, allProperties);
+ }
data[id] = blob;
}
});
@@ -339,8 +360,11 @@ void ScriptableAvatar::setAvatarEntityData(const AvatarEntityMap& avatarEntityDa
while (dataItr != avatarEntityData.end()) {
EntityItemProperties properties;
const QByteArray& blob = dataItr.value();
- if (!blob.isNull() && EntityItemProperties::blobToProperties(*_scriptEngine, blob, properties)) {
- newProperties[dataItr.key()] = properties;
+ {
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ if (!blob.isNull() && EntityItemProperties::blobToProperties(*_scriptEngine, blob, properties)) {
+ newProperties[dataItr.key()] = properties;
+ }
}
++dataItr;
}
@@ -419,9 +443,12 @@ void ScriptableAvatar::updateAvatarEntity(const QUuid& entityID, const QByteArra
EntityItemPointer entity;
EntityItemProperties properties;
- if (!EntityItemProperties::blobToProperties(*_scriptEngine, entityData, properties)) {
- // entityData is corrupt
- return;
+ {
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ if (!EntityItemProperties::blobToProperties(*_scriptEngine, entityData, properties)) {
+ // entityData is corrupt
+ return;
+ }
}
std::map<QUuid, EntityItemPointer>::iterator itr = _entities.find(entityID);
diff --git a/assignment-client/src/avatars/ScriptableAvatar.h b/assignment-client/src/avatars/ScriptableAvatar.h
index 703a0a9f64..56f9d2169d 100644
--- a/assignment-client/src/avatars/ScriptableAvatar.h
+++ b/assignment-client/src/avatars/ScriptableAvatar.h
@@ -111,6 +111,7 @@ class ScriptableAvatar : public AvatarData, public Dependency {
public:
ScriptableAvatar();
+ ~ScriptableAvatar();
/*@jsdoc
* Starts playing an animation on the avatar.
@@ -222,7 +223,11 @@ private:
QHash<QString, int> _fstJointIndices; ///< 1-based, since zero is returned for missing keys
QStringList _fstJointNames; ///< in order of depth-first traversal
QUrl _skeletonFBXURL;
- mutable ScriptEnginePointer _scriptEngine;
+
+ mutable std::mutex _scriptEngineLock;
+ mutable ScriptEnginePointer _scriptEngine { nullptr };
+ std::shared_ptr<QThread> _scriptEngineThread { nullptr };
+
std::map<QUuid, EntityItemPointer> _entities;
/// Loads the joint indices, names from the FST file (if any)
diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp
index 62100f9cae..de2ba27a77 100644
--- a/interface/src/avatar/MyAvatar.cpp
+++ b/interface/src/avatar/MyAvatar.cpp
@@ -406,6 +406,14 @@ MyAvatar::~MyAvatar() {
if (_addAvatarEntitiesToTreeTimer.isActive()) {
_addAvatarEntitiesToTreeTimer.stop();
}
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ if (_scriptEngine) {
+ if (_scriptEngineThread) {
+ _scriptEngineThread->quit();
+ _scriptEngineThread->wait();
+ }
+ _scriptEngine.reset();
+ }
}
QString MyAvatar::getDominantHand() const {
@@ -2092,8 +2100,16 @@ void MyAvatar::avatarEntityDataToJson(QJsonObject& root) const {
}
void MyAvatar::loadData() {
- if (!_scriptEngine) {
- _scriptEngine = newScriptEngine();
+ {
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ if (!_scriptEngine) {
+ _scriptEngine = newScriptEngine();
+ if (!_scriptEngineThread) {
+ _scriptEngineThread.reset(new QThread());
+ _scriptEngine->setThread(_scriptEngineThread.get());
+ _scriptEngineThread->start();
+ }
+ }
}
getHead()->setBasePitch(_headPitchSetting.get());
diff --git a/interface/src/avatar/MyAvatar.h b/interface/src/avatar/MyAvatar.h
index 5e0627360c..f3a04284c3 100644
--- a/interface/src/avatar/MyAvatar.h
+++ b/interface/src/avatar/MyAvatar.h
@@ -3101,8 +3101,12 @@ private:
mutable std::set<EntityItemID> _staleCachedAvatarEntityBlobs;
//
// keep a ScriptEngine around so we don't have to instantiate on the fly (these are very slow to create/delete)
+ // TODO: profile if it performs better when script engine is on avatar thread or on its own thread
+ // Own thread is safer from deadlocks
mutable std::mutex _scriptEngineLock;
ScriptEnginePointer _scriptEngine { nullptr };
+ std::shared_ptr<QThread> _scriptEngineThread { nullptr };
+
bool _needToSaveAvatarEntitySettings { false };
bool _reactionTriggers[NUM_AVATAR_TRIGGER_REACTIONS] { false, false };
diff --git a/libraries/baking/src/MaterialBaker.cpp b/libraries/baking/src/MaterialBaker.cpp
index 48718cd6c3..d28fc9aacf 100644
--- a/libraries/baking/src/MaterialBaker.cpp
+++ b/libraries/baking/src/MaterialBaker.cpp
@@ -36,9 +36,24 @@ MaterialBaker::MaterialBaker(const QString& materialData, bool isURL, const QStr
_isURL(isURL),
_destinationPath(destinationPath),
_bakedOutputDir(bakedOutputDir),
- _textureOutputDir(bakedOutputDir + "/materialTextures/" + QString::number(materialNum++)),
- _scriptEngine(newScriptEngine())
+ _textureOutputDir(bakedOutputDir + "/materialTextures/" + QString::number(materialNum++))
{
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ _scriptEngine = newScriptEngine();
+ _scriptEngineThread.reset(new QThread());
+ _scriptEngine->setThread(_scriptEngineThread.get());
+ _scriptEngineThread->start();
+}
+
+MaterialBaker::~MaterialBaker() {
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ if (_scriptEngine) {
+ if (_scriptEngineThread) {
+ _scriptEngineThread->quit();
+ _scriptEngineThread->wait();
+ }
+ _scriptEngine.reset();
+ }
}
void MaterialBaker::bake() {
@@ -214,6 +229,7 @@ void MaterialBaker::outputMaterial() {
if (_materialResource->parsedMaterials.networkMaterials.size() == 1) {
auto networkMaterial = _materialResource->parsedMaterials.networkMaterials.begin();
auto scriptableMaterial = scriptable::ScriptableMaterial(networkMaterial->second);
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
QVariant materialVariant =
scriptable::scriptableMaterialToScriptValue(_scriptEngine.get(), scriptableMaterial).toVariant();
json.insert("materials", QJsonDocument::fromVariant(materialVariant).object());
@@ -221,6 +237,7 @@ void MaterialBaker::outputMaterial() {
QJsonArray materialArray;
for (auto networkMaterial : _materialResource->parsedMaterials.networkMaterials) {
auto scriptableMaterial = scriptable::ScriptableMaterial(networkMaterial.second);
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
QVariant materialVariant =
scriptable::scriptableMaterialToScriptValue(_scriptEngine.get(), scriptableMaterial).toVariant();
materialArray.append(QJsonDocument::fromVariant(materialVariant).object());
diff --git a/libraries/baking/src/MaterialBaker.h b/libraries/baking/src/MaterialBaker.h
index 129b36aa8f..986dead3ed 100644
--- a/libraries/baking/src/MaterialBaker.h
+++ b/libraries/baking/src/MaterialBaker.h
@@ -32,6 +32,7 @@ class MaterialBaker : public Baker {
Q_OBJECT
public:
MaterialBaker(const QString& materialData, bool isURL, const QString& bakedOutputDir, QUrl destinationPath = QUrl());
+ virtual ~MaterialBaker();
QString getMaterialData() const { return _materialData; }
bool isURL() const { return _isURL; }
@@ -72,7 +73,9 @@ private:
QString _textureOutputDir;
QString _bakedMaterialData;
- ScriptEnginePointer _scriptEngine;
+ mutable std::mutex _scriptEngineLock;
+ ScriptEnginePointer _scriptEngine { nullptr };
+ std::shared_ptr<QThread> _scriptEngineThread { nullptr };
static std::function<QThread*()> _getNextOvenWorkerThreadOperator;
TextureFileNamer _textureFileNamer;
diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp
index fb7fbe6499..61535b913e 100644
--- a/libraries/entities/src/EntityTree.cpp
+++ b/libraries/entities/src/EntityTree.cpp
@@ -50,6 +50,12 @@ static const QString DOMAIN_UNLIMITED = "domainUnlimited";
EntityTree::EntityTree(bool shouldReaverage) :
Octree(shouldReaverage)
{
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ _scriptEngine = newScriptEngine();
+ _scriptEngineThread.reset(new QThread());
+ _scriptEngine->setThread(_scriptEngineThread.get());
+ _scriptEngineThread->start();
+
resetClientEditStats();
}
@@ -63,6 +69,14 @@ EntityTree::~EntityTree() {
// TODO: EntityTreeElement::_tree should be raw back pointer.
// AND: EntityItem::_element should be a raw back pointer.
//eraseAllOctreeElements(false); // KEEP THIS
+ std::lock_guard<std::mutex> lock(_scriptEngineLock);
+ if (_scriptEngine) {
+ if (_scriptEngineThread) {
+ _scriptEngineThread->quit();
+ _scriptEngineThread->wait();
+ }
+ _scriptEngine.reset();
+ }
}
void EntityTree::setEntityScriptSourceWhitelist(const QString& entityScriptSourceWhitelist) {
@@ -2553,8 +2567,8 @@ bool EntityTree::writeToMap(QVariantMap& entityDescription, OctreeElementPointer
}
entityDescription["DataVersion"] = _persistDataVersion;
entityDescription["Id"] = _persistID;
- const std::lock_guard<std::mutex> scriptLock(scriptEngineMutex);
- RecurseOctreeToMapOperator theOperator(entityDescription, element, scriptEngine.get(), skipDefaultValues,
+ const std::lock_guard<std::mutex> scriptLock(_scriptEngineLock);
+ RecurseOctreeToMapOperator theOperator(entityDescription, element, _scriptEngine.get(), skipDefaultValues,
skipThoseWithBadParents, _myAvatar);
withReadLock([&] {
recurseTreeWithOperator(&theOperator);
@@ -2729,8 +2743,8 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) {
EntityItemProperties properties;
{
- const std::lock_guard<std::mutex> scriptLock(scriptEngineMutex);
- ScriptValue entityScriptValue = variantMapToScriptValue(entityMap, *scriptEngine);
+ const std::lock_guard<std::mutex> scriptLock(_scriptEngineLock);
+ ScriptValue entityScriptValue = variantMapToScriptValue(entityMap, *_scriptEngine);
EntityItemPropertiesFromScriptValueIgnoreReadOnly(entityScriptValue, properties);
}
@@ -2881,8 +2895,8 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) {
}
bool EntityTree::writeToJSON(QString& jsonString, const OctreeElementPointer& element) {
- const std::lock_guard<std::mutex> scriptLock(scriptEngineMutex);
- RecurseOctreeToJSONOperator theOperator(element, scriptEngine.get(), jsonString);
+ const std::lock_guard<std::mutex> scriptLock(_scriptEngineLock);
+ RecurseOctreeToJSONOperator theOperator(element, _scriptEngine.get(), jsonString);
withReadLock([&] {
recurseTreeWithOperator(&theOperator);
});
diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h
index d5c3a74eb7..9d348e87a7 100644
--- a/libraries/entities/src/EntityTree.h
+++ b/libraries/entities/src/EntityTree.h
@@ -387,8 +387,9 @@ private:
MovingEntitiesOperator& moveOperator, bool force, bool tellServer);
// Script engine for writing entity tree data to and from JSON
- std::mutex scriptEngineMutex;
- ScriptEnginePointer scriptEngine{ newScriptEngine() };
+ std::mutex _scriptEngineLock;
+ ScriptEnginePointer _scriptEngine{ nullptr };
+ std::shared_ptr<QThread> _scriptEngineThread { nullptr };
};
void convertGrabUserDataToProperties(EntityItemProperties& properties);