From aa74e6930b69dbb21f61ff5dac00d39eb7bbe2c8 Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Sun, 1 Oct 2023 15:26:28 -0500 Subject: [PATCH] Batch delayed flushes to improve performance on macOS --- app/backend/computermanager.cpp | 53 ++++++++++++++++++++++----------- app/backend/computermanager.h | 9 ++++-- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/app/backend/computermanager.cpp b/app/backend/computermanager.cpp index b0fde97e..45f6fe5d 100644 --- a/app/backend/computermanager.cpp +++ b/app/backend/computermanager.cpp @@ -15,6 +15,9 @@ #define SER_HOSTS "hosts" #define SER_HOSTS_BACKUP "hostsbackup" +// We will wait up to this length of time to batch delayed flushes +#define DELAYED_FLUSH_TIMEOUT_MS 10000 + class PcMonitorThread : public QThread { Q_OBJECT @@ -149,7 +152,7 @@ ComputerManager::ComputerManager(QObject *parent) m_PollingRef(0), m_MdnsBrowser(nullptr), m_CompatFetcher(nullptr), - m_NeedsDelayedFlush(false) + m_NeedsFlush(FlushType::None) { QSettings settings; @@ -190,16 +193,17 @@ ComputerManager::~ComputerManager() // Stop the delayed flush thread before acquiring the lock in write mode // to avoid deadlocking with a flush that needs the lock in read mode. { - // Wake the delayed flush thread + // Wake the delayed flush thread for an immediate flush m_DelayedFlushThread->requestInterruption(); m_DelayedFlushCondition.wakeOne(); + m_ImmediateFlushCondition.wakeOne(); // Wait for it to terminate (and finish any pending flush) m_DelayedFlushThread->wait(); delete m_DelayedFlushThread; // Delayed flushes should have completed by now - Q_ASSERT(!m_NeedsDelayedFlush); + Q_ASSERT(m_NeedsFlush == FlushType::None); } QWriteLocker lock(&m_Lock); @@ -237,19 +241,25 @@ void DelayedFlushThread::run() { { QMutexLocker locker(&m_ComputerManager->m_DelayedFlushMutex); - while (!QThread::currentThread()->isInterruptionRequested() && !m_ComputerManager->m_NeedsDelayedFlush) { + // Wait for any flush to be pending + while (!QThread::currentThread()->isInterruptionRequested() && m_ComputerManager->m_NeedsFlush == ComputerManager::FlushType::None) { m_ComputerManager->m_DelayedFlushCondition.wait(&m_ComputerManager->m_DelayedFlushMutex); } // Bail without flushing if we woke up for an interruption alone. // If we have both an interruption and a flush request, do the flush. - if (!m_ComputerManager->m_NeedsDelayedFlush) { + if (m_ComputerManager->m_NeedsFlush == ComputerManager::FlushType::None) { Q_ASSERT(QThread::currentThread()->isInterruptionRequested()); break; } - // Reset the delayed flush flag to ensure any racing saveHosts() call will set it again - m_ComputerManager->m_NeedsDelayedFlush = false; + // If this is a delayed flush and we're not being interrupted, wait on the immediate flush condition (up to the delayed flush timeout). + while (!QThread::currentThread()->isInterruptionRequested() && + m_ComputerManager->m_NeedsFlush != ComputerManager::FlushType::Immediate && + m_ComputerManager->m_ImmediateFlushCondition.wait(&m_ComputerManager->m_DelayedFlushMutex, DELAYED_FLUSH_TIMEOUT_MS)); + + // Reset the flush condition + m_ComputerManager->m_NeedsFlush = ComputerManager::FlushType::None; } // Perform the flush @@ -287,15 +297,24 @@ void DelayedFlushThread::run() { } } -void ComputerManager::saveHosts() +void ComputerManager::saveHosts(bool immediate) { Q_ASSERT(m_DelayedFlushThread != nullptr && m_DelayedFlushThread->isRunning()); // Punt to a worker thread because QSettings on macOS can take ages (> 500 ms) // to persist our host list to disk (especially when a host has a bunch of apps). QMutexLocker locker(&m_DelayedFlushMutex); - m_NeedsDelayedFlush = true; + if (m_NeedsFlush == FlushType::None) { + m_NeedsFlush = immediate ? FlushType::Immediate : FlushType::Delayed; + } + else if (m_NeedsFlush == FlushType::Delayed && immediate) { + // Upgrade a delayed flush to an immediate flush if an immediate request comes in + m_NeedsFlush = FlushType::Immediate; + } m_DelayedFlushCondition.wakeOne(); + if (immediate) { + m_ImmediateFlushCondition.wakeOne(); + } } QHostAddress ComputerManager::getBestGlobalAddressV6(QVector &addresses) @@ -442,8 +461,8 @@ void ComputerManager::handleComputerStateChanged(NvComputer* computer) emit quitAppCompleted(QVariant()); } - // Save updated hosts to QSettings - saveHosts(); + // Save updated hosts to QSettings (delayed and batchable) + saveHosts(false); } QVector ComputerManager::getComputers() @@ -479,8 +498,8 @@ public: m_ComputerManager->m_KnownHosts.remove(m_Computer->uuid); } - // Persist the new host list - m_ComputerManager->saveHosts(); + // Persist the new host list immediately + m_ComputerManager->saveHosts(true); // Delete the polling entry first. This will stop all polling threads too. delete pollingEntry; @@ -520,9 +539,6 @@ void ComputerManager::renameHost(NvComputer* computer, QString name) void ComputerManager::clientSideAttributeUpdated(NvComputer* computer) { - // Persist the change - saveHosts(); - // Notify the UI of the state change handleComputerStateChanged(computer); } @@ -579,8 +595,9 @@ private: emit pairingCompleted(m_Computer, tr("Another pairing attempt is already in progress.")); break; case NvPairingManager::PairState::PAIRED: - // Persist the newly pinned server certificate for this host - m_ComputerManager->saveHosts(); + // Persist the newly pinned server certificate for this host. We do this immediately because + // loss of the pinned cert invalidates the pairing. This is also a very infrequent operation. + m_ComputerManager->saveHosts(true); emit pairingCompleted(m_Computer, nullptr); break; diff --git a/app/backend/computermanager.h b/app/backend/computermanager.h index 4ea2e8fc..96c109da 100644 --- a/app/backend/computermanager.h +++ b/app/backend/computermanager.h @@ -249,7 +249,7 @@ private slots: void handleMdnsServiceResolved(MdnsPendingComputer* computer, QVector& addresses); private: - void saveHosts(); + void saveHosts(bool immediate); QHostAddress getBestGlobalAddressV6(QVector& addresses); @@ -266,5 +266,10 @@ private: DelayedFlushThread* m_DelayedFlushThread; QMutex m_DelayedFlushMutex; QWaitCondition m_DelayedFlushCondition; - bool m_NeedsDelayedFlush; + QWaitCondition m_ImmediateFlushCondition; + enum class FlushType { + None, + Delayed, + Immediate + } m_NeedsFlush; };