ros2_java
ros2_java copied to clipboard
Use array instead of List for ROS message types
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.
@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.
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.
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).
I've added note about performance to the docblock of *AsLIst methods (0729503).
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.
why not merge into main branch?