Serialization Cache regression between Iron and Jazzy

[jmachowinski]

Hi,
we notices that a performance regression between the Iron and Jazzy release.
Perf analysis show, that the function

rmw_cyclonedds_cpp::CDRWriter::serialize(rmw_cyclonedds_cpp::CDRCursor*, void const*, rmw_cyclonedds_cpp::AnyValueType const*) const

takes around 10x the CPU time in Jazzy, as compared to Iron.
As also dyn_cast pops up heavily in the perf analysis and the serialization code was not changes at all between Jazzy and Iron, we believe, that the check for trivial serialization now returns false in

rmw_cyclonedds/rmw_cyclonedds_cpp/src/Serialization.cpp

Line 534 in b4963be

if (lookup_trivially_serialized(cursor->offset(), value_type)) {

As to why that happens, this is a mystery to us.
@fujitatomoya @mjcarroll any ideas ?

[fujitatomoya]

hmmm, i do not think of the reason for this... strange...

• keyed topic instance does not land in Jazzy. (Support topic instances in DDS topics ros2#1538, thought this could affect the introspection type support around here)
• service introspection has been merged in Iron, so both Iron and Jazzy have this feature. ([service introspection] generate service_event messages rosidl#700)
• the same exact version of cyclonedds releases/0.10.x used in Iron and Jazzy. that means underlying cyclonedds does not change anything.
• ros2/rosidl@jazzy...iron, i do not think i can find anything affects this behavior change...

this leaves me the following hypothetical scenarios...

• compiler / platform change? Jazzy targets Ubuntu 24.04 (GCC 13) vs. Iron on Ubuntu 22.04 (GCC 11). a different compiler version can produce different struct padding/alignment for the same source code, or different behavior around here?

sorry i could not really help here. we usually do not use cyclonedds for our products, so we do not have lots of experience here...

[fujitatomoya]

@eboasson what do you think? does anything ring a bell for you?

[eboasson]

@fujitatomoya I am afraid not, there have been so few changes in rmw_cyclonedds_cpp (and, as you note, Cyclone itself is identical for the ROS 2 releases mentioned) that it almost has to be something platform-specific.

If lookup_trivially_serialized now (~ always) returns false whereas before it often returned true for the specific data types, then I would compare the generated C++ header files. The memory layout rules don't change between the platform versions, but IDL compiler could be generating something (slightly) different. Not entering the "trivially serialized" path would be a reason for the dyn_cast to show up a lot more often. So I'd start with the hypothesis that it is collateral damage.

#550 is supports more Cyclone versions and cleans up the serializer code. While neither should affect this performance aspect (unless I unwittingly improved it), it probably does give a better basis to improve things. Besides, I really want it merged! (And I consider the test failures to be problems in FastDDS.)

[fujitatomoya]

@eboasson thanks for the information !

[jmachowinski]

We traced it down and found additional subscriptions due to a bug in the jazzy port. So the bug was actually not in rmw_cyclone therefore, sorry for the noise.

But while tracking it down, we found some code paths in rmw_cyclone, that were expensive and were easily optimizable.
So at least something good came out of it ;-)
PR can be found in #553.

[fujitatomoya]

@jmachowinski no worries. glad to hear that you found the root cause. thanks for letting us know here.