Pause asynchronous ROS updates with synchronous ones #1635
Conversation
VisualizationManager uses [start|stop]Update() to pause ROS updates during dialog box interaction (ros-visualization#631). However, asynchronous updates via threaded_queue_ were still performed, leading to a memory leak (ros-visualization#1621), because messages were partially processed by the thread (i.e. kept in memory), but not finally processed (and cleaned up) by the synchronous Display::update() call. Pausing threaded processing too resolves that issue.
|
Disclaimer first: I'm writing this from my mobile at Lake Garda ... Will be back in one week for more detailed review
You could use Qt's object tree for cleanup:
timer_ = new QTimer(this);
Will be automatically deleted in the destructor of this (calling delete manually as well is fine as it can deregister itself at the parent)
Use 'new' stile connect (Qt5 or newer):
connect(timer_, &QTimer::timeout, this, &VisManager::onUpdate);
Now to the actual issue: does receiving a PointCloud actually trigger an Ogre rendering? I don't fully understand why it was disabled in 2013 and if we still need this at all?
If we do still need this, your PR looks fine and I cannot think of obvious issues. Haven't tested anything though ...
#868 does not sound like a memory leak but just an unbounded queue. Doesn't the PointCloudDisplay already have a queue size that could be used to discard old messages like suggested in the comment?
Am Donnerstag, 17. Juni 2021 schrieb Robert Haschke:
… VisualizationManager uses `[start|stop]Update()` to pause ROS updates during dialog box interaction (#631).
However, asynchronous updates via `threaded_queue_` were still performed, leading to a memory leak (#1621),
because messages were partially processed by the thread (i.e. kept in memory), but not finally processed
(and cleaned up) by the synchronous `Display::update()` call.
Pausing threaded processing too resolves that issue.
Fixes #1606. Fixes #868.
@simonschmeisser: Do you see any issue pausing asynchronous message processing too while displaying dialogs?
You can view, comment on, or merge this pull request online at:
#1635
-- Commit Summary --
* Stop asynchronous ROS updates with synchronous ones
-- File Changes --
M src/rviz/visualization_manager.cpp (17)
-- Patch Links --
https://github.com/ros-visualization/rviz/pull/1635.patch
https://github.com/ros-visualization/rviz/pull/1635.diff
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1635
--
Gesendet von meinem Sailfish Gerät
|
|
First, I didn't modernize any code (e.g. switching to Qt's object tree instead of manual new/delete or using Qt5-style signal+slots) and I don't think it is worth the effort.
Me neither. There was an issue back than: #629, which was solved by disabling message processing while showing dialogues.
You are right. It's an unbounded queue "only". However, the queue grows rather fast and thus "feels" like a mem leak at first sight. There are several queues involved: First, there is the subscriber message queue, which size is controlled by the display. Second, there is an internal queue to pass partially processed clouds from the processing thread to the main rendering thread. Its size isn't controlled (and grew without bounds). |
PointCloudCommon was defining its own callback queue and spinner to allow for asynchronous processing of point cloud data. However, as most PointCloud displays just inherit MessageFilterDisplay, which is using the default update_nh_, eventually the default synchronous callback queue was used. (Although the callback queue was configured in the constructor to point to PointCloudCommon's private queue, Display::initialize() was resetting the queue back to the default.)
rhaschke
changed the title
VisualizationManager uses
[start|stop]Update()to pause ROS updates during dialog box interaction (#631).However, asynchronous updates via
threaded_queue_were still performed, leading to a memory leak (#1621),because messages were partially processed by the thread (i.e. kept in memory), but not finally processed
(and cleaned up) by the synchronous
Display::update()call.Pausing threaded processing too resolves that issue.
Fixes #1621. Fixes #868.
@simonschmeisser: Do you see any issue pausing asynchronous message processing too while displaying dialogs?