ros2_java icon indicating copy to clipboard operation
ros2_java copied to clipboard

Use array instead of List for ROS message types

Open jacobperron opened this issue 3 years ago • 6 comments

This builds on #151.

Switching from java.util.List to arrays significantly improves performance when converting from C to Java types. Though, for some applications it is still nice to use a list so I've added a convenience methods for getting and setting the value as a List (df08714). I've still opted for having the default getter method return an array type to encourage users to use arrays, since converting to a List is not very efficient.

jacobperron avatar Dec 21 '20 21:12 jacobperron

@ivanpauno Thanks for the review :slightly_smiling_face:

I think we can keep make use of a List backed by an array to get both the performance and the nice API.

I'm open to suggestions for how to make this work, but my understanding is that the performance hit is unavoidable when converting from primitive types to their corresponding boxed type. The key improvement here is in https://github.com/ros2-java/ros2_java/pull/165/commits/9a652359db97027dbe715194f33dc448e999d27d, where we use std::copy in the case of primitive types. IIUC, performance is not being improved for arrays of object types since we still have to instantiate new Java objects for them.

jacobperron avatar Jan 04 '21 20:01 jacobperron

I'm open to suggestions for how to make this work, but my understanding is that the performance hit is unavoidable when converting from primitive types to their corresponding boxed type.

Ah, I missed that part, makes sense.

I'm not sure if the change from List<rcl_interfaces.msg.ParameterValue> to rcl_interfaces.msg.ParameterValue[] is actually worth doing, but the change from List<Byte> to byte[]` surely is.

ivanpauno avatar Jan 04 '21 20:01 ivanpauno

I'm not sure if the change from List<rcl_interfaces.msg.ParameterValue> to rcl_interfaces.msg.ParameterValue[] is actually worth doing, but the change from List<Byte> to byte[]` surely is.

In which context are you referring to?

In the context of code generation, we could generate different code depending if the type is a primitive or not, but I opted for consistency, for example, consider the following ROS message definitions:

Foo.msg

int[] foo_field

Bar.msg

Foo[] bar_field

IMO, it would be odd if Foo.foo_field had array type int[] and Bar.bar_field had list type List<Foo>.

In the context of the rcljava API calls, I don't have a strong opinion. I can add back methods for accepting/returning List types (though we probably want overloads for array types for convenience).

jacobperron avatar Jan 04 '21 22:01 jacobperron

I've added note about performance to the docblock of *AsLIst methods (0729503).

jacobperron avatar Jan 06 '21 22:01 jacobperron

LGTM, but considering that this is a hard breaking change it might be worth waiting until @esteve can take a look.

@ivanpauno thanks for looping me in. If you guys are ok with the API changes, then I'm ok too :-) Thanks @jacobperron for all the work.

esteve avatar Jan 07 '21 18:01 esteve

why not merge into main branch?

caihead123 avatar May 04 '23 03:05 caihead123