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

Don't copy contiguous bytes on reception #343

Merged
merged 11 commits into from
Dec 23, 2024

Conversation

fuzzypixelz
Copy link
Contributor

This uses the slices iterator API of zenoh-c to avoid unecessarily copying bytes into an owned slice, if and only if the bytes is made up of exactly one slice.

@fuzzypixelz
Copy link
Contributor Author

fuzzypixelz commented Dec 16, 2024

The aim of this pull request is to fix high latency for relatively large topics (e.g. 1 MB tagus) in the single-process iRobot benchmark with --ipc off. Results were obtained using the Mont Blanc topology on an Ubuntu 24.04.1 LTS machine with an Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz.

dd82e84

node topic size_b mean_us min_us max_us
ponce tagus 1000000 3016 2512 5705
geneva tagus 1000000 3513 2997 6457
mandalay tagus 1000000 2678 2214 5233

eed223a

node topic size_b mean_us min_us max_us
ponce tagus 1000000 1651 1363 2252
geneva tagus 1000000 1875 1557 2563
mandalay tagus 1000000 1390 1151 1867

eed223a + 401016c

node topic size_b mean_us min_us max_us
ponce tagus 1000000 393 257 1301
geneva tagus 1000000 657 438 1701
mandalay tagus 1000000 108 68 650

@fuzzypixelz fuzzypixelz force-pushed the no-copy-contiguous-bytes branch from a4be550 to eed223a Compare December 16, 2024 17:49
@fuzzypixelz fuzzypixelz changed the title Don't copy contiguous bytes on reception. Don't copy contiguous bytes on reception Dec 16, 2024
@clalancette
Copy link
Collaborator

Thanks for opening the PR.

We are just in the process of converting to zenoh-cpp as our API in #327. I'm assuming a similar idea will work there, but until we merge that, I'm going to mark this as a draft PR, and we can revisit it once we've merged that in.

@clalancette clalancette marked this pull request as draft December 16, 2024 18:13
@fuzzypixelz
Copy link
Contributor Author

Thanks for opening the PR.

We are just in the process of converting to zenoh-cpp as our API in #327. I'm assuming a similar idea will work there, but until we merge that, I'm going to mark this as a draft PR, and we can revisit it once we've merged that in.

The bytes slice iterator API exists in zenoh-cpp, however there is no z_bytes_to_slice equivalent so we would need to manually allocate a buffer in that case or just use as_vector. We could coordinate potential changes to the zenoh-cpp API if necessary.

@clalancette
Copy link
Collaborator

All right, we've now merged in #327. Please rebase this and make any necessary changes to adapt to zenoh-cpp, and then we can do a full review of it.

@fuzzypixelz fuzzypixelz force-pushed the no-copy-contiguous-bytes branch 7 times, most recently from d32f34a to bf7c354 Compare December 18, 2024 14:47
@fuzzypixelz
Copy link
Contributor Author

The following are updated benchmark results after switching from the C to the C++ Zenoh API.

df6ec26

node topic size_b mean_us min_us max_us
ponce tagus 1000000 1117 642 3399
geneva tagus 1000000 1433 868 3775
mandalay tagus 1000000 755 351 2807

bf7c354

node topic size_b mean_us min_us max_us
ponce tagus 1000000 629 402 2343
geneva tagus 1000000 873 611 2674
mandalay tagus 1000000 304 160 1943

@fuzzypixelz
Copy link
Contributor Author

All right, we've now merged in #327. Please rebase this and make any necessary changes to adapt to zenoh-cpp, and then we can do a full review of it.

Should be ready for review now.

@clalancette clalancette marked this pull request as ready for review December 18, 2024 15:16
Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Besides what I put inline, I'd also prefer not to have the implementation in the header file. That means that any downstream consumer has to compile the code all of the time. My suggestion is to just put this whole thing in the rmw_subscription_data.cpp file, but if you want to keep it a separate file then I'd suggest keeping just the API in the .hpp file and putting the implementation in a .cpp file.

rmw_zenoh_cpp/src/detail/payload.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/payload.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/payload.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/payload.hpp Outdated Show resolved Hide resolved
@fuzzypixelz
Copy link
Contributor Author

fuzzypixelz commented Dec 18, 2024

Besides what I put inline, I'd also prefer not to have the implementation in the header file. That means that any downstream consumer has to compile the code all of the time. My suggestion is to just put this whole thing in the rmw_subscription_data.cpp file, but if you want to keep it a separate file then I'd suggest keeping just the API in the .hpp file and putting the implementation in a .cpp file.

I will keep a separate translation unit for Payload as it might be useful for queries as well.

@YuanYuYuan YuanYuYuan mentioned this pull request Dec 18, 2024
rmw_zenoh_cpp/src/detail/payload.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/payload.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/payload.hpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/detail/payload.hpp Outdated Show resolved Hide resolved
@fuzzypixelz fuzzypixelz force-pushed the no-copy-contiguous-bytes branch from 92bdac4 to 662530c Compare December 19, 2024 15:46
@YuanYuYuan
Copy link
Contributor

YuanYuYuan commented Dec 20, 2024

A little suggestion: The Payload name seems to be too generic and we don't explain its behavior (which is a copy-on-write when making the bytes contiguous). Can we give it a proper name like CowPayload or CowBytes?

fuzzypixelz and others added 6 commits December 20, 2024 14:45
This uses the slices iterator API of zenoh-cpp to avoid unecessarily
copying bytes into a vecotr, if and only if the bytes is made up of
exactly one slice.
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Mahmoud Mazouz <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Signed-off-by: Mahmoud Mazouz <[email protected]>
@fuzzypixelz
Copy link
Contributor Author

79fc5ce seems to have broken the Jazzy build.

Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I'm happy with this now. Thanks for the revisions, I'm going to go ahead and merge this.

@clalancette clalancette merged commit cebb972 into ros2:rolling Dec 23, 2024
5 of 6 checks passed
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.

5 participants