Add in-place filter updates#95
Conversation
Introduce InPlaceFitler<T> as an FilterBase<T> subclass for filters that can update data in place. Extend FilterChain<T> with an in-place update path and capability query, using in-place filters when available and the existing buffered path for filters without in-place support.
|
Thanks for this! I'll review it tomorrow. |
|
The CI failure looks like it has nothing to do with the code changes - just rolling packages missing at the moment. I'll try re-running it. |
jonbinney
left a comment
jonbinney
left a comment
There was a problem hiding this comment.
Approved, with one question to discuss. I also need to debug the CI failure before merging; It'll take me a few days to do that.
| T * next = &buffer0_; | ||
| for (size_t i = 1; i < reference_pointers_.size(); ++i) { | ||
| auto in_place_filter = | ||
| dynamic_cast<filters::InPlaceFilter<T> *>(reference_pointers_[i].get()); |
There was a problem hiding this comment.
Does this dynamic cast need to happen on every update call? It probably only needs to be done once when the filter plugins are loaded, and then the resulting pointers cached, right? I'm just bringing this up to get your thoughts though - I actually don't think the slight speed increase from caching would be enough to justify the increased code complexity.
There was a problem hiding this comment.
A dynamic_cast is not "huge", but it is not free either. I think for filters that run on point clouds or images, the performance boost would likely be negligible compared to the actual filter work. But if someone uses this on higher-frequency data, for example IMU data, I can see the repeated RTTI check being less ideal.
I think caching this once in configure() would be smart, and I do not see it adding much complexity. We can just store the optional inplace pointers directly:
std::vector<InPlaceFilterBase<T> *> inplace_filters_;Then configure() does the dynamic_cast once when the plugins are loaded, and update() only checks whether the cached pointer is nullptr.
To be honest, I think the cost of the dynamic_cast itself is probably very small, maybe somewhere around 10-200 ns depending on the type hierarchy, compiler, and cache state. So even for something like a 300 Hz IMU pipeline with 10 filters, we would likely still only be talking about a fraction of a millisecond per second overall. (That is only a rough estimate though)
The main reason I lean toward caching it is not because I expect a meaningful performance win, but because the result is static after the plugins are loaded. Whether a filter supports the inplace interface is configuration-time state, not something that changes on every update() call. If we can cache that with a small and straightforward change, I think it makes sense and does not add much complexity.
There was a problem hiding this comment.
But I will leave it up to you. If you prefer caching it, I can make that change.
There was a problem hiding this comment.
The CI failure is a bit confusing. ament_cmake_xmllint is listed in the Rolling distribution metadata:
There also appears to be a prebuilt Noble .deb available for it:
https://packages.ros.org/ros2/ubuntu/pool/main/r/ros-rolling-ament-cmake-xmllint/
So this does not look like the package is simply missing from Rolling. The failure seems to be specifically in rosdep resolving the key for Rolling on Ubuntu Noble.
This will probably get fixed on the ROS infrastructure side, so we can also just wait a few days and rerun the CI.
There was a problem hiding this comment.
Lets leave caching out of this PR. It could always be done in a different PR if we really want it. CI still failing.... I'm traveling so I'll check on it again next week.
There was a problem hiding this comment.
Documentation says rolling is now on Ubuntu 26.04 Resolute (https://docs.ros.org/en/rolling/Installation.html)
Ubuntu Noble is 24.04
There was a problem hiding this comment.
ros-controls/mujoco_ros2_control#178
We need to wait until the upstream ros:rolling Docker images are updating
|
Tick the box to add this pull request to the merge queue (same as
|
2 participants
As discussed in #94, this adds optional in-place update support for
FilterChain<T>.Filters that can update their input directly can opt in by deriving from
InPlaceFilter<T>, while existingFilterBase<T>filters continue to work through the buffered path.Changes
InPlaceFilter<T>as aFilterBase<T>subclass.FilterChain<T>::update(T & data)for in-place chain execution.FilterChain<T>::can_update_fully_in_place()to report whether all configured filters support in-place updates.FilterChain<T>::update(const T &, T &)API and use in-place-capable filters internally where possible.InPlaceIncrementFilter<T>next to the existing increment filter and register it indefault_plugins.xml.