[23504] Optimize (de)serialization of flat, non-primitive arrays #274

Mario-DL · 2025-07-23T12:57:19Z

Description

This PR makes Fast CDR look for external (de)serialize_array template specializations (now provided by the generated types) so that flat, non-primitive arrays or vectors can be optimally serialized (memcpy) if certain conditions are met.
These changes break API so must be in a new major release.

Related PRs:

[23504] Optimize (de)serialization of flat, non-primitive arrays Fast-DDS-Gen#489
[23504] Add types for the new (de)serialization methods of flat, non-primitive arrays dds-types-test#44
[23504] Optimize (de)serialization of flat, non-primitive arrays Fast-DDS#5953

Contributor Checklist

Commit messages follow the project guidelines.
The code follows the style guidelines of this project.
Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
Any new/modified methods have been properly documented using Doxygen.
NO Changes are backport compatible: they do NOT break ABI nor change library core behavior.
NO Changes are API compatible.
New feature has been added to the versions.md file (if applicable).
N/A Applicable backports have been included in the description.

Reviewer Checklist

The PR has a milestone assigned.
The title and description correctly express the PR's purpose.
Check contributor checklist is correct.
Check CI results: changes do not issue any warning.
Check CI results: CI pass and failing tests are unrelated with the changes.

Signed-off-by: Mario Dominguez <[email protected]>

…TypesShortStruct Signed-off-by: Mario Dominguez <[email protected]>

Signed-off-by: Mario Dominguez <[email protected]>

MiguelCompany

Very nice refactor!
I really like the traits and SFINAE being used.

Leaving some comments

MiguelCompany · 2025-07-30T08:31:17Z

test/cdr/container_introspection.cpp

+    static_assert(static_is_multi_array_primitive<uint8_t const*>::value, "uint8_t* should be a multi primitive array");
+    static_assert(static_is_multi_array_primitive<uint16_t const*>::value,
+            "uint16_t* should be a multi primitive array");
+    static_assert(static_is_multi_array_primitive<uint32_t const*>::value,
+            "uint32_t* should be a multi primitive array");
+    static_assert(static_is_multi_array_primitive<uint64_t const*>::value,
+            "uint64_t* should be a multi primitive array");


Missing signed versions of these tests

Also missing tests for enum

MiguelCompany · 2025-07-30T08:43:59Z

test/cdr/container_introspection.cpp

+    static_assert(!static_is_multi_array_primitive<std::string const*>::value,
+            "std::string* should not be a multi primitive array");


Missing fixed_string

MiguelCompany · 2025-07-30T08:46:23Z

include/fastcdr/Cdr.h

+template<class _T>
+extern void serialize_array(
+        Cdr&,
+        const _T*,
+        const size_t);
+
+template<class _T>
+extern void deserialize_array(
+        Cdr&,
+        _T*,
+        const size_t);
+


Add comments for all four declarations

Also change the declaration order. Either serialize, deserialize, serialize_array, deserialize_array or serialize, serialize_array, deserialize, deserialize_array

MiguelCompany · 2025-07-30T08:55:20Z

include/fastcdr/Cdr.h

    Cdr& serialize(
            const std::array<_T, _Size>& array_t)
    {
-        if (!is_multi_array_primitive(&array_t))


Seems is_multi_array_primitive is not used anymore (since we now rely on static_is_multi_array_primitive).
Might be a good idea to remove file container_recursive_inspector.hpp and perhaps also rename static_is_multi_array_primitive into is_multi_array_primitive.

MiguelCompany · 2025-07-30T09:03:32Z

include/fastcdr/Cdr.h

+        Cdr::state dheader_state {allocate_xcdrv2_dheader()};

-            serialize_array(array_t.data(), array_t.size());
+        serialize(static_cast<int32_t>(vector_t.size()));


Suggested change

serialize(static_cast<int32_t>(vector_t.size()));

serialize(static_cast<uint32_t>(vector_t.size()));

We should check that the size is not bigger than the maximum value of a uint32_t.
I propose having a serialize_collection_length(size_t length) method that throws an exception when length > MAX_UINT32, and then serializes static_cast<uint32_t>(length).

Note: this would imply changes in other methods unrelated to this PR. It might be a good idea to do that refactoring on a separate PR that we can backport.

MiguelCompany · 2025-07-30T09:21:25Z

include/fastcdr/Cdr.h

+            if ((end_ - offset_) < sequence_length)
+            {
+                set_state(state_before_error);
+                throw exception::NotEnoughMemoryException(
+                          exception::NotEnoughMemoryException::NOT_ENOUGH_MEMORY_MESSAGE_DEFAULT);
+            }
+
+            try
+            {
+                vector_t.resize(sequence_length);
+                eprosima::fastcdr::deserialize_array(*this, vector_t.data(), vector_t.size());
+            }
+            catch (exception::Exception& ex)
+            {
+                set_state(state_before_error);
+                ex.raise();
+            }


We shall follow this same part in the XCDRv2 part of the if above.

Again, this seems to come from the old code, so a separate PR fixing it that can be backported should be done.

Refs #23504: Bump project major to 3

5944145

Signed-off-by: Mario Dominguez <[email protected]>

Mario-DL added this to the v3.0.0 milestone Jul 23, 2025

Mario-DL added 5 commits July 23, 2025 15:06

Refs #23504: Add container_introspection_helpers.hpp

ef3491d

Signed-off-by: Mario Dominguez <[email protected]>

Refs #23504: Add tests for container introspection meta-functions

f8f8dab

Signed-off-by: Mario Dominguez <[email protected]>

Refs #23504: external (de)serialize_array() for std::array

343f19c

Signed-off-by: Mario Dominguez <[email protected]>

Refs #23504: external (de)serialize_array() for std::vector

b9baed5

Signed-off-by: Mario Dominguez <[email protected]>

Refs #23504: Fix tests, implement (de)serialize_array() on InnerBasic…

699810b

…TypesShortStruct Signed-off-by: Mario Dominguez <[email protected]>

Mario-DL force-pushed the feature/23504 branch 2 times, most recently from c1a125a to 8a54298 Compare July 23, 2025 13:22

Mario-DL mentioned this pull request Jul 23, 2025

[23504] Optimize (de)serialization of flat, non-primitive arrays eProsima/Fast-DDS-Gen#489

Open

7 tasks

Refs #23504: Linter

931c8dc

Signed-off-by: Mario Dominguez <[email protected]>

Mario-DL force-pushed the feature/23504 branch from 8a54298 to 931c8dc Compare July 23, 2025 13:43

This was referenced Jul 23, 2025

[23504] Add types for the new (de)serialization methods of flat, non-primitive arrays eProsima/dds-types-test#44

Open

[23504] Optimize (de)serialization of flat, non-primitive arrays eProsima/Fast-DDS#5953

Open

MiguelCompany added versions-pending blocked-till-branch-out labels Jul 30, 2025

MiguelCompany requested changes Jul 30, 2025

View reviewed changes

		static_assert(!static_is_multi_array_primitive<std::string const*>::value,
		"std::string* should not be a multi primitive array");

	serialize(static_cast<int32_t>(vector_t.size()));
	serialize(static_cast<uint32_t>(vector_t.size()));

[23504] Optimize (de)serialization of flat, non-primitive arrays #274

Are you sure you want to change the base?

[23504] Optimize (de)serialization of flat, non-primitive arrays #274

Uh oh!

Conversation

Mario-DL commented Jul 23, 2025 • edited by MiguelCompany Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor Checklist

Reviewer Checklist

Uh oh!

MiguelCompany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Mario-DL commented Jul 23, 2025 •

edited by MiguelCompany

Loading