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

Speedup conversion of arrays and numpy arrays #29

Open
wants to merge 4 commits into
base: rolling
Choose a base branch
from

Conversation

beckzoli
Copy link

The _convert_value function takes a lot of time converting array types, especially making conversion of image types very slow. This change accelerates this conversion when the complete images are handled (no_arr option is off, no truncation applied).

Example for speedup:

  • Before change:
$ python -m timeit -s "from rosidl_runtime_py import message_to_ordereddict; from sensor_msgs.msg import CompressedImage; img = CompressedImage(data=range(256))" -n 100000 "message_to_ordereddict(img)"
100000 loops, best of 5: 149 usec per loop
  • After change:
$ python -m timeit -s "from rosidl_runtime_py import message_to_ordereddict; from sensor_msgs.msg import CompressedImage; img = CompressedImage(data=range(256))" -n 100000 "message_to_ordereddict(img)"
100000 loops, best of 5: 11.7 usec per loop

The conversion time went down for a compressed image of 256 bytes more than 10 folds, with possibly higher gains at reasonable image sizes.

@beckzoli beckzoli requested a review from sloretz as a code owner October 16, 2024 04:14
Copy link

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! That's an exciting speedup! I have two requests:

@@ -196,52 +196,54 @@ def _convert_value(
value,
*,
field_type=None,
truncate_length=None,
truncate_len=None,
Copy link

Choose a reason for hiding this comment

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

Please keep this argument name as truncate_length. This appears to be the cause of some of the test failures.

@sloretz sloretz added the enhancement New feature or request label Oct 29, 2024
@mjcarroll mjcarroll added the more information needed Further information is requested label Nov 8, 2024
Zoltan Beck added 4 commits December 6, 2024 14:11
The `_convert_value` function takes a lot of time converting array types, especially making conversion of image types very slow.
This change accelerates this conversion when the complete images are handled (`no_arr` option is off, no truncation applied).

Signed-off-by: Zoltan Beck <[email protected]>
Signed-off-by: Zoltan Beck <[email protected]>
This reverts commit 6e9c991.

Signed-off-by: Zoltan Beck <[email protected]>
@beckzoli beckzoli force-pushed the feat/accelerate-array-conversion branch from 3aeeed0 to 2dbc7b1 Compare December 6, 2024 05:13
@beckzoli beckzoli requested a review from sloretz December 6, 2024 05:18
@beckzoli
Copy link
Author

beckzoli commented Dec 6, 2024

Sorry for the delay. Looks like I managed to make the checks green.

Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

@clalancette
Copy link
Contributor

Pulls: #29
Gist: https://gist.githubusercontent.com/clalancette/dc8e0ecb0e171f4f2d19f4ec1255ee63/raw/66885698ef8ded7ffc2eac0d9342b1872a363e8d/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14983

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request more information needed Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants