Mirrors/overte-JulianGro
3
0
Fork 0
mirror of https://github.com/JulianGro/overte.git synced 2025-04-05 23:42:58 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions

Improve settings system, by moving all writes to a thread

Browse source 
This should complete what was started in the HiFi days but didn't quite succeed.

Setting::Manager is now thread safe, and delegates all settings writes to a thread
that nothing waits on, which should ensure that settings don't degrade performance
even on slow storage devices.

Functions that weren't thread safe were removed from Setting::Manager, and it was
reduced to a key/value store.

Functions that modify state like beginGroup were implemented in the Settings class
instead, which should be created only in the context where it's needed. It will
forward all changes to the manager.

A few QSettings functions were left unimplemented because they're not used in
the code. They may be implemented later if there's a need.
This commit is contained in:
Dale Glass 2022-10-23 23:25:42 +02:00
parent 51d16c2ecf
commit cdc15d7821
7 changed files with 360 additions and 207 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

99
libraries/shared/src/SettingHandle.cpp
View file

@ -34,13 +34,11 @@ void Settings::remove(const QString& key) {
_manager->remove(key);
}
QStringList Settings::childGroups() const {
return _manager->childGroups();
}
// QStringList Settings::childGroups() const {
// }
QStringList Settings::childKeys() const {
return _manager->childKeys();
}
// QStringList Settings::childKeys() const {
// }
QStringList Settings::allKeys() const {
return _manager->allKeys();
@ -52,37 +50,56 @@ bool Settings::contains(const QString& key) const {
}
int Settings::beginReadArray(const QString& prefix) {
return _manager->beginReadArray(prefix);
_groups.push(Group(prefix));
_groupPrefix = getGroupPrefix();
int size = _manager->value(_groupPrefix + "/size", -1).toInt();
_groups.top().setSize(size);
return size;
}
void Settings::beginWriteArray(const QString& prefix, int size) {
_manager->beginWriteArray(prefix, size);
_groups.push(Group(prefix));
_groupPrefix = getGroupPrefix();
_manager->setValue(_groupPrefix + "/size", size);
_groups.top().setSize(size);
_groups.top().setIndex(0);
_groupPrefix = getGroupPrefix();
}
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) {
if (_manager->value(name) != value) {
_manager->setValue(name, value);
if (!_groups.empty()) {
_groups.pop();
_groupPrefix = getGroupPrefix();
}
}
void Settings::setArrayIndex(int i) {
if (!_groups.empty()) {
_groups.top().setIndex(i);
_groupPrefix = getGroupPrefix();
}
}
void Settings::beginGroup(const QString& prefix) {
_groups.push(Group(prefix));
_groupPrefix = getGroupPrefix();
}
void Settings::endGroup() {
_groups.pop();
_groupPrefix = getGroupPrefix();
}
void Settings::setValue(const QString& name, const QVariant& value) {
QString fullPath = getPath(name);
_manager->setValue(fullPath, value);
}
QVariant Settings::value(const QString& name, const QVariant& defaultValue) const {
return _manager->value(name, defaultValue);
QString fullPath = getPath(name);
return _manager->value(fullPath, defaultValue);
}
@ -153,3 +170,31 @@ void Settings::getQuatValueIfValid(const QString& name, glm::quat& quatValue) {
endGroup();
}
QString Settings::getGroupPrefix() const {
QString ret;
for(Group g : _groups) {
if (!ret.isEmpty()) {
ret.append("/");
}
ret.append(g.name());
if ( g.isArray() ) {
// QSettings indexes arrays from 1
ret.append(QString("/%1").arg(g.index() + 1));
}
}
return ret;
}
QString Settings::getPath(const QString &key) const {
QString ret = _groupPrefix;
if (!ret.isEmpty() ) {
ret.append("/");
}
ret.append(key);
return ret;
}

57
libraries/shared/src/SettingHandle.h
View file

@ -27,17 +27,63 @@
#include "SettingInterface.h"
// TODO: remove
/**
* @brief QSettings analog
*
* This class emulates the interface of QSettings, and forwards all reads and writes to the global Setting::Manager.
*
* It should be used in the same manner as QSettings -- created only wherever it happens to be needed.
* It's not thread safe, and each thread should have their own.
*
* Unlike QSettings, destruction doesn't cause the config to be saved, instead Config::Manager is in
* charge of taking care of that.
*
* @note childGroups and childKeys are unimplemented because nothing in the code needs them so far.
*/
class Settings {
public:
class Group {
public:
Group(const QString &groupName = QString()) {
_name = groupName;
}
QString name() const { return _name; }
bool isArray() const { return _arraySize != -1; }
void setIndex(int i) {
if ( _arraySize < i+1) {
_arraySize = i+1;
}
_arrayIndex = i;
}
int index() const { return _arrayIndex; }
int size() const { return _arraySize; }
void setSize(int sz) { _arraySize = sz; }
private:
QString _name;
int _arrayIndex{0};
int _arraySize{-1};
};
static const QString firstRun;
Settings();
QString fileName() const;
void remove(const QString& key);
QStringList childGroups() const;
QStringList childKeys() const;
// These are not currently being used
// QStringList childGroups() const;
// QStringList childKeys() const;
QStringList allKeys() const;
bool contains(const QString& key) const;
int beginReadArray(const QString & prefix);
@ -61,7 +107,12 @@ public:
void getQuatValueIfValid(const QString& name, glm::quat& quatValue);
private:
QString getGroupPrefix() const;
QString getPath(const QString &value) const;
QSharedPointer<Setting::Manager> _manager;
QStack<Group> _groups;
QString _groupPrefix;
};
namespace Setting {

53
libraries/shared/src/SettingInterface.cpp
View file

@ -29,17 +29,7 @@ namespace Setting {
auto globalManager = DependencyManager::get<Manager>();
Q_ASSERT(qApp && globalManager);
// Grab the settings thread to shut it down
QThread* settingsManagerThread = globalManager->thread();
// Quit the settings manager thread and wait for it so we
// don't get concurrent accesses when we save all settings below
settingsManagerThread->quit();
settingsManagerThread->wait();
// [IMPORTANT] Save all settings when the QApplication goes down
globalManager->saveAll();
globalManager->forceSave();
qCDebug(shared) << "Settings thread stopped.";
}
@ -48,37 +38,6 @@ namespace Setting {
auto globalManager = DependencyManager::get<Manager>();
Q_ASSERT(qApp && globalManager);
// Let's set up the settings private instance on its own thread
QThread* thread = new QThread(qApp);
Q_CHECK_PTR(thread);
thread->setObjectName("Settings Thread");
// Setup setting periodical save timer
QObject::connect(thread, &QThread::started, globalManager.data(), [globalManager] {
setThreadName("Settings Save Thread");
globalManager->startTimer();
});
QObject::connect(thread, &QThread::finished, globalManager.data(), &Manager::stopTimer);
// Setup manager threading affinity
// This makes the timer fire on the settings thread so we don't block the main
// thread with a lot of file I/O.
// We bring back the manager to the main thread when the QApplication goes down
globalManager->moveToThread(thread);
QObject::connect(thread, &QThread::finished, globalManager.data(), [] {
auto globalManager = DependencyManager::get<Manager>();
Q_ASSERT(qApp && globalManager);
// Move manager back to the main thread (has to be done on owning thread)
globalManager->moveToThread(qApp->thread());
});
// Start the settings save thread
thread->start();
qCDebug(shared) << "Settings thread started.";
// Register cleanupSettingsSaveThread to run inside QCoreApplication's destructor.
// This will cleanup the settings thread and save all settings before shut down.
qAddPostRoutine(cleanupSettingsSaveThread);
}
@ -115,7 +74,7 @@ namespace Setting {
// 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)
// is currently not supported
qWarning() << "Setting::Interface::init() for key" << _key << "- Manager not yet created." <<
qWarning() << "Setting::Interface::init() for key" << _key << "- Manager not yet created." <<
"Settings persistence disabled.";
} else {
_manager = DependencyManager::get<Manager>();
@ -127,7 +86,7 @@ namespace Setting {
} else {
qWarning() << "Settings interface used after manager destroyed";
}
// Load value from disk
load();
}
@ -144,20 +103,20 @@ namespace Setting {
}
}
void Interface::maybeInit() const {
if (!_isInitialized) {
const_cast<Interface*>(this)->init();
}
}
void Interface::save() {
auto manager = _manager.lock();
if (manager) {
manager->saveSetting(this);
}
}
void Interface::load() {
auto manager = _manager.lock();
if (manager) {

179
libraries/shared/src/SettingManager.cpp
View file

@ -19,11 +19,60 @@
namespace Setting {
void WriteWorker::start() {
// QSettings seems to have some issues with moving to a new thread.
// Make sure the object is only created once the thread is already up and running.
init();
}
void WriteWorker::setValue(const QString &key, const QVariant &value) {
//qDebug() << "Setting config " << key << "to" << value;
init();
_qSettings->setValue(key, value);
}
void WriteWorker::removeKey(const QString &key) {
init();
_qSettings->remove(key);
}
void WriteWorker::sync() {
//qDebug() << "Forcing settings sync";
init();
_qSettings->sync();
}
Manager::Manager(QObject *parent) {
WriteWorker *worker = new WriteWorker();
// We operate purely from memory, and forward all changes to a thread that has writing the
// settings as its only job.
qDebug() << "Initializing settings write thread";
_workerThread.setObjectName("Settings Writer");
worker->moveToThread(&_workerThread);
connect(&_workerThread, &QThread::started, worker, &WriteWorker::start, Qt::QueuedConnection);
connect(&_workerThread, &QThread::finished, worker, &QObject::deleteLater, Qt::QueuedConnection);
connect(this, &Manager::valueChanged, worker, &WriteWorker::setValue, Qt::QueuedConnection);
connect(this, &Manager::keyRemoved, worker, &WriteWorker::removeKey, Qt::QueuedConnection);
connect(this, &Manager::syncRequested, worker, &WriteWorker::sync, Qt::QueuedConnection);
_workerThread.start();
// Load all current settings
QSettings settings;
_fileName = settings.fileName();
for(QString key : settings.allKeys()) {
qDebug() << "Loaded key" << key << "with value" << settings.value(key);
_settings[key] = settings.value(key);
}
}
Manager::~Manager() {
// Cleanup timer
stopTimer();
delete _saveTimer;
_saveTimer = nullptr;
}
// Custom deleter does nothing, because we need to shutdown later than the dependency manager
@ -48,12 +97,8 @@ namespace Setting {
void Manager::loadSetting(Interface* handle) {
const auto& key = handle->getKey();
withWriteLock([&] {
QVariant loadedValue;
if (_pendingChanges.contains(key) && _pendingChanges[key] != UNSET_VALUE) {
loadedValue = _pendingChanges[key];
} else {
loadedValue = _qSettings.value(key);
}
QVariant loadedValue = _settings[key];
if (loadedValue.isValid()) {
handle->setVariant(loadedValue);
}
@ -69,56 +114,10 @@ namespace Setting {
}
withWriteLock([&] {
_pendingChanges[key] = handleValue;
_settings[key] = handleValue;
});
}
static const int SAVE_INTERVAL_MSEC = 5 * 1000; // 5 sec
void Manager::startTimer() {
if (!_saveTimer) {
_saveTimer = new QTimer(this);
Q_CHECK_PTR(_saveTimer);
_saveTimer->setSingleShot(true); // We will restart it once settings are saved.
_saveTimer->setInterval(SAVE_INTERVAL_MSEC); // 5s, Qt::CoarseTimer acceptable
connect(_saveTimer, SIGNAL(timeout()), this, SLOT(saveAll()));
}
_saveTimer->start();
}
void Manager::stopTimer() {
if (_saveTimer) {
_saveTimer->stop();
}
}
void Manager::saveAll() {
withWriteLock([&] {
bool forceSync = false;
for (auto key : _pendingChanges.keys()) {
auto newValue = _pendingChanges[key];
auto savedValue = _qSettings.value(key, UNSET_VALUE);
if (newValue == savedValue) {
continue;
}
forceSync = true;
if (newValue == UNSET_VALUE || !newValue.isValid()) {
_qSettings.remove(key);
} else {
_qSettings.setValue(key, newValue);
}
}
_pendingChanges.clear();
if (forceSync) {
_qSettings.sync();
}
});
// Restart timer
if (_saveTimer) {
_saveTimer->start();
}
}
/**
* @brief Forces saving the current configuration
@ -126,92 +125,46 @@ namespace Setting {
* @warning This function is for testing only, should only be called from the test suite.
*/
void Manager::forceSave() {
withWriteLock([&] {
_qSettings.sync();
});
emit syncRequested();
}
QString Manager::fileName() const {
return resultWithReadLock<QString>([&] {
return _qSettings.fileName();
return _fileName;
});
}
void Manager::remove(const QString &key) {
withWriteLock([&] {
_qSettings.remove(key);
_settings.remove(key);
});
}
QStringList Manager::childGroups() const {
return resultWithReadLock<QStringList>([&] {
return _qSettings.childGroups();
});
}
QStringList Manager::childKeys() const {
return resultWithReadLock<QStringList>([&] {
return _qSettings.childKeys();
});
emit keyRemoved(key);
}
QStringList Manager::allKeys() const {
return resultWithReadLock<QStringList>([&] {
return _qSettings.allKeys();
return _settings.keys();
});
}
bool Manager::contains(const QString &key) const {
return resultWithReadLock<bool>([&] {
return _qSettings.contains(key);
});
}
int Manager::beginReadArray(const QString &prefix) {
return resultWithReadLock<int>([&] {
return _qSettings.beginReadArray(prefix);
});
}
void Manager::beginGroup(const QString &prefix) {
withWriteLock([&] {
_qSettings.beginGroup(prefix);
});
}
void Manager::beginWriteArray(const QString &prefix, int size) {
withWriteLock([&] {
_qSettings.beginWriteArray(prefix, size);
});
}
void Manager::endArray() {
withWriteLock([&] {
_qSettings.endArray();
});
}
void Manager::endGroup() {
withWriteLock([&] {
_qSettings.endGroup();
});
}
void Manager::setArrayIndex(int i) {
withWriteLock([&] {
_qSettings.setArrayIndex(i);
return _settings.contains(key);
});
}
void Manager::setValue(const QString &key, const QVariant &value) {
withWriteLock([&] {
_qSettings.setValue(key, value);
_settings[key] = value;
});
emit valueChanged(key, value);
}
QVariant Manager::value(const QString &key, const QVariant &defaultValue) const {
return resultWithReadLock<QVariant>([&] {
return _qSettings.value(key, defaultValue);
return _settings.value(key, defaultValue);
});
}
}

98
libraries/shared/src/SettingManager.h
View file

@ -17,6 +17,9 @@
#include <QtCore/QTimer>
#include <QtCore/QUuid>
#include <QSettings>
#include <QThread>
#include "DependencyManager.h"
#include "shared/ReadWriteLockable.h"
@ -30,25 +33,86 @@ class SettingsTestsWorker;
namespace Setting {
class Interface;
/**
* @brief Settings write worker
*
* This class is used by Setting::Manager to write settings to permanent storage
* without blocking anything else. It receives setting updates, and writes them
* to disk whenever convenient.
*
* All communication to this class must be done over queued connections.
*
* This class is purely an implementation detail and shouldn't be used outside of Setting::Manager.
*
*/
class WriteWorker : public QObject {
Q_OBJECT
public slots:
/**
* @brief Initialize anything that needs initializing, called on thread start.
*
*/
void start();
/**
* @brief Sets a configuration value
*
* @param key Configuration key
* @param value Configuration value
*/
void setValue(const QString &key, const QVariant &value);
/**
* @brief Remove a value from the configuration
*
* @param key Key to remove
*/
void removeKey(const QString &key);
/**
* @brief Force writing the config to disk
*
*/
void sync();
private:
void init() {
if (!_qSettings) {
_qSettings = new QSettings();
}
}
QSettings* _qSettings = nullptr;
};
/**
* @brief Settings manager
*
* This class is the main implementation of the settings system, and the container
* of the current configuration.
*
* Most users should either use the Setting::Handle or the Settings classes instead,
* both of which talk to the single global instance of this class.
*
* The class is thread-safe, and delegates config writing to a separate thread. It
* is safe to change settings as often as it might be needed.
*
*/
class Manager : public QObject, public ReadWriteLockable, public Dependency {
Q_OBJECT
public:
Manager(QObject *parent = nullptr);
void customDeleter() override;
// thread-safe proxies into QSettings
QString fileName() const;
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 beginGroup(const QString &prefix);
void beginWriteArray(const QString &prefix, int size = -1);
void endArray();
void endGroup();
void setArrayIndex(int i);
void setValue(const QString &key, const QVariant &value);
QVariant value(const QString &key, const QVariant &defaultValue = QVariant()) const;
@ -61,17 +125,15 @@ namespace Setting {
void saveSetting(Interface* handle);
void forceSave();
private slots:
void startTimer();
void stopTimer();
void saveAll();
signals:
void valueChanged(const QString &key, const QVariant &value);
void keyRemoved(const QString &key);
void syncRequested();
private:
QHash<QString, Interface*> _handles;
QPointer<QTimer> _saveTimer = nullptr;
const QVariant UNSET_VALUE { QUuid::createUuid() };
QHash<QString, QVariant> _pendingChanges;
friend class Interface;
friend class ::SettingsTests;
@ -81,7 +143,9 @@ namespace Setting {
friend void setupSettingsSaveThread();
QSettings _qSettings;
QHash<QString, QVariant> _settings;
QString _fileName;
QThread _workerThread;
};
}

75
tests/shared/src/SettingsTests.cpp
View file

@ -16,6 +16,8 @@
#include <QDebug>
#include <QCoreApplication>
#include "SettingInterface.h"
#include "SettingHandle.h"
QTEST_MAIN(SettingsTests)
@ -67,6 +69,79 @@ void SettingsTests::saveSettings() {
qDebug() << "Wrote" << s.fileName();
}
void SettingsTests::testSettings() {
auto sm = DependencyManager::get<Setting::Manager>();
Settings s;
s.setValue("settingsTest", 1);
QVERIFY(sm->value("settingsTest") == 1);
}
void SettingsTests::testGroups() {
auto sm = DependencyManager::get<Setting::Manager>();
Settings s;
s.setValue("valueNotInGroupBefore", 1);
s.beginGroup("testGroup");
s.setValue("valueInGroup", 2);
s.endGroup();
s.beginGroup("testGroupFirst");
s.beginGroup("testGroupSecond");
s.setValue("valueInGroup", 44);
s.endGroup();
s.endGroup();
s.setValue("valueNotInGroupAfter", 3);
QVERIFY(sm->value("valueNotInGroupBefore") == 1);
QVERIFY(sm->value("testGroup/valueInGroup") == 2);
QVERIFY(sm->value("testGroupFirst/testGroupSecond/valueInGroup") == 44);
QVERIFY(sm->value("valueNotInGroupAfter") == 3);
}
void SettingsTests::testArray() {
auto sm = DependencyManager::get<Setting::Manager>();
Settings s;
s.beginWriteArray("testArray", 2);
s.setValue("A", 1);
s.setValue("B", 2);
s.setArrayIndex(1);
s.setValue("A", 11);
s.setValue("B", 22);
s.endArray();
s.setValue("valueNotInArray", 6);
QVERIFY(sm->value("testArray/size") == 2);
QVERIFY(sm->value("testArray/1/A") == 1);
QVERIFY(sm->value("testArray/1/B") == 2);
QVERIFY(sm->value("testArray/2/A") == 11);
QVERIFY(sm->value("testArray/2/B") == 22);
QVERIFY(sm->value("valueNotInArray") == 6);
}
void SettingsTests::testArrayInGroup() {
auto sm = DependencyManager::get<Setting::Manager>();
Settings s;
s.beginGroup("groupWithArray");
s.beginWriteArray("arrayInGroup", 2);
s.setValue("X", 10);
s.setArrayIndex(1);
s.setValue("X", 20);
s.endArray();
s.endGroup();
s.setValue("valueNotInArrayOrGroup", 8);
QVERIFY(sm->value("groupWithArray/arrayInGroup/size") == 2);
QVERIFY(sm->value("groupWithArray/arrayInGroup/1/X") == 10);
QVERIFY(sm->value("groupWithArray/arrayInGroup/2/X") == 20);
QVERIFY(sm->value("valueNotInArrayOrGroup") == 8);
}
void SettingsTests::benchmarkSetValue() {
auto sm = DependencyManager::get<Setting::Manager>();
int i = 0;

6
tests/shared/src/SettingsTests.h
View file

@ -29,6 +29,12 @@ private slots:
void loadSettings();
void saveSettings();
void testSettings();
void testGroups();
void testArray();
void testArrayInGroup();
void benchmarkSetValue();
void benchmarkSaveSettings();
void benchmarkSetValueConcurrent();