From f7397b5e67f4b49be2722db3a4ae2c404121e6da Mon Sep 17 00:00:00 2001
From: Rainer Kartmann <rainer.kartmann@kit.edu>
Date: Mon, 16 Aug 2021 15:51:57 +0200
Subject: [PATCH] Fetch updates asynchronously (out of GUI thread)

---
 .../ArMemMemoryViewerWidgetController.cpp     |   2 +-
 .../armem/client/MemoryNameSystem.cpp         |  27 ++-
 .../libraries/armem/mns/MemoryNameSystem.cpp  |   1 -
 .../libraries/armem_gui/MemoryViewer.cpp      | 173 +++++++++++++++---
 .../libraries/armem_gui/MemoryViewer.h        |  29 ++-
 .../armem_gui/PeriodicUpdateWidget.cpp        |  15 +-
 .../armem_gui/PeriodicUpdateWidget.h          |   8 +-
 7 files changed, 216 insertions(+), 39 deletions(-)

diff --git a/source/RobotAPI/gui-plugins/ArMemMemoryViewer/ArMemMemoryViewerWidgetController.cpp b/source/RobotAPI/gui-plugins/ArMemMemoryViewer/ArMemMemoryViewerWidgetController.cpp
index 780dec62f..ff79f501c 100644
--- a/source/RobotAPI/gui-plugins/ArMemMemoryViewer/ArMemMemoryViewerWidgetController.cpp
+++ b/source/RobotAPI/gui-plugins/ArMemMemoryViewer/ArMemMemoryViewerWidgetController.cpp
@@ -54,7 +54,7 @@ namespace armarx
 
                      widget.statusLabel
                  );
-        viewer->setLogTag(getName());
+        viewer->setLogTag("ArMem Memory Viewer");
 
         armarx::gui::useSplitter(widget.treesLayout);
 
diff --git a/source/RobotAPI/libraries/armem/client/MemoryNameSystem.cpp b/source/RobotAPI/libraries/armem/client/MemoryNameSystem.cpp
index cbacda1eb..79140344e 100644
--- a/source/RobotAPI/libraries/armem/client/MemoryNameSystem.cpp
+++ b/source/RobotAPI/libraries/armem/client/MemoryNameSystem.cpp
@@ -39,7 +39,32 @@ namespace armarx::armem::client
 
         if (result.success)
         {
-            this->servers = result.proxies;
+            for (const auto& [name, proxy] : result.proxies)
+            {
+                auto [it, inserted] = servers.try_emplace(name, proxy);
+                // inserted: OK
+                if (not inserted)
+                {
+                    // Compare ice identities, update if it changed.
+                    if (it->second->ice_getIdentity() != proxy->ice_getIdentity())
+                    {
+                        // Replace old proxy by new one.
+                        it->second = proxy;
+                    }
+                }
+            }
+            // Remove all entries which are not there anymore.
+            for (auto it = servers.begin(); it != servers.end();)
+            {
+                if (result.proxies.count(it->first) == 0)
+                {
+                    it = servers.erase(it);
+                }
+                else
+                {
+                    ++it;
+                }
+            }
         }
         else
         {
diff --git a/source/RobotAPI/libraries/armem/mns/MemoryNameSystem.cpp b/source/RobotAPI/libraries/armem/mns/MemoryNameSystem.cpp
index ece0fa743..6e001784b 100644
--- a/source/RobotAPI/libraries/armem/mns/MemoryNameSystem.cpp
+++ b/source/RobotAPI/libraries/armem/mns/MemoryNameSystem.cpp
@@ -98,7 +98,6 @@ namespace armarx::armem::mns
             }
             catch (const Ice::Exception&)
             {
-                ;
             }
         }
 
diff --git a/source/RobotAPI/libraries/armem_gui/MemoryViewer.cpp b/source/RobotAPI/libraries/armem_gui/MemoryViewer.cpp
index d52b8c8fd..0eab722cc 100644
--- a/source/RobotAPI/libraries/armem_gui/MemoryViewer.cpp
+++ b/source/RobotAPI/libraries/armem_gui/MemoryViewer.cpp
@@ -23,6 +23,7 @@
 #include <QLabel>
 #include <QLayout>
 #include <QSettings>
+#include <QTimer>
 
 #include <iomanip>
 
@@ -63,6 +64,11 @@ namespace armarx::armem::gui
         updateWidget = new armem::gui::PeriodicUpdateWidget(2.0, 60);
         updateWidgetLayout->insertWidget(0, updateWidget);
 
+        processQueryResultTimer = new QTimer(this);
+        processQueryResultTimer->setInterval(1000 / 60);  // Keep this stable.
+        processQueryResultTimer->start();
+
+
         // Memory View
         memoryGroup = new armem::gui::MemoryGroupBox();
         armarx::gui::replaceWidget(memoryGroupBox, memoryGroup, memoryGroupBoxParentLayout);
@@ -88,8 +94,9 @@ namespace armarx::armem::gui
         connect(diskControl, &armem::gui::disk::ControlWidget::requestedStoreOnDisk, this, &This::storeOnDisk);
         connect(diskControl, &armem::gui::disk::ControlWidget::requestedLoadFromDisk, this, &This::loadFromDisk);
 
-        connect(this, &This::connected, this, &This::updateMemories);
-        connect(updateWidget, &armem::gui::PeriodicUpdateWidget::update, this, &This::updateMemories);
+        connect(this, &This::connected, this, &This::startQueries);
+        connect(updateWidget, &armem::gui::PeriodicUpdateWidget::update, this, &This::startQueries);
+        connect(processQueryResultTimer, &QTimer::timeout, this, &This::processQueryResults);
 
         connect(memoryGroup->queryWidget(), &armem::gui::QueryWidget::storeInLTM, this, &This::storeInLTM);
 
@@ -101,11 +108,13 @@ namespace armarx::armem::gui
         connect(instanceGroup->view, &armem::gui::InstanceView::memoryIdResolutionRequested, this, &This::resolveMemoryID);
     }
 
+
     void MemoryViewer::setLogTag(const std::string& _tag)  // Leading _ silences a warning
     {
         Logging::setTag(_tag);
     }
 
+
     void MemoryViewer::onInit(ManagedIceObject& component)
     {
         if (mnsName.size() > 0)
@@ -120,6 +129,7 @@ namespace armarx::armem::gui
         emit initialized();
     }
 
+
     void MemoryViewer::onConnect(ManagedIceObject& component)
     {
         if (!mnsName.empty())
@@ -141,6 +151,7 @@ namespace armarx::armem::gui
         emit connected();
     }
 
+
     void MemoryViewer::onDisconnect(ManagedIceObject&)
     {
         updateWidget->stopTimer();
@@ -148,6 +159,7 @@ namespace armarx::armem::gui
         emit disconnected();
     }
 
+
     const armem::wm::Memory* MemoryViewer::getSingleMemoryData(const std::string& memoryName)
     {
         auto it = memoryData.find(memoryName);
@@ -170,6 +182,7 @@ namespace armarx::armem::gui
         }
     }
 
+
     void MemoryViewer::storeInLTM()
     {
         TIMING_START(MemoryStore)
@@ -184,6 +197,7 @@ namespace armarx::armem::gui
         TIMING_END_STREAM(MemoryStore, ARMARX_VERBOSE)
     }
 
+
     void MemoryViewer::storeOnDisk(QString directory)
     {
         TIMING_START(MemoryExport)
@@ -212,43 +226,25 @@ namespace armarx::armem::gui
     }
 
 
-    void MemoryViewer::updateMemories()
+    void MemoryViewer::startQueries()
     {
-        int errorCount = 0;
-
-        memoryData.clear();
         memoryReaders = mns.getAllReaders(true);
+        startDueQueries(runningQueries, memoryReaders);
+    }
 
-        armem::client::QueryInput input = memoryGroup->queryWidget()->queryInput();
 
-        for (auto& [name, reader] : memoryReaders)
-        {
-            TIMING_START(MemoryQuery)
-            {
-                armem::client::QueryResult result = reader.query(input);
-                if (result.success)
-                {
-                    memoryData[name] = result.memory;
-                }
-                else
-                {
-                    ARMARX_WARNING << "A query for memory '" << name << "' produced an error: " << result.errorMessage;
-                    errorCount++;
-                }
-            }
-            TIMING_END_STREAM(MemoryQuery, ARMARX_VERBOSE)
+    void MemoryViewer::processQueryResults()
+    {
+        const std::map<std::string, client::QueryResult> results = collectQueryResults(runningQueries, memoryReaders);
 
-            if (debugObserver)
-            {
-                debugObserver->setDebugDatafield(Logging::tag.tagName, "Memory Query [ms]", new Variant(MemoryQuery.toMilliSecondsDouble()));
-            }
-        }
+        int errorCount = 0;
+        applyQueryResults(results, &errorCount);
 
         emit memoryDataChanged();
-
         updateStatusLabel(errorCount);
     }
 
+
     void MemoryViewer::updateStatusLabel(int errorCount)
     {
         // Code to output status label information
@@ -266,6 +262,119 @@ namespace armarx::armem::gui
     }
 
 
+    void MemoryViewer::startDueQueries(
+        std::map<std::string, Ice::AsyncResultPtr>& queries,
+        const std::map<std::string, armem::client::Reader>& readers)
+    {
+        armem::client::QueryInput input = memoryGroup->queryWidget()->queryInput();
+
+        for (auto& [name, reader] : readers)
+        {
+            if (queries.count(name) == 0)
+            {
+                queries[name] = reader.memoryPrx->begin_query(input.toIce());
+            }
+        }
+    }
+
+
+    std::map<std::string, client::QueryResult>
+    MemoryViewer::collectQueryResults(
+        std::map<std::string, Ice::AsyncResultPtr>& queries,
+        const std::map<std::string, client::Reader>& readers)
+    {
+        TIMING_START(tCollectQueryResults)
+
+        std::map<std::string, client::QueryResult> results;
+        for (auto it = queries.begin(); it != queries.end();)
+        {
+            const std::string& name = it->first;
+            Ice::AsyncResultPtr& queryPromise = it->second;
+
+            if (queryPromise->isCompleted())
+            {
+                if (auto jt = memoryReaders.find(name); jt != readers.end())
+                {
+                    try
+                    {
+                        results[name] = client::QueryResult::fromIce(jt->second.memoryPrx->end_query(queryPromise));
+                    }
+                    catch (const Ice::ConnectionRefusedException&)
+                    {
+                        // Server is gone (MNS did not now about it yet) => Skip result.
+                    }
+                }
+                // else: Server is gone (MNS new about it) => Skip result.
+
+                // Promise is completed => Clean up in any case.
+                it = runningQueries.erase(it);
+            }
+            else
+            {
+                ++it;  // Uncompleted => Keep.
+            }
+        }
+
+        TIMING_END_STREAM(tCollectQueryResults, ARMARX_VERBOSE)
+        if (debugObserver)
+        {
+            debugObserver->begin_setDebugChannel(Logging::tag.tagName,
+            {
+                { "t Collect Query Results [ms]", new Variant(tCollectQueryResults.toMilliSecondsDouble()) },
+                { "# Collected Query Results", new Variant(static_cast<int>(results.size())) },
+            });
+        }
+
+        return results;
+    }
+
+
+    void MemoryViewer::applyQueryResults(
+        const std::map<std::string, client::QueryResult>& results,
+        int* outErrorCount)
+    {
+        TIMING_START(tProcessQueryResults)
+        for (const auto& [name, result] : results)
+        {
+            if (result.success)
+            {
+                memoryData[name] = std::move(result.memory);
+            }
+            else
+            {
+                ARMARX_WARNING << "Querying memory server '" << name << "' produced an error: \n" << result.errorMessage;
+                if (outErrorCount)
+                {
+                    outErrorCount++;
+                }
+            }
+        }
+
+        // Drop all entries in memoryData which are not in memoryReaders anymore.
+        for (auto it = memoryData.begin(); it != memoryData.end();)
+        {
+            if (memoryReaders.count(it->first) == 0)
+            {
+                it = memoryData.erase(it);
+            }
+            else
+            {
+                ++it;
+            }
+        }
+
+        TIMING_END_STREAM(tProcessQueryResults, ARMARX_VERBOSE)
+        if (debugObserver)
+        {
+            debugObserver->begin_setDebugChannel(Logging::tag.tagName,
+            {
+                { "t Process Query Results [ms]", new Variant(tProcessQueryResults.toMilliSecondsDouble()) },
+                { "# Processed Query Results", new Variant(static_cast<int>(results.size())) },
+            });
+        }
+    }
+
+
     void MemoryViewer::updateInstanceTree(const armem::MemoryID& selectedID)
     {
         const armem::wm::Memory* data = getSingleMemoryData(selectedID.memoryName);
@@ -315,6 +424,7 @@ namespace armarx::armem::gui
         }
     }
 
+
     void MemoryViewer::resolveMemoryID(const MemoryID& id)
     {
         // ARMARX_IMPORTANT << "Resolving memory ID: " << id;
@@ -381,6 +491,7 @@ namespace armarx::armem::gui
         }
     }
 
+
     void MemoryViewer::updateMemoryTree()
     {
         std::map<std::string, const armem::wm::Memory*> convMap;
@@ -401,9 +512,11 @@ namespace armarx::armem::gui
         }
     }
 
+
     const static std::string CONFIG_KEY_MEMORY = "MemoryViewer.MemoryNameSystem";
     const static std::string CONFIG_KEY_DEBUG_OBSERVER = "MemoryViewer.DebugObserverName";
 
+
     void MemoryViewer::loadSettings(QSettings* settings)
     {
         mnsName = settings->value(QString::fromStdString(CONFIG_KEY_MEMORY), "MemoryNameSystem").toString().toStdString();
@@ -415,12 +528,12 @@ namespace armarx::armem::gui
         settings->setValue(QString::fromStdString(CONFIG_KEY_DEBUG_OBSERVER), QString::fromStdString(debugObserverName));
     }
 
+
     void MemoryViewer::writeConfigDialog(SimpleConfigDialog* dialog)
     {
         dialog->addProxyFinder<armarx::armem::mns::MemoryNameSystemInterfacePrx>({CONFIG_KEY_MEMORY, "MemoryNameSystem", "MemoryNameSystem"});
         dialog->addProxyFinder<armarx::DebugObserverInterfacePrx>({CONFIG_KEY_DEBUG_OBSERVER, "Debug Observer", "DebugObserver"});
     }
-
     void MemoryViewer::readConfigDialog(SimpleConfigDialog* dialog)
     {
         mnsName = dialog->getProxyName(CONFIG_KEY_MEMORY);
diff --git a/source/RobotAPI/libraries/armem_gui/MemoryViewer.h b/source/RobotAPI/libraries/armem_gui/MemoryViewer.h
index f2fba3438..e29a23520 100644
--- a/source/RobotAPI/libraries/armem_gui/MemoryViewer.h
+++ b/source/RobotAPI/libraries/armem_gui/MemoryViewer.h
@@ -23,6 +23,7 @@ class QGroupBox;
 class QLabel;
 class QLayout;
 class QSettings;
+class QTimer;
 
 
 namespace armarx
@@ -65,7 +66,6 @@ namespace armarx::armem::gui
 
     public slots:
 
-        void updateMemories();
         void updateInstanceTree(const armem::MemoryID& selectedID);
 
         void resolveMemoryID(const MemoryID& id);
@@ -90,8 +90,13 @@ namespace armarx::armem::gui
 
     private slots:
 
+        void startQueries();
+        void processQueryResults();
+
         void updateMemoryTree();
 
+
+
     signals:
 
         void memoryDataChanged();
@@ -107,6 +112,23 @@ namespace armarx::armem::gui
         void updateStatusLabel(int errorCount);
 
 
+
+        /// Start a query for each entry in `memoryReaders` which is not in `runningQueries`.
+        void startDueQueries(
+            std::map<std::string, Ice::AsyncResultPtr>& queries,
+            const std::map<std::string, armem::client::Reader>& readers);
+
+        std::map<std::string, client::QueryResult>
+        collectQueryResults(
+            std::map<std::string, Ice::AsyncResultPtr>& queries,
+            const std::map<std::string, armem::client::Reader>& readers);
+
+        void applyQueryResults(
+            const std::map<std::string, client::QueryResult>& results,
+            int* outErrorCount = nullptr);
+
+
+
     public:
 
         std::string mnsName;
@@ -115,6 +137,11 @@ namespace armarx::armem::gui
         std::map<std::string, armem::client::Reader> memoryReaders;
         std::map<std::string, armem::wm::Memory> memoryData;
 
+        std::map<std::string, Ice::AsyncResultPtr> runningQueries;
+        /// Periodically triggers query result collection.
+        QTimer* processQueryResultTimer;
+
+
         QLayout* updateWidgetLayout = nullptr;
         armem::gui::PeriodicUpdateWidget* updateWidget = nullptr;
 
diff --git a/source/RobotAPI/libraries/armem_gui/PeriodicUpdateWidget.cpp b/source/RobotAPI/libraries/armem_gui/PeriodicUpdateWidget.cpp
index dd51a5e7f..2e315e6f0 100644
--- a/source/RobotAPI/libraries/armem_gui/PeriodicUpdateWidget.cpp
+++ b/source/RobotAPI/libraries/armem_gui/PeriodicUpdateWidget.cpp
@@ -46,8 +46,11 @@ namespace armarx::armem::gui
         connect(_frequencySpinBox, &QDoubleSpinBox::editingFinished, this, &This::_updateTimerFrequency);
 
         // Public connections.
-        connect(_updateButton, &QPushButton::pressed, this, &This::update);
-        connect(_timer, &QTimer::timeout, this, &This::update);
+        connect(_updateButton, &QPushButton::pressed, this, &This::updateSingle);
+        connect(_timer, &QTimer::timeout, this, &This::updatePeriodic);
+
+        connect(this, &This::updateSingle, this, &This::update);
+        connect(this, &This::updatePeriodic, this, &This::update);
     }
 
     QPushButton* PeriodicUpdateWidget::updateButton()
@@ -55,6 +58,12 @@ namespace armarx::armem::gui
         return _updateButton;
     }
 
+
+    int PeriodicUpdateWidget::getUpdateIntervalMs() const
+    {
+        return static_cast<int>(std::round(1000 / _frequencySpinBox->value()));
+    }
+
     void PeriodicUpdateWidget::startTimerIfEnabled()
     {
         if (_autoCheckBox->isChecked())
@@ -77,7 +86,7 @@ namespace armarx::armem::gui
 
     void PeriodicUpdateWidget::_updateTimerFrequency()
     {
-        _timer->setInterval(static_cast<int>(std::round(1000 / _frequencySpinBox->value())));
+        _timer->setInterval(getUpdateIntervalMs());
     }
 
     void PeriodicUpdateWidget::_toggleAutoUpdates(bool enabled)
diff --git a/source/RobotAPI/libraries/armem_gui/PeriodicUpdateWidget.h b/source/RobotAPI/libraries/armem_gui/PeriodicUpdateWidget.h
index 41b725220..db8d9d5a3 100644
--- a/source/RobotAPI/libraries/armem_gui/PeriodicUpdateWidget.h
+++ b/source/RobotAPI/libraries/armem_gui/PeriodicUpdateWidget.h
@@ -27,8 +27,9 @@ namespace armarx::armem::gui
         QDoubleSpinBox* frequencySpinBox();
         QPushButton* updateButton();
 
-        bool autoEnabled() const;
-        double updateFrequency() const;
+        bool isAutoEnabled() const;
+        double getUpdateFrequency() const;
+        int getUpdateIntervalMs() const;
 
         void startTimerIfEnabled();
         void stopTimer();
@@ -40,6 +41,9 @@ namespace armarx::armem::gui
 
         void update();
 
+        void updateSingle();
+        void updatePeriodic();
+
 
     private slots:
 
-- 
GitLab