From 0c916e0c03a9bead84bc74a0f949b98a12a05275 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Thu, 30 May 2019 16:08:47 -0700 Subject: [PATCH 1/3] guard MyAvatar::_scriptEngine with mutex --- interface/src/avatar/MyAvatar.cpp | 18 +++++++++++++----- interface/src/avatar/MyAvatar.h | 1 + 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 7ac2103543..b8042f1ce7 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -1598,6 +1598,7 @@ void MyAvatar::handleChangedAvatarEntityData() { blobFailed = true; // blob doesn't exist return; } + std::lock_guard guard(_myScriptEngineLock); if (!EntityItemProperties::blobToProperties(*_myScriptEngine, itr.value(), properties)) { blobFailed = true; // blob is corrupt } @@ -1630,6 +1631,7 @@ void MyAvatar::handleChangedAvatarEntityData() { skip = true; return; } + std::lock_guard guard(_myScriptEngineLock); if (!EntityItemProperties::blobToProperties(*_myScriptEngine, itr.value(), properties)) { skip = true; } @@ -1737,7 +1739,10 @@ bool MyAvatar::updateStaleAvatarEntityBlobs() const { if (found) { ++numFound; QByteArray blob; - EntityItemProperties::propertiesToBlob(*_myScriptEngine, getID(), properties, blob); + { + std::lock_guard guard(_myScriptEngineLock); + EntityItemProperties::propertiesToBlob(*_myScriptEngine, getID(), properties, blob); + } _avatarEntitiesLock.withWriteLock([&] { _cachedAvatarEntityBlobs[id] = blob; }); @@ -2476,15 +2481,18 @@ QVariantList MyAvatar::getAvatarEntitiesVariant() { if (!entity) { continue; } - QVariantMap avatarEntityData; EncodeBitstreamParams params; auto desiredProperties = entity->getEntityProperties(params); desiredProperties += PROP_LOCAL_POSITION; desiredProperties += PROP_LOCAL_ROTATION; - EntityItemProperties entityProperties = entity->getProperties(desiredProperties); - QScriptValue scriptProperties = EntityItemPropertiesToScriptValue(_myScriptEngine, entityProperties); + QVariantMap avatarEntityData; avatarEntityData["id"] = entityID; - avatarEntityData["properties"] = scriptProperties.toVariant(); + { + std::lock_guard guard(_myScriptEngineLock); + EntityItemProperties entityProperties = entity->getProperties(desiredProperties); + QScriptValue scriptProperties = EntityItemPropertiesToScriptValue(_myScriptEngine, entityProperties); + avatarEntityData["properties"] = scriptProperties.toVariant(); + } avatarEntitiesData.append(QVariant(avatarEntityData)); } } diff --git a/interface/src/avatar/MyAvatar.h b/interface/src/avatar/MyAvatar.h index 5b12885d1f..ee14ad1fda 100755 --- a/interface/src/avatar/MyAvatar.h +++ b/interface/src/avatar/MyAvatar.h @@ -2741,6 +2741,7 @@ private: mutable std::set _staleCachedAvatarEntityBlobs; // // keep a ScriptEngine around so we don't have to instantiate on the fly (these are very slow to create/delete) + mutable std::mutex _myScriptEngineLock; QScriptEngine* _myScriptEngine { nullptr }; bool _needToSaveAvatarEntitySettings { false }; }; From 5f9262cf5e7263b2412b3b7d96aae21cf8731228 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 31 May 2019 11:03:11 -0700 Subject: [PATCH 2/3] change data member names to not poke other devs in the eye --- interface/src/avatar/MyAvatar.cpp | 24 ++++++++++++------------ interface/src/avatar/MyAvatar.h | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index b8042f1ce7..613d7f81ef 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -325,8 +325,8 @@ MyAvatar::MyAvatar(QThread* thread) : MyAvatar::~MyAvatar() { _lookAtTargetAvatar.reset(); - delete _myScriptEngine; - _myScriptEngine = nullptr; + delete _scriptEngine; + _scriptEngine = nullptr; } QString MyAvatar::getDominantHand() const { @@ -1598,8 +1598,8 @@ void MyAvatar::handleChangedAvatarEntityData() { blobFailed = true; // blob doesn't exist return; } - std::lock_guard guard(_myScriptEngineLock); - if (!EntityItemProperties::blobToProperties(*_myScriptEngine, itr.value(), properties)) { + std::lock_guard guard(_scriptEngineLock); + if (!EntityItemProperties::blobToProperties(*_scriptEngine, itr.value(), properties)) { blobFailed = true; // blob is corrupt } }); @@ -1631,8 +1631,8 @@ void MyAvatar::handleChangedAvatarEntityData() { skip = true; return; } - std::lock_guard guard(_myScriptEngineLock); - if (!EntityItemProperties::blobToProperties(*_myScriptEngine, itr.value(), properties)) { + std::lock_guard guard(_scriptEngineLock); + if (!EntityItemProperties::blobToProperties(*_scriptEngine, itr.value(), properties)) { skip = true; } }); @@ -1740,8 +1740,8 @@ bool MyAvatar::updateStaleAvatarEntityBlobs() const { ++numFound; QByteArray blob; { - std::lock_guard guard(_myScriptEngineLock); - EntityItemProperties::propertiesToBlob(*_myScriptEngine, getID(), properties, blob); + std::lock_guard guard(_scriptEngineLock); + EntityItemProperties::propertiesToBlob(*_scriptEngine, getID(), properties, blob); } _avatarEntitiesLock.withWriteLock([&] { _cachedAvatarEntityBlobs[id] = blob; @@ -1888,8 +1888,8 @@ void MyAvatar::avatarEntityDataToJson(QJsonObject& root) const { } void MyAvatar::loadData() { - if (!_myScriptEngine) { - _myScriptEngine = new QScriptEngine(); + if (!_scriptEngine) { + _scriptEngine = new QScriptEngine(); } getHead()->setBasePitch(_headPitchSetting.get()); @@ -2488,9 +2488,9 @@ QVariantList MyAvatar::getAvatarEntitiesVariant() { QVariantMap avatarEntityData; avatarEntityData["id"] = entityID; { - std::lock_guard guard(_myScriptEngineLock); + std::lock_guard guard(_scriptEngineLock); EntityItemProperties entityProperties = entity->getProperties(desiredProperties); - QScriptValue scriptProperties = EntityItemPropertiesToScriptValue(_myScriptEngine, entityProperties); + QScriptValue scriptProperties = EntityItemPropertiesToScriptValue(_scriptEngine, entityProperties); avatarEntityData["properties"] = scriptProperties.toVariant(); } avatarEntitiesData.append(QVariant(avatarEntityData)); diff --git a/interface/src/avatar/MyAvatar.h b/interface/src/avatar/MyAvatar.h index ee14ad1fda..058603f320 100755 --- a/interface/src/avatar/MyAvatar.h +++ b/interface/src/avatar/MyAvatar.h @@ -2741,8 +2741,8 @@ private: mutable std::set _staleCachedAvatarEntityBlobs; // // keep a ScriptEngine around so we don't have to instantiate on the fly (these are very slow to create/delete) - mutable std::mutex _myScriptEngineLock; - QScriptEngine* _myScriptEngine { nullptr }; + mutable std::mutex _scriptEngineLock; + QScriptEngine* _scriptEngine { nullptr }; bool _needToSaveAvatarEntitySettings { false }; }; From 297517c85d256041834a68aa56f430ac02703f94 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Fri, 31 May 2019 11:06:16 -0700 Subject: [PATCH 3/3] minimize lock duration --- interface/src/avatar/MyAvatar.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 613d7f81ef..ad39b6663a 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -2487,12 +2487,13 @@ QVariantList MyAvatar::getAvatarEntitiesVariant() { desiredProperties += PROP_LOCAL_ROTATION; QVariantMap avatarEntityData; avatarEntityData["id"] = entityID; + EntityItemProperties entityProperties = entity->getProperties(desiredProperties); + QScriptValue scriptProperties; { std::lock_guard guard(_scriptEngineLock); - EntityItemProperties entityProperties = entity->getProperties(desiredProperties); - QScriptValue scriptProperties = EntityItemPropertiesToScriptValue(_scriptEngine, entityProperties); - avatarEntityData["properties"] = scriptProperties.toVariant(); + scriptProperties = EntityItemPropertiesToScriptValue(_scriptEngine, entityProperties); } + avatarEntityData["properties"] = scriptProperties.toVariant(); avatarEntitiesData.append(QVariant(avatarEntityData)); } }