Merge pull request #8019 from jherico/settings

Eliminate file IO contentions for settings

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;

libraries/shared/src/SettingHandle.cpp

@@ -10,11 +10,76 @@
 //
 
 #include "SettingHandle.h"
+#include "SettingManager.h"
 
 #include <math.h>
 
+
+
 const QString Settings::firstRun { "firstRun" };
 
+Settings::Settings() : 
+    _manager(DependencyManager::get<Setting::Manager>()), 
+    _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;

libraries/shared/src/SettingHandle.h

@@ -14,19 +14,40 @@
 
 #include <type_traits>
 
-#include <QSettings>
-#include <QString>
-#include <QVariant>
+#include <QtCore/QSettings>
+#include <QtCore/QStack>
+#include <QtCore/QString>
+#include <QtCore/QVariant>
+#include <QtCore/QReadWriteLock>
 
 #include <glm/glm.hpp>
 #include <glm/gtc/quaternion.hpp>
 
 #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<Setting::Manager> _manager;
+    QWriteLocker _locker;
 };
 
 namespace Setting {

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 <QCoreApplication>
 #include <QDebug>
 #include <QFile>
 #include <QThread>
 
 #include "PathUtils.h"
-#include "SettingInterface.h"
 #include "SettingManager.h"
 #include "SharedLogging.h"
 
 namespace Setting {
-    static Manager* privateInstance = nullptr;
+    static QSharedPointer<Manager> 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<Manager>()->thread();
+
         // tell the private instance to clean itself up on its thread
-        privateInstance->deleteLater();
-        privateInstance = NULL;
+        DependencyManager::destroy<Manager>();
+
+        //
+        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<Manager>();
+
+        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<Manager>()) {
             // 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<Manager>();
+            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);
         }
     }
 }

libraries/shared/src/SettingInterface.h

@@ -12,17 +12,23 @@
 #ifndef hifi_SettingInterface_h
 #define hifi_SettingInterface_h
 
-#include <QString>
-#include <QVariant>
+#include <memory>
+#include <QtCore/QWeakPointer>
+#include <QtCore/QString>
+#include <QtCore/QVariant>
 
 namespace Setting {
+    class Manager;
+
     void preInit();
     void init();
     void cleanupSettings();
 
     class Interface {
     public:
-        QString getKey() const { return _key; }
+        static const QString FIRST_RUN;
+
+        const QString& getKey() const { return _key; }
         bool isSet() const { return _isSet; } 
 
         virtual void setVariant(const QVariant& variant) = 0;
@@ -44,6 +50,8 @@ namespace Setting {
         const QString _key;
         
         friend class Manager;
+
+        QWeakPointer<Manager> _manager;
     };
 }
 

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 <QThread>
-#include <QDebug>
+#include <QtCore/QThread>
+#include <QtCore/QDebug>
+#include <QtCore/QUuid>
 
 #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([&] {
@@ -44,15 +50,29 @@ namespace Setting {
     }
 
     void Manager::loadSetting(Interface* handle) {
-        handle->setVariant(value(handle->getKey()));
+        const auto& key = handle->getKey();
+        withWriteLock([&] {
+            QVariant loadedValue;
+            if (_pendingChanges.contains(key)) {
+                loadedValue = _pendingChanges[key];
+            } else {
+                loadedValue = value(key);
+            }
+            handle->setVariant(loadedValue);
+        });
     }
 
+
     void Manager::saveSetting(Interface* handle) {
+        const 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,10 +94,20 @@ namespace Setting {
     }
 
     void Manager::saveAll() {
-        withReadLock([&] {
-            for (auto handle : _handles) {
-                saveSetting(handle);
+        withWriteLock([&] {
+            for (auto key : _pendingChanges.keys()) {
+                auto newValue = _pendingChanges[key];
+                auto savedValue = value(key, UNSET_VALUE);
+                if (newValue == savedValue) {
+                    continue;
+                }
+                if (newValue == UNSET_VALUE) {
+                    remove(key);
+                } else {
+                    setValue(key, newValue);
+                }
             }
+            _pendingChanges.clear();
         });
 
         // Restart timer

libraries/shared/src/SettingManager.h

@@ -12,17 +12,23 @@
 #ifndef hifi_SettingManager_h
 #define hifi_SettingManager_h
 
-#include <QPointer>
-#include <QSettings>
-#include <QTimer>
+#include <QtCore/QPointer>
+#include <QtCore/QSettings>
+#include <QtCore/QTimer>
+#include <QtCore/QUuid>
 
+#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<QString, Interface*> _handles;
         QPointer<QTimer> _saveTimer = nullptr;
+        const QVariant UNSET_VALUE { QUuid::createUuid().variant() };
+        QHash<QString, QVariant> _pendingChanges;
 
         friend class Interface;
         friend void cleanupPrivateInstance();

libraries/shared/src/shared/ReadWriteLockable.h

@@ -45,6 +45,8 @@ public:
     template <typename F>
     bool withTryReadLock(F&& f, int timeout) const;
 
+    QReadWriteLock& getLock() const { return _lock; }
+
 private:
     mutable QReadWriteLock _lock { QReadWriteLock::Recursive };
 };