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

Use array instead of List for ROS message types #165

Open
wants to merge 8 commits into
base: dashing
Choose a base branch
from

Conversation

jacobperron
Copy link
Contributor

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.

pluris and others added 6 commits December 21, 2020 12:02
Copy link
Contributor

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

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

@jacobperron
Copy link
Contributor Author

@ivanpauno Thanks for the review 🙂

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 9a65235, 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.

@ivanpauno
Copy link
Contributor

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.

@jacobperron
Copy link
Contributor Author

jacobperron commented Jan 4, 2021

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 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).

Copy link
Contributor

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

The change sounds reasonable, but we're replacing a relocatable array with an array of immutable length, which isn't that nice to handle and doesn't match what happens in cpp and python.

I'm not sure if there's a way of getting both the performance and the nice API.
(for fixed size IDL arrays, this is surely a better match)

This could also make use of an extra pair of eyes, as I had a hard time reviewing some parts of the empy templates.

rosidl_generator_java/resource/msg.cpp.em Outdated Show resolved Hide resolved
rosidl_generator_java/resource/msg.cpp.em Outdated Show resolved Hide resolved
rosidl_generator_java/resource/msg.cpp.em Outdated Show resolved Hide resolved
Signed-off-by: Jacob Perron <[email protected]>
Copy link
Contributor

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

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

It might be also worth considering what for example the DDS java mapping does (e.g. rti connext docs), they use arrays like int[] for IDL fixed size arrays, but a custom generated type for IDL sequences and bounded sequences.

@jacobperron
Copy link
Contributor Author

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

@esteve
Copy link
Member

esteve commented Jan 7, 2021

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.

@caihead123
Copy link

why not merge into main branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants