Improve wide string (de)serialization #113

MiguelCompany · 2024-02-20T16:04:27Z

This PR changes (de)serialization of wide strings so they don't perform u16string <-> wstring conversions

MiguelCompany · 2024-02-26T16:21:56Z

@clalancette This one is ready for review. It is similar to ros2/rmw_fastrtps#740 (already merged) but for the static type support. I've run the tests in the test_communication package and they passed.

We will base the upgrade to Fast CDR 2 on the changes here.

EduPonz

Changes LGTM. I'm only wondering if we should consider removing u16string_to_wstring and wstring_to_u16string

clalancette

I've left some comments about the sizes of things that we should think about.

Just so I understand; is the goal here to make sure we don't convert from rosidl_runtime_c__U16String to std::wstring, and then serialize the wstring into Cdr? Instead, we serialize directly from the rosidl_runtime_c__U16String into Cdr? I don't have any problem with it, but can you explain why?

Finally, to address @EduPonz point about removing u16string_to_wstring and wstring_to_u16string; I don't think we should do that, at least not for this release. That's because they are public functions and someone may be depending on them. If we think they don't have value anymore, we can mark them as deprecated for this release, and then remove them in the next.

rosidl_typesupport_fastrtps_c/include/rosidl_typesupport_fastrtps_c/serialization_helpers.hpp

clalancette · 2024-03-15T12:59:09Z

rosidl_typesupport_fastrtps_c/include/rosidl_typesupport_fastrtps_c/serialization_helpers.hpp

+  auto len = static_cast<uint32_t>(u16str.size);
+  cdr << len;
+  for (uint32_t i = 0; i < len; ++i) {
+    uint32_t c = static_cast<uint32_t>(u16str.data[i]);


Is there any particular reason to use uint32_t here? I know that rosidl_runtime_c__U16String uses uint_least16_t for the data, but on all platforms we support we have a true uint16_t. So it seems to me that we could change this to:

Suggested change

uint32_t c = static_cast<uint32_t>(u16str.data[i]);

uint16_t c = static_cast<uint16_t>(u16str.data[i]);

The main reason is interoperability / backward compatibility. The serialization of the wstring is using 4 bytes per character.

I initially did what you suggest, and used a uint16_t, but changed it when the tests in test_communication failed between rmw_cyclonedds and rmw_fastrtps for WStrings type.

Note: we can change this in the future if we don't mind breaking backward compatibility, but should do it at the same time in all DDS-based RMW implementations

Ah, got it. OK, interoperability is a really good reason.

In that case, I'm totally fine with leaving this as-is, but please put in a comment saying that we are using uint32_t for the interoperability reason.

clalancette · 2024-03-15T13:01:14Z

rosidl_typesupport_fastrtps_c/include/rosidl_typesupport_fastrtps_c/serialization_helpers.hpp

+    uint32_t c;
+    cdr >> c;
+    u16str.data[i] = static_cast<char16_t>(c);


Same questions here about the sizes; this seems like we should extract a uint16_t, then cast it into a uint_least16_t to put it into the array.

Same reasoning as in here.

I added a check that throws an exception if the deserialized uint32_t does not fit into a uint16_t in commit c4eb425. Note that this would be captured up in rmw_fastrtps code, since it is a fastcdr exception

...l_typesupport_fastrtps_cpp/include/rosidl_typesupport_fastrtps_cpp/serialization_helpers.hpp

…t_fastrtps_c. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

…t_fastrtps_cpp. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

MiguelCompany · 2024-03-18T12:09:25Z

Instead, we serialize directly from the rosidl_runtime_c__U16String into Cdr? I don't have any problem with it, but can you explain why?

due to performance reasons, since we avoid copying into the wstring, and then into the serialization buffer.
It allows us to change the serialization in the future if we want to (see my comment here)

clalancette

The code here looks good to me. I'm just going to request some comments about why we are using uint32_t instead of uint16_t when serializing/deserializing, as well as marking the functions we are no longer using as deprecated. Then we can run CI on this.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

MiguelCompany · 2024-03-18T15:59:44Z

The code here looks good to me. I'm just going to request some comments about why we are using uint32_t instead of uint16_t when serializing/deserializing, as well as marking the functions we are no longer using as deprecated. Then we can run CI on this.

@clalancette I added two commits addressing the two points you mention here.

clalancette

All right, looking good to me now. I'll go ahead and run full CI on this.

clalancette · 2024-03-19T19:18:11Z

CI:

Linux
Linux-aarch64
Windows

clalancette · 2024-03-19T21:34:32Z

Oh. Right. Tests are throwing warnings because they are using the deprecated methods.

I think what we should do here is to update those tests to use the new helpers instead, which will still test what we are trying to test, and fix the warnings.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

MiguelCompany · 2024-03-20T11:04:23Z

@clalancette I updated the tests here and also fixed some linting issues.

clalancette · 2024-03-20T12:08:48Z

All right, here is another shot at CI:

Linux
Linux-aarch64
Windows

MiguelCompany requested a review from sloretz as a code owner February 20, 2024 16:04

MiguelCompany force-pushed the improve-u16-strings branch from 1c9e039 to 7f79e20 Compare February 26, 2024 15:57

EduPonz approved these changes Feb 29, 2024

View reviewed changes

MiguelCompany mentioned this pull request Feb 29, 2024

Support Fast CDR v2 #114

Merged

clalancette requested changes Mar 15, 2024

View reviewed changes

MiguelCompany and others added 7 commits March 18, 2024 10:52

Added serialization helpers for char16_t strings in rosidl_typesuppor…

60fba6f

…t_fastrtps_c. Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Change generated code in rosidl_typesupport_fastrtps_c

2ee730a

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Added serialization helpers for char16_t strings in rosidl_typesuppor…

69cbd23

…t_fastrtps_cpp. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

Change generated code in rosidl_typesupport_fastrtps_cpp

e37b4d3

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Throw overflow_error when size does not fit.

349d1c9

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Change character type casts.

9a9776b

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Throw exception when deserialized char is too big.

c4eb425

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

MiguelCompany force-pushed the improve-u16-strings branch from 7f79e20 to c4eb425 Compare March 18, 2024 11:08

clalancette requested changes Mar 18, 2024

View reviewed changes

Adding comments on uint32_t serialization.

f9a8358

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

clalancette approved these changes Mar 19, 2024

View reviewed changes

MiguelCompany added 6 commits March 20, 2024 10:30

Deprecate unused methods

7f2533a

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Fix dependency on fastcdr

3ba0207

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Update test_wstring_conversion

090b784

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Linters

e8dba93

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Update test_wstring_conversion_mem

24099c5

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Update benchmark_string_conversions

945f193

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

MiguelCompany force-pushed the improve-u16-strings branch from eca7130 to 945f193 Compare March 20, 2024 10:45

clalancette approved these changes Mar 20, 2024

View reviewed changes

clalancette merged commit b4f63c0 into ros2:rolling Mar 20, 2024
2 of 3 checks passed

MiguelCompany deleted the improve-u16-strings branch March 20, 2024 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve wide string (de)serialization #113

Improve wide string (de)serialization #113

MiguelCompany commented Feb 20, 2024

MiguelCompany commented Feb 26, 2024

EduPonz left a comment

clalancette left a comment

clalancette Mar 15, 2024

MiguelCompany Mar 18, 2024

MiguelCompany Mar 18, 2024

clalancette Mar 18, 2024

MiguelCompany Mar 18, 2024

clalancette Mar 15, 2024

MiguelCompany Mar 18, 2024

MiguelCompany Mar 18, 2024

MiguelCompany Mar 18, 2024

MiguelCompany commented Mar 18, 2024

clalancette left a comment

MiguelCompany commented Mar 18, 2024

clalancette left a comment

clalancette commented Mar 19, 2024

clalancette commented Mar 19, 2024

MiguelCompany commented Mar 20, 2024

clalancette commented Mar 20, 2024

	uint32_t c = static_cast<uint32_t>(u16str.data[i]);
	uint16_t c = static_cast<uint16_t>(u16str.data[i]);

Improve wide string (de)serialization #113

Improve wide string (de)serialization #113

Conversation

MiguelCompany commented Feb 20, 2024

MiguelCompany commented Feb 26, 2024

EduPonz left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCompany commented Mar 18, 2024

clalancette left a comment

Choose a reason for hiding this comment

MiguelCompany commented Mar 18, 2024

clalancette left a comment

Choose a reason for hiding this comment

clalancette commented Mar 19, 2024

clalancette commented Mar 19, 2024

MiguelCompany commented Mar 20, 2024

clalancette commented Mar 20, 2024