From 39dcd1f9bd602bce050816a50a263b8540bbea64 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Sat, 4 Jun 2016 17:55:30 -0700 Subject: [PATCH] Eliminate file IO contentions for the settings --- interface/src/avatar/MyAvatar.cpp | 2 +- libraries/shared/src/SettingHandle.cpp | 65 +++++++++++++++++++ libraries/shared/src/SettingHandle.h | 32 +++++++-- libraries/shared/src/SettingInterface.cpp | 63 +++++++++++------- libraries/shared/src/SettingInterface.h | 10 ++- libraries/shared/src/SettingManager.cpp | 41 +++++++++--- libraries/shared/src/SettingManager.h | 16 +++-- .../shared/src/shared/ReadWriteLockable.h | 2 + 8 files changed, 186 insertions(+), 45 deletions(-) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 6fdcd8f797..f48104235d 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -739,7 +739,7 @@ void MyAvatar::saveData() { settings.endGroup(); } -float loadSetting(QSettings& settings, const char* name, float defaultValue) { +float loadSetting(Settings& settings, const QString& name, float defaultValue) { float value = settings.value(name, defaultValue).toFloat(); if (glm::isnan(value)) { value = defaultValue; diff --git a/libraries/shared/src/SettingHandle.cpp b/libraries/shared/src/SettingHandle.cpp index d2e84d8b75..cad2a0286f 100644 --- a/libraries/shared/src/SettingHandle.cpp +++ b/libraries/shared/src/SettingHandle.cpp @@ -10,11 +10,76 @@ // #include "SettingHandle.h" +#include "SettingManager.h" #include + + const QString Settings::firstRun { "firstRun" }; +Settings::Settings() : + _manager(DependencyManager::get()), + _locker(&(_manager->getLock())) +{ +} + +Settings::~Settings() { +} + +void Settings::remove(const QString& key) { + _manager->remove(key); +} + +QStringList Settings::childGroups() const { + return _manager->childGroups(); +} + +QStringList Settings::childKeys() const { + return _manager->childKeys(); +} + +QStringList Settings::allKeys() const { + return _manager->allKeys(); +} + +bool Settings::contains(const QString& key) const { + return _manager->contains(key); +} + +int Settings::beginReadArray(const QString & prefix) { + return _manager->beginReadArray(prefix); +} + +void Settings::beginWriteArray(const QString& prefix, int size) { + _manager->beginWriteArray(prefix, size); +} + +void Settings::endArray() { + _manager->endArray(); +} + +void Settings::setArrayIndex(int i) { + _manager->setArrayIndex(i); +} + +void Settings::beginGroup(const QString& prefix) { + _manager->beginGroup(prefix); +} + +void Settings::endGroup() { + _manager->endGroup(); +} + +void Settings::setValue(const QString& name, const QVariant& value) { + _manager->setValue(name, value); +} + +QVariant Settings::value(const QString& name, const QVariant& defaultValue) const { + return _manager->value(name, defaultValue); +} + + void Settings::getFloatValueIfValid(const QString& name, float& floatValue) { const QVariant badDefaultValue = NAN; bool ok = true; diff --git a/libraries/shared/src/SettingHandle.h b/libraries/shared/src/SettingHandle.h index bef2daf84c..e83c563036 100644 --- a/libraries/shared/src/SettingHandle.h +++ b/libraries/shared/src/SettingHandle.h @@ -14,19 +14,40 @@ #include -#include -#include -#include +#include +#include +#include +#include +#include #include #include #include "SettingInterface.h" + // TODO: remove -class Settings : public QSettings { +class Settings { public: static const QString firstRun; + Settings(); + ~Settings(); + + void remove(const QString& key); + QStringList childGroups() const; + QStringList childKeys() const; + QStringList allKeys() const; + bool contains(const QString& key) const; + int beginReadArray(const QString & prefix); + void beginWriteArray(const QString& prefix, int size = -1); + void endArray(); + void setArrayIndex(int i); + + void beginGroup(const QString& prefix); + void endGroup(); + + void setValue(const QString& name, const QVariant& value); + QVariant value(const QString& name, const QVariant& defaultValue = QVariant()) const; void getFloatValueIfValid(const QString& name, float& floatValue); void getBoolValue(const QString& name, bool& boolValue); @@ -36,6 +57,9 @@ public: void setQuatValue(const QString& name, const glm::quat& quatValue); void getQuatValueIfValid(const QString& name, glm::quat& quatValue); + + QSharedPointer _manager; + QWriteLocker _locker; }; namespace Setting { diff --git a/libraries/shared/src/SettingInterface.cpp b/libraries/shared/src/SettingInterface.cpp index a931875771..630d52bf7d 100644 --- a/libraries/shared/src/SettingInterface.cpp +++ b/libraries/shared/src/SettingInterface.cpp @@ -9,27 +9,33 @@ // See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html // +#include "SettingInterface.h" + #include #include #include #include #include "PathUtils.h" -#include "SettingInterface.h" #include "SettingManager.h" #include "SharedLogging.h" namespace Setting { - static Manager* privateInstance = nullptr; + static QSharedPointer globalManager; + const QString Interface::FIRST_RUN { "firstRun" }; + // cleans up the settings private instance. Should only be run once at closing down. void cleanupPrivateInstance() { // grab the thread before we nuke the instance - QThread* settingsManagerThread = privateInstance->thread(); - + QThread* settingsManagerThread = DependencyManager::get()->thread(); + // tell the private instance to clean itself up on its thread - privateInstance->deleteLater(); - privateInstance = NULL; + DependencyManager::destroy(); + + // + globalManager->deleteLater(); + globalManager.reset(); // quit the settings manager thread and wait on it to make sure it's gone settingsManagerThread->quit(); @@ -63,14 +69,13 @@ namespace Setting { QThread* thread = new QThread(); Q_CHECK_PTR(thread); thread->setObjectName("Settings Thread"); - - privateInstance = new Manager(); - Q_CHECK_PTR(privateInstance); - QObject::connect(privateInstance, SIGNAL(destroyed()), thread, SLOT(quit())); - QObject::connect(thread, SIGNAL(started()), privateInstance, SLOT(startTimer())); + globalManager = DependencyManager::set(); + + QObject::connect(globalManager.data(), SIGNAL(destroyed()), thread, SLOT(quit())); + QObject::connect(thread, SIGNAL(started()), globalManager.data(), SLOT(startTimer())); QObject::connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater())); - privateInstance->moveToThread(thread); + globalManager->moveToThread(thread); thread->start(); qCDebug(shared) << "Settings thread started."; @@ -79,7 +84,7 @@ namespace Setting { } void Interface::init() { - if (!privateInstance) { + if (!DependencyManager::isSet()) { // WARNING: As long as we are using QSettings this should always be triggered for each Setting::Handle // in an assignment-client - the QSettings backing we use for this means persistence of these // settings from an AC (when there can be multiple terminating at same time on one machine) @@ -87,9 +92,13 @@ namespace Setting { qWarning() << "Setting::Interface::init() for key" << _key << "- Manager not yet created." << "Settings persistence disabled."; } else { - // Register Handle - privateInstance->registerHandle(this); - _isInitialized = true; + _manager = DependencyManager::get(); + auto manager = _manager.lock(); + if (manager) { + // Register Handle + manager->registerHandle(this); + _isInitialized = true; + } // Load value from disk load(); @@ -97,11 +106,13 @@ namespace Setting { } void Interface::deinit() { - if (_isInitialized && privateInstance) { - // Save value to disk - save(); - - privateInstance->removeHandle(_key); + if (_isInitialized && _manager) { + auto manager = _manager.lock(); + if (manager) { + // Save value to disk + save(); + manager->removeHandle(_key); + } } } @@ -113,14 +124,16 @@ namespace Setting { } void Interface::save() { - if (privateInstance) { - privateInstance->saveSetting(this); + auto manager = _manager.lock(); + if (manager) { + manager->saveSetting(this); } } void Interface::load() { - if (privateInstance) { - privateInstance->loadSetting(this); + auto manager = _manager.lock(); + if (manager) { + manager->loadSetting(this); } } } diff --git a/libraries/shared/src/SettingInterface.h b/libraries/shared/src/SettingInterface.h index 3aad048dbe..8a868c5900 100644 --- a/libraries/shared/src/SettingInterface.h +++ b/libraries/shared/src/SettingInterface.h @@ -12,8 +12,10 @@ #ifndef hifi_SettingInterface_h #define hifi_SettingInterface_h -#include -#include +#include +#include +#include +#include namespace Setting { void preInit(); @@ -22,6 +24,8 @@ namespace Setting { class Interface { public: + static const QString FIRST_RUN; + QString getKey() const { return _key; } bool isSet() const { return _isSet; } @@ -44,6 +48,8 @@ namespace Setting { const QString _key; friend class Manager; + + QWeakPointer _manager; }; } diff --git a/libraries/shared/src/SettingManager.cpp b/libraries/shared/src/SettingManager.cpp index 7c0051c809..abe6d50f2e 100644 --- a/libraries/shared/src/SettingManager.cpp +++ b/libraries/shared/src/SettingManager.cpp @@ -9,13 +9,15 @@ // See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html // -#include -#include +#include +#include +#include #include "SettingInterface.h" #include "SettingManager.h" namespace Setting { + Manager::~Manager() { // Cleanup timer stopTimer(); @@ -27,6 +29,10 @@ namespace Setting { // sync will be called in the QSettings destructor } + // Custom deleter does nothing, because we need to shutdown later than the dependency manager + void Manager::customDeleter() { } + + void Manager::registerHandle(Setting::Interface* handle) { QString key = handle->getKey(); withWriteLock([&] { @@ -47,12 +53,17 @@ namespace Setting { handle->setVariant(value(handle->getKey())); } + void Manager::saveSetting(Interface* handle) { + auto key = handle->getKey(); + QVariant handleValue = UNSET_VALUE; if (handle->isSet()) { - setValue(handle->getKey(), handle->getVariant()); - } else { - remove(handle->getKey()); + handleValue = handle->getVariant(); } + + withWriteLock([&] { + _pendingChanges[key] = handleValue; + }); } static const int SAVE_INTERVAL_MSEC = 5 * 1000; // 5 sec @@ -74,12 +85,24 @@ namespace Setting { } void Manager::saveAll() { - withReadLock([&] { - for (auto handle : _handles) { - saveSetting(handle); - } + QHash newPendingChanges; + withWriteLock([&] { + newPendingChanges.swap(_pendingChanges); }); + for (auto key : newPendingChanges.keys()) { + auto newValue = newPendingChanges[key]; + auto savedValue = value(key, UNSET_VALUE); + if (newValue == savedValue) { + continue; + } + if (newValue == UNSET_VALUE) { + remove(key); + } else { + setValue(key, newValue); + } + } + // Restart timer if (_saveTimer) { _saveTimer->start(); diff --git a/libraries/shared/src/SettingManager.h b/libraries/shared/src/SettingManager.h index e4981f1bce..1f309c966f 100644 --- a/libraries/shared/src/SettingManager.h +++ b/libraries/shared/src/SettingManager.h @@ -12,17 +12,23 @@ #ifndef hifi_SettingManager_h #define hifi_SettingManager_h -#include -#include -#include +#include +#include +#include +#include +#include "DependencyManager.h" #include "shared/ReadWriteLockable.h" namespace Setting { class Interface; - class Manager : public QSettings, public ReadWriteLockable { + class Manager : public QSettings, public ReadWriteLockable, public Dependency { Q_OBJECT + + public: + void customDeleter() override; + protected: ~Manager(); void registerHandle(Interface* handle); @@ -40,6 +46,8 @@ namespace Setting { private: QHash _handles; QPointer _saveTimer = nullptr; + const QVariant UNSET_VALUE { QUuid::createUuid().variant() }; + QHash _pendingChanges; friend class Interface; friend void cleanupPrivateInstance(); diff --git a/libraries/shared/src/shared/ReadWriteLockable.h b/libraries/shared/src/shared/ReadWriteLockable.h index 07b46bb92a..539678a73d 100644 --- a/libraries/shared/src/shared/ReadWriteLockable.h +++ b/libraries/shared/src/shared/ReadWriteLockable.h @@ -45,6 +45,8 @@ public: template bool withTryReadLock(F&& f, int timeout) const; + QReadWriteLock& getLock() const { return _lock; } + private: mutable QReadWriteLock _lock { QReadWriteLock::Recursive }; };