Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Handle zenoh keyexpr with std::string_view rather than const std::string & #325

Open
YuanYuYuan opened this issue Nov 27, 2024 · 2 comments
Labels
backlog Long-term improvement or addition

Comments

@YuanYuYuan
Copy link
Contributor

The zenoh key expression is not null-terminated. That's why we need to attain the pointer with its length while printing the string in zenoh-c.

printf("%.*s", (int)z_string_len(z_loan(keyexpr)), z_string_data(z_loan(keyexpr));

Currently, we use const std::string & to process the passing keyexpr after conversion. This forces us to copy the context by creating a std::string. The most efficient way to borrow and read the zenoh keyexpr is

  1. Use z_keyexpr_as_view_string to read the z_view_string_t from a loaned keyexpr.
  2. Pass the zenoh view string by calling std::string_view(z_string_data, z_string_len).

NOTE
We need to mind the lifetime of owned key expr. Ideally, all the rmw_zenoh entities should own the data rather than a reference to somewhere.

@YuanYuYuan
Copy link
Contributor Author

A relevant discussion before on RCLCPP. ros2/rclcpp#2634

@clalancette
Copy link
Collaborator

As I mentioned in ros2/rclcpp#2634 (comment), I'm not a huge fan of std::string_view; it just seems incredibly easy to get your lifetimes wrong, and thus to access already-freed memory. Passing a constref around is equivalent to passing a pointer, i.e. extremely cheap. So I'm not convinced that switching to a std::string_view will be more efficient, and it is a loaded footgun just waiting for someone to pull the trigger.

In other words, I'm not in favor of this unless it is shown that the processing of our keyexpression is causing a huge slowdown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Long-term improvement or addition
Projects
None yet
Development

No branches or pull requests

3 participants