Extremely slow message creation for large arrays in Python

[Flova]

The message serialization is very slow (> hundred seconds for large point clouds), when setting the data property of a PointCloud2 message in Python by passing a byte string (e.g. by NumPy asbytes()).

This does not happen if the PointCloud2 message constructor is used (see #155 for the inconsistent behavior) or if a array.array is passed (does not work with the constructor because of #176).

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • apt
• Version or commit hash:
  • rolling
• Client library (if applicable):
  • rclpy

Steps to reproduce issue

• Create a large Numpy Array called nparr
• Create a matching PointCloud2 message called msg with parameters matching the NumPy array format
• Set the data msg.data = nparr.tobytes()
• Time this process and compare it to setting the data directly in the constructor

The second one is much slower:

• 2.7484822273254395 seconds # byte string set via the constructor
• 412.08923530578613 seconds # byte string set via the property

Expected behavior

Both should be equally fast

Actual behavior

The second one is much slower, because an additional assertion is performed. This assertion is skipped in the constructor because of the bug in #155. It therefore performs much better.

The constructor directly casts the data and sends it to the setter

self.data = array.array('B', kwargs.get('data', []))

The setter itself checks if the cast is already done and skips the casing, including the following assertion

assert \
                ((isinstance(value, Sequence) or
                  isinstance(value, Set) or
                  isinstance(value, UserList)) and
                 not isinstance(value, str) and
                 not isinstance(value, UserString) and
                 all(isinstance(v, int) for v in value) and
                 all(val >= 0 and val < 256 for val in value)), \
                "The 'data' field must be a set or sequence and each value of type 'int' and each unsigned integer in [0, 255]"

The following part of the assertion is critical because it massively slows it down by iterating >2x over all values.

all(isinstance(v, int) for v in value)  and
all(val >= 0 and val < 256 for val in value))

This happens only in __debug__ is set, but that is the case for many dev machines, and this effectively crashes the code by freezing for many seconds.

Implementation considerations

Drop the check for byte strings. As it does not make any sense in the case of this datatype ether way.

I honestly don't know how to change this since the templating seems quite complex. Some advice would be appreciated.

Issue moved from https://github.com/ros2/common_interfaces/issues/177

[clalancette]

The following part of the assertion is critical because it massively slows it down by iterating >2x over all values.

Just a question; why is this > 2x? Isn't it just 2x + a constant amount for all of the isinstance checks above?

Drop the check for byte strings. As it does not make any sense in the case of this datatype ether way.

So I would say that the general problem here is that we don't know the type of the values that are being passed in. We don't even know that they are all the same type, which is why we have the check for if they are an int first. Since Python doesn't have a primitive type for uint8, we need both of those checks for a generic container that is being passed in, because if we don't do it here it will fail later on in obscure and hard-to-debug ways.

So with that said, I think the right thing to do here would probably be to special-case a bytestring so that we just assign it directly without doing the check on it. To do that, you would generally modify the code at https://github.com/ros2/rosidl_python/blob/3dc587255effce330db05e045a5cead78df28339/rosidl_generator_py/resource/_msg.py.em#L463-L521 . Unfortunately it is complex; that is the nature of the templating at the moment. There are tests for at least some of this, so if you do modify it I'll suggest running a full suite of tests.

[Flova]

Just a question; why is this > 2x? Isn't it just 2x + a constant amount for all of the isinstance checks above?

I am a bit confused as well. Maybe I thought this was a list comprehension and not a generator comprehension, causing another iteration by the all operators. Now I only think it is iterating 2x over it. Once for the isinstance check on every element (assuming no element fails the check) and once the bounds checks (also assuming none of them is out of bounds and leads to all exiting early).

So I would say that the general problem here is that we don't know the type of the values that are being passed in. We don't even know that they are all the same type, which is why we have the check for if they are an int first. Since Python doesn't have a primitive type for uint8, we need both of those checks for a generic container that is being passed in, because if we don't do it here it will fail later on in obscure and hard-to-debug ways.

Correct me if I am wrong, but this is only the case if an iterable python object containing references to all elements is passed. A check is definitely required in this case. But the described issue focused on data provided as a bytestring. Checking each byte passes both restrictions/checks by design. This should lead to them being skipped as you suggested.

I will try to wrap my head around the templating and suggest a solution.

[hidmic]

So with that said, I think the right thing to do here would probably be to special-case a bytestring so that we just assign it directly without doing the check on it.

I agree that we'd have to specialize, but I'd suggest we specialize for numpy.ndarray instances within this block instead (as it apply to many numeric types unlike bytestrings). array.array.frombytes() and numpy.ndarray.tobytes() may be used to do the conversion.