Added optimization for copying arrays of simple types from python to C using buffer protocol by ksuszka · Pull Request #129 · ros2/rosidl_python

ksuszka · 2021-04-28T15:42:37Z

This is rather naive approach to improve perfomance of copying large amount of data from python to C.
I didn't remove old code, but instead I've added a check if data can be accessed using buffer protocol, and if the answer is yes then a sequence is copied in single step, if the answer is no, then fallback to previous method is used.

It seems to work for ROS2 version of carla_ros_bridge where we discovered this issues. It seems to be the same issue as described in ros2/rclpy#763

sloretz · 2021-04-28T15:59:24Z

Thanks for the PR! Updating this templated code is no easy task :)

I started a full CI run on Linux to see if that catches anything first

I didn't remove old code, but instead I've added a check if data can be accessed using buffer protocol, and if the answer is yes then a sequence is copied in single step, if the answer is no, then fallback to previous method is used.

Sounds reasonable to me

ksuszka · 2021-04-28T18:37:13Z

- PyObject * item = PySequence_Fast_GET_ITEM(seq_field, i);
- if (!item) {
- Py_DECREF(seq_field);
+@[ if isinstance(member.type, AbstractSequence) and isinstance(member.type.value_type, BasicType)]@


Lines 251-280 is a new code. Lines 281-430 is the old code with an additional level of indentation, used as a backup if PyObject_CheckBuffer call fails.

Just as a note for interested parties - github diff view has "Ignore whitespace changes" under the "settings gear" which makes the diff a lot easier to look at in this case

karl-schulz · 2021-04-29T21:13:20Z

Hi, I can confirm that this improves the publishing performance and also fixes ros2/rclpy#763 (tested by applying the changes to the foxy branch).

Thank you @ksuszka this is really great!

clalancette

I've left a couple of comments for consideration.

clalancette · 2021-04-30T13:26:35Z

+ PyErr_SetString(PyExc_RuntimeError, "unable to get buffer");
 Py_DECREF(field);
 return false;


Here I'm wondering whether we should fall back to the "slow" method if PyObject_GetBuffer fails. It's not clear to me in which situations it would fail.

Quote from reference (https://docs.python.org/3/c-api/buffer.html#c.PyObject_GetBuffer): "Send a request to exporter to fill in view as specified by flags. If the exporter cannot provide a buffer of the exact type, it MUST raise PyExc_BufferError, set view->obj to NULL and return -1.".
So, if I understand correctly, in theory it could happen if python structure we try to access is some complex, multidimensional array and we ask for some other shape. But we ask for PyBUF_SIMPLE (https://docs.python.org/3/c-api/buffer.html#shape-strides-suboffsets), so for the simplest possible structure. So it probably will never fail.
However, I'm not a python expert so I'm guessing here.
EDIT: Also, we already check on template level that we are handling AbstractSequence of BasicTypes.

EDIT: Also, we already check on template level that we are handling AbstractSequence of BasicTypes.
This part doesn't matter, I think. At runtime, if the __convert_from_py function was handed a message that had an attribute for data that we thought should be a buffer, but wasn't, we still might fail here.
But in that case the message would have been not what we were expecting anyway, so any decoding would probably eventually fail. I'm OK with not falling back in this case.

clalancette · 2021-04-30T13:27:53Z

+ PyErr_SetString(PyExc_RuntimeError, "unable to copy buffer");
+ PyBuffer_Release(&view);
+ Py_DECREF(field);
+ return false;


Same question here about whether we should fall back to the slow method.

Quote from reference (https://docs.python.org/3/c-api/buffer.html#c.PyBuffer_ToContiguous): "This function fails if len != src->len.".
As I undrestand it - as we just literally took the value from the src it should never happen. However, just for any future possible change in the bahaviour I would left this check, just in case.

clalancette

This looks good to me, and should be a great speedup on Python. I'd like to get another review from @sloretz (and CI) before we move forward.

sloretz

This is a great improvement! I just a couple nitpicks in the error handling.

Also, mind rebasing this on top of master? That will help with running CI.

sloretz · 2021-06-09T00:03:07Z

This is a fantastic contribution! It really makes a big difference for arrays of numeric types.

I checked that the non-buffer API case works, but it isn't that important because the setters for array fields set the field to an array.array() instance. A user would have to set the field beginning with an underscore (or monkey patch out the setter) to actually use that case.

Test script

#!/usr/bin/python3importarrayimportbuiltin_interfaces.msgimportcollectionsimportrclpyfromrclpy.nodeimportNodefromrclpy.qosimportQoSProfilefromtest_msgs.msgimportUnboundedSequencesimportsysclassNoBuffer(collections.Sequence):
def__init__(self, seq):
self._seq=seqdef__getitem__(self, *args, **kwargs):
returnself._seq.__getitem__(*args, **kwargs)
def__len__(self):
returnlen(self._seq)
classSelfCycler(Node):
"""Publish large messages to itself endlessly."""def__init__(self):
super().__init__("self_cycle_py")
pub_qos=QoSProfile(depth=10)
sub_qos=QoSProfile(depth=10)
self._sub=self.create_subscription(UnboundedSequences, topic="seq_py", callback=self._handle_msg, qos_profile=sub_qos)
self._pub=self.create_publisher(UnboundedSequences, topic="seq_py", qos_profile=pub_qos)
self._timer=self.create_timer(0.1, self.publish_msg)
defpublish_msg(self):
num_items=327680# BROKEN :(# self._publish_msg_attr("byte_values", b'\xaa' * num_items)self._publish_msg_attr("bool_values", NoBuffer((True,) *num_items))
self._publish_msg_attr("byte_values", NoBuffer((b'\xaa',) *num_items))
self._publish_msg_attr("char_values", NoBuffer((234,) *num_items))
self._publish_msg_attr("float32_values", NoBuffer((3.14,) *num_items))
self._publish_msg_attr("float64_values", NoBuffer((6.28,) *num_items))
self._publish_msg_attr("int8_values", NoBuffer((127,) *num_items))
self._publish_msg_attr("uint8_values", NoBuffer((235,) *num_items))
self._publish_msg_attr("int16_values", NoBuffer((1234,) *num_items))
self._publish_msg_attr("uint16_values", NoBuffer((17000,) *num_items))
self._publish_msg_attr("int32_values", NoBuffer((100123,) *num_items))
self._publish_msg_attr("uint32_values", NoBuffer((3000123,) *num_items))
self._publish_msg_attr("int64_values", NoBuffer((42,) *num_items))
self._publish_msg_attr("uint64_values", NoBuffer((84,) *num_items))
self._publish_msg_attr("string_values", NoBuffer(("hi",) *num_items))
self._publish_msg_attr("bool_values", (True,) *num_items)
self._publish_msg_attr("byte_values", (b'\xaa',) *num_items)
self._publish_msg_attr("char_values", (234,) *num_items)
self._publish_msg_attr("float32_values", (3.14,) *num_items)
self._publish_msg_attr("float64_values", (6.28,) *num_items)
self._publish_msg_attr("int8_values", (127,) *num_items)
self._publish_msg_attr("uint8_values", (235,) *num_items)
self._publish_msg_attr("int16_values", (1234,) *num_items)
self._publish_msg_attr("uint16_values", (17000,) *num_items)
self._publish_msg_attr("int32_values", (100123,) *num_items)
self._publish_msg_attr("uint32_values", (3000123,) *num_items)
self._publish_msg_attr("int64_values", (42,) *num_items)
self._publish_msg_attr("uint64_values", (84,) *num_items)
self._publish_msg_attr("string_values", ("hi",) *num_items)
self._publish_msg_attr("bool_values", [True,] *num_items)
self._publish_msg_attr("byte_values", [b'\xaa',] *num_items)
self._publish_msg_attr("char_values", [234,] *num_items)
self._publish_msg_attr("float32_values", [3.14,] *num_items)
self._publish_msg_attr("float64_values", [6.28,] *num_items)
self._publish_msg_attr("int8_values", [127,] *num_items)
self._publish_msg_attr("uint8_values", [235,] *num_items)
self._publish_msg_attr("int16_values", [1234,] *num_items)
self._publish_msg_attr("uint16_values", [17000,] *num_items)
self._publish_msg_attr("int32_values", [100123,] *num_items)
self._publish_msg_attr("uint32_values", [3000123,] *num_items)
self._publish_msg_attr("int64_values", [42,] *num_items)
self._publish_msg_attr("uint64_values", [84,] *num_items)
self._publish_msg_attr("string_values", ["hi",] *num_items)
self._publish_msg_attr("char_values", array.array('B', [234,] *num_items))
self._publish_msg_attr("float32_values", array.array('f', [3.14,] *num_items))
self._publish_msg_attr("float64_values", array.array('d', [6.28,] *num_items))
self._publish_msg_attr("int8_values", array.array('b', [127,] *num_items))
self._publish_msg_attr("uint8_values", array.array('B', [235,] *num_items))
self._publish_msg_attr("int16_values", array.array('h', [1234,] *num_items))
self._publish_msg_attr("uint16_values", array.array('H', [17000,] *num_items))
# BROKEN :(# self._publish_msg_attr("int32_values", array.array('l', [100123,] * num_items))# self._publish_msg_attr("uint32_values", array.array('L', [3000123,] * num_items))# self._publish_msg_attr("int64_values", array.array('q', [42,] * num_items))# self._publish_msg_attr("uint64_values", array.array('Q', [84,] * num_items))sys.exit()
def_publish_msg_attr(self, msg_attr, value):
msg=UnboundedSequences()
setattr(msg, msg_attr, value)
t1=self.get_clock().now().to_msg()
self._pub.publish(msg)
t2=self.get_clock().now().to_msg()
self.get_logger().info(f"{msg_attr}{type(value)} took {self.format_dt(t1, t2)}")
defformat_dt(self, t1: builtin_interfaces.msg.Time, t2: builtin_interfaces.msg.Time):
""" Helper for formatting the difference between two stamps in microseconds """us1=t1.sec*1e6+t1.nanosec//1e3us2=t2.sec*1e6+t2.nanosec//1e3returnf"{int(us2-us1):5} [us]"def_handle_msg(self, msg):
pass# Nothing to do heredefmain(args=None):
rclpy.init()
node=SelfCycler()
try:
rclpy.spin(node)
finally:
node.destroy_node()
if__name__=='__main__':
main()

Results on master

[INFO] [1623195713.989881327] [self_cycle_py]: bool_values <class '__main__.NoBuffer'> took 65064 [us]
[INFO] [1623195714.138823820] [self_cycle_py]: byte_values <class '__main__.NoBuffer'> took 65078 [us]
[INFO] [1623195714.381579971] [self_cycle_py]: char_values <class '__main__.NoBuffer'> took 3898 [us]
[INFO] [1623195714.550229209] [self_cycle_py]: float32_values <class '__main__.NoBuffer'> took 10549 [us]
[INFO] [1623195714.720721546] [self_cycle_py]: float64_values <class '__main__.NoBuffer'> took 11546 [us]
[INFO] [1623195714.970193817] [self_cycle_py]: int8_values <class '__main__.NoBuffer'> took 4016 [us]
[INFO] [1623195715.215124704] [self_cycle_py]: uint8_values <class '__main__.NoBuffer'> took 3794 [us]
[INFO] [1623195715.465594131] [self_cycle_py]: int16_values <class '__main__.NoBuffer'> took 10057 [us]
[INFO] [1623195715.715654631] [self_cycle_py]: uint16_values <class '__main__.NoBuffer'> took 9806 [us]
[INFO] [1623195715.971763413] [self_cycle_py]: int32_values <class '__main__.NoBuffer'> took 10592 [us]
[INFO] [1623195716.212935232] [self_cycle_py]: uint32_values <class '__main__.NoBuffer'> took 10460 [us]
[INFO] [1623195716.459274196] [self_cycle_py]: int64_values <class '__main__.NoBuffer'> took 5511 [us]
[INFO] [1623195716.697001717] [self_cycle_py]: uint64_values <class '__main__.NoBuffer'> took 4865 [us]
[INFO] [1623195717.023702700] [self_cycle_py]: string_values <class '__main__.NoBuffer'> took 244059 [us]
[INFO] [1623195717.042494632] [self_cycle_py]: bool_values <class 'tuple'> took 1548 [us]
[INFO] [1623195717.060993832] [self_cycle_py]: byte_values <class 'tuple'> took 1792 [us]
[INFO] [1623195717.108553494] [self_cycle_py]: char_values <class 'tuple'> took 3895 [us]
[INFO] [1623195717.142507722] [self_cycle_py]: float32_values <class 'tuple'> took 9627 [us]
[INFO] [1623195717.176651889] [self_cycle_py]: float64_values <class 'tuple'> took 10167 [us]
[INFO] [1623195717.227831034] [self_cycle_py]: int8_values <class 'tuple'> took 4491 [us]
[INFO] [1623195717.277971306] [self_cycle_py]: uint8_values <class 'tuple'> took 4394 [us]
[INFO] [1623195717.335102504] [self_cycle_py]: int16_values <class 'tuple'> took 11053 [us]
[INFO] [1623195717.389710578] [self_cycle_py]: uint16_values <class 'tuple'> took 9946 [us]
[INFO] [1623195717.442223293] [self_cycle_py]: int32_values <class 'tuple'> took 9908 [us]
[INFO] [1623195717.489922885] [self_cycle_py]: uint32_values <class 'tuple'> took 10574 [us]
[INFO] [1623195717.536908216] [self_cycle_py]: int64_values <class 'tuple'> took 4009 [us]
[INFO] [1623195717.578159363] [self_cycle_py]: uint64_values <class 'tuple'> took 3910 [us]
[INFO] [1623195717.776084602] [self_cycle_py]: string_values <class 'tuple'> took 181596 [us]
[INFO] [1623195717.794193547] [self_cycle_py]: bool_values <class 'list'> took 1369 [us]
[INFO] [1623195717.812611641] [self_cycle_py]: byte_values <class 'list'> took 1626 [us]
[INFO] [1623195717.859447383] [self_cycle_py]: char_values <class 'list'> took 3904 [us]
[INFO] [1623195717.895154971] [self_cycle_py]: float32_values <class 'list'> took 11825 [us]
[INFO] [1623195717.930338661] [self_cycle_py]: float64_values <class 'list'> took 10625 [us]
[INFO] [1623195717.978289517] [self_cycle_py]: int8_values <class 'list'> took 4180 [us]
[INFO] [1623195718.025640987] [self_cycle_py]: uint8_values <class 'list'> took 3949 [us]
[INFO] [1623195718.079544347] [self_cycle_py]: int16_values <class 'list'> took 10764 [us]
[INFO] [1623195718.132896078] [self_cycle_py]: uint16_values <class 'list'> took 10335 [us]
[INFO] [1623195718.186715182] [self_cycle_py]: int32_values <class 'list'> took 10588 [us]
[INFO] [1623195718.235905414] [self_cycle_py]: uint32_values <class 'list'> took 10356 [us]
[INFO] [1623195718.283507342] [self_cycle_py]: int64_values <class 'list'> took 4033 [us]
[INFO] [1623195718.325601239] [self_cycle_py]: uint64_values <class 'list'> took 4091 [us]
[INFO] [1623195718.522702160] [self_cycle_py]: string_values <class 'list'> took 180183 [us]
[INFO] [1623195718.537421971] [self_cycle_py]: char_values <class 'array.array'> took 4822 [us]
[INFO] [1623195718.556528717] [self_cycle_py]: float32_values <class 'array.array'> took 10719 [us]
[INFO] [1623195718.576482634] [self_cycle_py]: float64_values <class 'array.array'> took 10562 [us]
[INFO] [1623195718.591256600] [self_cycle_py]: int8_values <class 'array.array'> took 4608 [us]
[INFO] [1623195718.605320856] [self_cycle_py]: uint8_values <class 'array.array'> took 4311 [us]
[INFO] [1623195718.626135337] [self_cycle_py]: int16_values <class 'array.array'> took 11158 [us]
[INFO] [1623195718.646768185] [self_cycle_py]: uint16_values <class 'array.array'> took 11129 [us]

Results with this PR

[INFO] [1623195780.963380689] [self_cycle_py]: bool_values <class '__main__.NoBuffer'> took 68367 [us]
[INFO] [1623195781.124780865] [self_cycle_py]: byte_values <class '__main__.NoBuffer'> took 76704 [us]
[INFO] [1623195781.389771226] [self_cycle_py]: char_values <class '__main__.NoBuffer'> took 252 [us]
[INFO] [1623195781.556462384] [self_cycle_py]: float32_values <class '__main__.NoBuffer'> took 1552 [us]
[INFO] [1623195781.724900569] [self_cycle_py]: float64_values <class '__main__.NoBuffer'> took 1397 [us]
[INFO] [1623195781.975215139] [self_cycle_py]: int8_values <class '__main__.NoBuffer'> took 250 [us]
[INFO] [1623195782.230622547] [self_cycle_py]: uint8_values <class '__main__.NoBuffer'> took 293 [us]
[INFO] [1623195782.480543196] [self_cycle_py]: int16_values <class '__main__.NoBuffer'> took 291 [us]
[INFO] [1623195782.729770611] [self_cycle_py]: uint16_values <class '__main__.NoBuffer'> took 311 [us]
[INFO] [1623195782.981284070] [self_cycle_py]: int32_values <class '__main__.NoBuffer'> took 897 [us]
[INFO] [1623195783.220575091] [self_cycle_py]: uint32_values <class '__main__.NoBuffer'> took 843 [us]
[INFO] [1623195783.475862916] [self_cycle_py]: int64_values <class '__main__.NoBuffer'> took 2310 [us]
[INFO] [1623195783.719173923] [self_cycle_py]: uint64_values <class '__main__.NoBuffer'> took 1867 [us]
[INFO] [1623195784.086068116] [self_cycle_py]: string_values <class '__main__.NoBuffer'> took 277784 [us]
[INFO] [1623195784.104673597] [self_cycle_py]: bool_values <class 'tuple'> took 1493 [us]
[INFO] [1623195784.123106659] [self_cycle_py]: byte_values <class 'tuple'> took 1744 [us]
[INFO] [1623195784.166476384] [self_cycle_py]: char_values <class 'tuple'> took 246 [us]
[INFO] [1623195784.191412029] [self_cycle_py]: float32_values <class 'tuple'> took 434 [us]
[INFO] [1623195784.217411019] [self_cycle_py]: float64_values <class 'tuple'> took 1403 [us]
[INFO] [1623195784.261776626] [self_cycle_py]: int8_values <class 'tuple'> took 255 [us]
[INFO] [1623195784.305674347] [self_cycle_py]: uint8_values <class 'tuple'> took 257 [us]
[INFO] [1623195784.351714231] [self_cycle_py]: int16_values <class 'tuple'> took 314 [us]
[INFO] [1623195784.398818347] [self_cycle_py]: uint16_values <class 'tuple'> took 320 [us]
[INFO] [1623195784.444154092] [self_cycle_py]: int32_values <class 'tuple'> took 436 [us]
[INFO] [1623195784.481809148] [self_cycle_py]: uint32_values <class 'tuple'> took 494 [us]
[INFO] [1623195784.525865841] [self_cycle_py]: int64_values <class 'tuple'> took 718 [us]
[INFO] [1623195784.563531212] [self_cycle_py]: uint64_values <class 'tuple'> took 682 [us]
[INFO] [1623195784.765877229] [self_cycle_py]: string_values <class 'tuple'> took 185914 [us]
[INFO] [1623195784.783523332] [self_cycle_py]: bool_values <class 'list'> took 1348 [us]
[INFO] [1623195784.800969589] [self_cycle_py]: byte_values <class 'list'> took 1572 [us]
[INFO] [1623195784.846241046] [self_cycle_py]: char_values <class 'list'> took 326 [us]
[INFO] [1623195784.871957117] [self_cycle_py]: float32_values <class 'list'> took 430 [us]
[INFO] [1623195784.898597912] [self_cycle_py]: float64_values <class 'list'> took 723 [us]
[INFO] [1623195784.945617497] [self_cycle_py]: int8_values <class 'list'> took 348 [us]
[INFO] [1623195784.994728133] [self_cycle_py]: uint8_values <class 'list'> took 291 [us]
[INFO] [1623195785.042587106] [self_cycle_py]: int16_values <class 'list'> took 453 [us]
[INFO] [1623195785.091172213] [self_cycle_py]: uint16_values <class 'list'> took 308 [us]
[INFO] [1623195785.137493895] [self_cycle_py]: int32_values <class 'list'> took 490 [us]
[INFO] [1623195785.178155761] [self_cycle_py]: uint32_values <class 'list'> took 491 [us]
[INFO] [1623195785.227621850] [self_cycle_py]: int64_values <class 'list'> took 712 [us]
[INFO] [1623195785.270939707] [self_cycle_py]: uint64_values <class 'list'> took 730 [us]
[INFO] [1623195785.482988367] [self_cycle_py]: string_values <class 'list'> took 193858 [us]
[INFO] [1623195785.492819617] [self_cycle_py]: char_values <class 'array.array'> took 267 [us]
[INFO] [1623195785.501720923] [self_cycle_py]: float32_values <class 'array.array'> took 416 [us]
[INFO] [1623195785.510730501] [self_cycle_py]: float64_values <class 'array.array'> took 630 [us]
[INFO] [1623195785.520222595] [self_cycle_py]: int8_values <class 'array.array'> took 214 [us]
[INFO] [1623195785.529275890] [self_cycle_py]: uint8_values <class 'array.array'> took 184 [us]
[INFO] [1623195785.538551928] [self_cycle_py]: int16_values <class 'array.array'> took 255 [us]
[INFO] [1623195785.548259438] [self_cycle_py]: uint16_values <class 'array.array'> took 283 [us]

Signed-off-by: ksuszka <ksuszka@autonomous-systems.pl>

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl>

Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl>

sloretz · 2021-06-09T20:37:41Z

CI (build: --packages-above-and-dependencies rosidl_generator_py test: --packages-select rosidl_generator_py)

Linux
Linux-aarch64
macOS
Windows

sloretz · 2021-06-11T16:21:18Z

CI LGTM, the warnings on windows are also in the nightly https://ci.ros2.org/view/nightly/job/nightly_win_rel/1961/msbuild/new/

sloretz

LGTM! Thanks for the PR!

sloretz · 2021-10-06T23:58:52Z

@Mergifyio backport galactic

…C using buffer protocol (#129) * Added optimization for copying arrays of simple types from python to C Signed-off-by: ksuszka <ksuszka@autonomous-systems.pl> * Apply suggestions from code review Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> * Remove setting error message twice Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> Co-authored-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit 1796dfd)

mergify · 2021-10-06T23:59:29Z

Command backport galactic: success

Backports have been created
#145 Added optimization for copying arrays of simple types from python to C using buffer protocol (backport #129) has been created for branch galactic

…C using buffer protocol (#129) (#145) * Added optimization for copying arrays of simple types from python to C Signed-off-by: ksuszka <ksuszka@autonomous-systems.pl> * Apply suggestions from code review Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> * Remove setting error message twice Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> Co-authored-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit 1796dfd) Co-authored-by: ksuszka <susel@post.pl>

…C using buffer protocol (ros2#129) * Added optimization for copying arrays of simple types from python to C Signed-off-by: ksuszka <ksuszka@autonomous-systems.pl> * Apply suggestions from code review Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> * Remove setting error message twice Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> Co-authored-by: Chris Lalancette <clalancette@gmail.com>

…C using buffer protocol (ros2#129) * Added optimization for copying arrays of simple types from python to C Signed-off-by: ksuszka <ksuszka@autonomous-systems.pl> * Apply suggestions from code review Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> * Remove setting error message twice Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Aleksandr Rozhdestvenskii <hexonxons@gmail.com>

sloretz · 2021-11-23T23:12:34Z

@Mergifyio backport foxy

…C using buffer protocol (#129) * Added optimization for copying arrays of simple types from python to C Signed-off-by: ksuszka <ksuszka@autonomous-systems.pl> * Apply suggestions from code review Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> * Remove setting error message twice Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> Co-authored-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit 1796dfd)

mergify · 2021-11-23T23:13:18Z

backport foxy

✅ Backports have been created

Details

#148 Added optimization for copying arrays of simple types from python to C using buffer protocol (backport #129) has been created for branch foxy

…C using buffer protocol (#129) (#146) * Added optimization for copying arrays of simple types from python to C Signed-off-by: ksuszka <ksuszka@autonomous-systems.pl> * Apply suggestions from code review Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> * Remove setting error message twice Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl> Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Aleksandr Rozhdestvenskii <hexonxons@gmail.com> Co-authored-by: ksuszka <susel@post.pl> Co-authored-by: Chris Lalancette <clalancette@gmail.com>

aditya2592 · 2021-12-23T03:11:49Z

Does this also fix potential issues with CPP to python conversion? I have a CPP service that sends data in the form of sensor_msgs/Image. For image of size 3 x 720 x 1280, it takes 350 ms for the image to be received in a Python client. For an image of size 3 x 480 x 640, it takes 100 ms. Since the time as well as size difference is close to 3, I was wondering if the data copy issue is a factor here as well.

aprotyas · 2021-12-23T03:52:58Z

Does this also fix potential issues with CPP to python conversion? I have a CPP service that sends data in the form of sensor_msgs/Image. For image of size 3 x 720 x 1280, it takes 350 ms for the image to be received in a Python client. For an image of size 3 x 480 x 640, it takes 100 ms. Since the time as well as size difference is close to 3, I was wondering if the data copy issue is a factor here as well.

Possibly, are you on Foxy? This patch hasn't been released in Foxy yet, but you could try to compare against Galactic and Rolling if possible.

aditya2592 · 2021-12-23T05:11:59Z

Does this also fix potential issues with CPP to python conversion? I have a CPP service that sends data in the form of sensor_msgs/Image. For image of size 3 x 720 x 1280, it takes 350 ms for the image to be received in a Python client. For an image of size 3 x 480 x 640, it takes 100 ms. Since the time as well as size difference is close to 3, I was wondering if the data copy issue is a factor here as well.
Possibly, are you on Foxy? This patch hasn't been released in Foxy yet, but you could try to compare against Galactic and Rolling if possible.

I am trying on Galactic (built from source)

ksuszka force-pushed the array-copying-optimization branch from 966901a to 841dfdbCompare April 28, 2021 15:43

ksuszka commented Apr 28, 2021
View reviewed changes

clalancette reviewed Apr 30, 2021
View reviewed changes

ksuszka force-pushed the array-copying-optimization branch from 0f1d815 to 930c672Compare April 30, 2021 16:46

clalancette approved these changes May 3, 2021
View reviewed changes

sloretz mentioned this pull request May 22, 2021
Fix int32[] and uint32[] 32 bit sequence types only accepting 16 bit sequences #130
Open

clalancette assigned sloretz Jun 3, 2021

sloretz mentioned this pull request Jun 8, 2021
byte[] AKA IDL sequence<octet> fields required to be sequence of bytes instances instead of bytes instance #134
Open

sloretz requested changes Jun 8, 2021
View reviewed changes

Comment threadrosidl_generator_py/resource/_msg_support.c.em Outdated
Comment threadrosidl_generator_py/resource/_msg_support.c.em Outdated

gavanderhoorn mentioned this pull request Jun 9, 2021
Python wrapper performance terrible, especially when customized msg includes big byte array ros2/ros2#1151
Closed

ksuszkaand others added 2 commits June 9, 2021 20:18

Added optimization for copying arrays of simple types from python to C
ea3e4a9
Signed-off-by: ksuszka <ksuszka@autonomous-systems.pl>

Apply suggestions from code review
c727729
Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl>

ksuszka force-pushed the array-copying-optimization branch from 930c672 to c727729Compare June 9, 2021 18:19

Remove setting error message twice
0510135
Signed-off-by: Krzysztof Suszka <ksuszka@autonomous-systems.pl>

ksuszka force-pushed the array-copying-optimization branch from d637f31 to 0510135Compare June 9, 2021 19:20

sloretz approved these changes Jun 11, 2021
View reviewed changes

sloretz merged commit 1796dfd into ros2:masterJun 11, 2021

sloretz mentioned this pull request Jun 11, 2021
Backport array optimization to Galactic and Foxy #135
Closed

ksuszka deleted the array-copying-optimization branch June 12, 2021 09:22

sloretz mentioned this pull request Jun 23, 2021
Publishing large data is 30x-100x slower than for rclcpp ros2/rclpy#763
Closed

mergifyBot mentioned this pull request Oct 6, 2021
Added optimization for copying arrays of simple types from python to C using buffer protocol (backport #129) #145
Merged

This was referenced Oct 13, 2021
Bad performance of the python wrapper for customized message ros2/ros2#1200
Closed
Bad performance of the python wrapper for customized message ros2/rclpy#836
Open

hexonxons mentioned this pull request Oct 18, 2021
Added optimization for copying arrays of simple types from python to C using buffer protocol (backport #129) #146
Merged

mergifyBot mentioned this pull request Nov 23, 2021
Added optimization for copying arrays of simple types from python to C using buffer protocol (backport #129) #148
Closed

aprotyas mentioned this pull request Dec 2, 2021
Very slow hd image publihser (not image transport) for hd image using rclpy ros2/rclpy#856
Open

GiteZz mentioned this pull request Feb 3, 2022
Not supported on ROS2 galactic stereolabs/zed-ros2-wrapper#74
Closed
2 tasks

Uh oh!

Conversation

ksuszka commented Apr 28, 2021

Uh oh!

sloretz commented Apr 28, 2021

Uh oh!

ksuszkaApr 28, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emersonknappMay 19, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karl-schulz commented Apr 29, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clalancetteApr 30, 2021

Choose a reason for hiding this comment

Uh oh!

ksuszkaApr 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancetteMay 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clalancetteApr 30, 2021

Choose a reason for hiding this comment

Uh oh!

ksuszkaApr 30, 2021

Choose a reason for hiding this comment

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sloretz commented Jun 9, 2021

Uh oh!

sloretz commented Jun 9, 2021

Uh oh!

sloretz commented Jun 11, 2021

Uh oh!

sloretz left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz commented Oct 6, 2021

Uh oh!

mergifyBot commented Oct 6, 2021

Uh oh!

sloretz commented Nov 23, 2021

Uh oh!

mergifyBot commented Nov 23, 2021

✅ Backports have been created

Uh oh!

aditya2592 commented Dec 23, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aprotyas commented Dec 23, 2021

Uh oh!

aditya2592 commented Dec 23, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ksuszkaApr 28, 2021•
edited
Loading

emersonknappMay 19, 2021•
edited
Loading

karl-schulz commented Apr 29, 2021•
edited
Loading

ksuszkaApr 30, 2021•
edited
Loading

aditya2592 commented Dec 23, 2021•
edited
Loading

aditya2592 commented Dec 23, 2021•
edited
Loading