Skip to content

Enable zero-copy#171

Merged
nachovizzo merged 8 commits intoPRBonn:mainfrom
roncapat:enable_ipc
Jul 13, 2023
Merged

Enable zero-copy#171
nachovizzo merged 8 commits intoPRBonn:mainfrom
roncapat:enable_ipc

Conversation

@roncapat
Copy link
Copy Markdown
Contributor

@roncapat roncapat commented Jun 6, 2023

See #164.

For now, only clouds and odometry ownership is moved to IPC manager.
Path publishing for now is kept standard (of course we may create a copy of it and gift to IPC manager).

UNTESTED CODE (this is why it is still draft)

@roncapat roncapat marked this pull request as ready for review June 13, 2023 09:01
@roncapat
Copy link
Copy Markdown
Contributor Author

@nachovizzo this seems working for us (cc. @Fixit-Davide). Could you run again your demo bags as for the previous PR? Theoretically, no need to compose the node to demonstrate that the patch is working, just run it standalone or as you like.

Of course, you should see performance improvements in the following conditions:

  • composed kiss-icp with two subscribers on the same topic (e.g. outgoing point cloud)
  • intra-process enabled

@roncapat
Copy link
Copy Markdown
Contributor Author

roncapat commented Jun 13, 2023

Well... there is a little problem with intra-process enabled. TF2 StaticTransformBroadcaster does not support intra-process on. See my previous patch against TF2 here. I will open a PR against them, but in the meantime I will also need to put in place a workaround in this KISS-ICP patch to remove the problem locally.

Edit: PR opened and fixed locally with custom options.

@roncapat
Copy link
Copy Markdown
Contributor Author

@nachovizzo have you found time to review? BTW, do you think we still need to support Foxy?

@nachovizzo
Copy link
Copy Markdown
Collaborator

@roncapat not really! But it's on my todo-list. I don't think I will have even 5 minutes to check this on the next 2 weeks or so. Apologize for the slowness :(

But definitely going to look for it.

PS: What would be the advantage of killing foxy support?

@roncapat
Copy link
Copy Markdown
Contributor Author

No worries, just checking if I can do any improvements in the meantime :)

Foxy does not have QoS configurability (override support) so as you see the CI job is failing.

QoS override is unfortunately needed to force-disable intra-process on the StaticTransformPublisher. If we don't suppress it, we can't use true intra-process all around the node. I'm trying to figure out if a fix for foxy could exist...

@roncapat
Copy link
Copy Markdown
Contributor Author

Hey @nachovizzo, do you have any updates? :)

@nachovizzo
Copy link
Copy Markdown
Collaborator

@roncapat I guess I will kill ROS2 foxy, since EOL was reached a few weeks ago anyways. I'm reviewing this, testing it, and merging it soon.

Could you give me access to push to your branch? This way you don't need to change everything I'm asking

@roncapat
Copy link
Copy Markdown
Contributor Author

Of course, I sent you the invite :)

Copy link
Copy Markdown
Collaborator

@nachovizzo nachovizzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I only have 1 small question.

Comment thread ros/ros2/Utils.hpp Outdated
@nachovizzo
Copy link
Copy Markdown
Collaborator

@roncapat I'm done reviewing and testing this. Thanks a lot again!

If you have time, just eye-ball the last diff to make sure we are on the same page, I'll merge after you give me thumbs up 👍

Thanks!

@roncapat
Copy link
Copy Markdown
Contributor Author

LGTM :)

@nachovizzo
Copy link
Copy Markdown
Collaborator

Great!

Sorry I squeezed some small non-related changes 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants