improve MessageTypeSupport performance. by fujitatomoya · Pull Request #562 · ros2/rmw_cyclonedds

fujitatomoya · 2026-03-06T03:28:13Z

Description

The optimization follows biodranik's suggestion from the PR review, addressing the performance issue at its source.

Removed expensive operations:

❌ std::regex_replace and std::regex("__")
❌ std::ostringstream string concatenation

Replaced with efficient plain string operations:

✅ Simple find/replace loop to replace "__" with "::"
✅ reserve() to pre-allocate exact memory needed
✅ Direct string concatenation with += operator

related to #561

Is this user-facing behavior change?

No,

Did you use Generative AI?

Yes, Claude Sonnet 4.6

Additional Information

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya · 2026-03-06T03:29:13Z

@biodranik CCed

biodranik

Thanks! LGTM if it works.

ahcorde · 2026-03-06T09:15:50Z

Pulls: #562
Gist: https://gist.githubusercontent.com/ahcorde/b79cf81cdcba82bbe8b64dfbbfddae0e/raw/aae245497cfc359b3a20bb870967a5a8913e2461/ros2.repos
BUILD args: --packages-above-and-dependencies rmw_cyclonedds_cpp
TEST args: --packages-above rmw_cyclonedds_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/18372

Linux
Linux-aarch64
Linux-rhel
Windows

fujitatomoya · 2026-03-08T22:47:58Z

@ahcorde is this good to go?

jmachowinski · 2026-03-09T08:08:48Z

I would like to see a performance measurement first.

fujitatomoya · 2026-03-09T09:48:41Z

@jmachowinski do you have benchmark test? or anybody else does?

jmachowinski · 2026-03-09T10:09:01Z

Nope, and I see this is a problem, because we have no way of figuring out, if we improve stuff, or introduce regressions. @mjcarroll recently tried to improve things, and afterwards we figured out, the benchmark was pointless.
For this specific change, one could do a quick test in compiler explore, and check if std::regex is really that slow (I expect it to be, but you never know).
This also looks like it will not improve the real performance of a system, as this constructor will only be called a few times during initialization of the serializers etc. And this is exactly my point, I think we are optimizing the wrong things here...

jmachowinski · 2026-03-09T10:09:44Z

rmw_cyclonedds_cpp/src/MessageTypeSupport_impl.hpp

+  name += message_name;
+  name += '_';
+
+  this->setName(name.c_str());


setName accepts a std::string

Suggested change

this->setName(name.c_str());

this->setName(name));

jmachowinski · 2026-03-09T10:14:49Z

rmw_cyclonedds_cpp/src/MessageTypeSupport_impl.hpp

  std::string message_namespace(this->members_->message_namespace_);
  std::string message_name(this->members_->message_name_);
+
  if (!message_namespace.empty()) {
    // Find and replace C namespace separator with C++, in case this is using C typesupport
-    message_namespace = std::regex_replace(message_namespace, std::regex("__"), "::");
-    ss << message_namespace << "::";
+    std::string::size_type pos = 0;
+    while ((pos = message_namespace.find("__", pos)) != std::string::npos) {
+      message_namespace.replace(pos, 2, "::");
+      pos += 2;
+    }
+  }
+
+  std::string name;
+  name.reserve(
+    message_namespace.size() + 2 + 5 + message_name.size() + 1);  // "::" + "dds_::" + "_"
+
+  if (!message_namespace.empty()) {
+    name += message_namespace;
+    name += "::";
  }


The code flow is a bit odd, why not declare name first, and do everything related to the namespace in one if, and keep the 'namespace string' scope local to the if ?

Agree, the order should be

if (!message_namespace.empty()) { std::string name; name.reserve(...) // ... }

Timple · 2026-03-09T10:36:56Z

rmw_cyclonedds_cpp/src/MessageTypeSupport_impl.hpp

-    message_namespace = std::regex_replace(message_namespace, std::regex("__"), "::");
-    ss << message_namespace << "::";
+    std::string::size_type pos = 0;
+    while ((pos = message_namespace.find("__", pos)) != std::string::npos) {


Perhaps comment that this is like this for performance reasons.
People might read this snippet and try to clean as you go and introduce a regex :)

Any C++ developer should understand that:

There's a huge overhead when using std::regex

In-place/in-memory replacement is faster than any memory allocation + memory copying

For this one there are good arguments for both code versions. If it is really only one time, the performance does not matter, and I would argue that maintainability and simplicity out weight performance. If it is in a hot code path the proposed version is clearly the winner. Anyway I am fine with both versions.

biodranik

This also looks like it will not improve the real performance of a system, as this constructor will only be called a few times during initialization of the serializers etc. And this is exactly my point, I think we are optimizing the wrong things here...

Even if this is not a performance-critical place, avoiding unnecessary, avoidable overhead everywhere in the code would be a good example to new contributors (and LLMs) to produce a better quality, faster code.

FOSS should not be slow and ugly, right?

biodranik · 2026-03-09T10:40:43Z

rmw_cyclonedds_cpp/src/MessageTypeSupport_impl.hpp

-    message_namespace = std::regex_replace(message_namespace, std::regex("__"), "::");
-    ss << message_namespace << "::";
+    std::string::size_type pos = 0;
+    while ((pos = message_namespace.find("__", pos)) != std::string::npos) {


Any C++ developer should understand that:

There's a huge overhead when using std::regex

In-place/in-memory replacement is faster than any memory allocation + memory copying

biodranik · 2026-03-09T10:42:13Z

rmw_cyclonedds_cpp/src/MessageTypeSupport_impl.hpp

  std::string message_namespace(this->members_->message_namespace_);
  std::string message_name(this->members_->message_name_);
+
  if (!message_namespace.empty()) {
    // Find and replace C namespace separator with C++, in case this is using C typesupport
-    message_namespace = std::regex_replace(message_namespace, std::regex("__"), "::");
-    ss << message_namespace << "::";
+    std::string::size_type pos = 0;
+    while ((pos = message_namespace.find("__", pos)) != std::string::npos) {
+      message_namespace.replace(pos, 2, "::");
+      pos += 2;
+    }
+  }
+
+  std::string name;
+  name.reserve(
+    message_namespace.size() + 2 + 5 + message_name.size() + 1);  // "::" + "dds_::" + "_"
+
+  if (!message_namespace.empty()) {
+    name += message_namespace;
+    name += "::";
  }


Agree, the order should be

if (!message_namespace.empty()) { std::string name; name.reserve(...) // ... }

mjcarroll · 2026-03-09T16:42:47Z

And this is exactly my point, I think we are optimizing the wrong things here...

I agree that this isn't going to have a huge impact, but getting this code cleaner won't hurt. I wouldn't encourage people to go out of their way to hunt for these things, but in this case it was uncovered when looking for other optimizations, so it seems low cost to just fix now.

improve MessageTypeSupport performance.

73fc045

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

fujitatomoya self-assigned this Mar 6, 2026

fujitatomoya requested review from jmachowinski and mjcarroll March 6, 2026 03:28

fujitatomoya mentioned this pull request Mar 6, 2026

perf(rmw_cyclonedds_cpp): Add thread-local caching to rmw_serialize and rmw_deserialize #561

Open

biodranik approved these changes Mar 6, 2026

View reviewed changes

jmachowinski reviewed Mar 9, 2026

View reviewed changes

Timple reviewed Mar 9, 2026

View reviewed changes

biodranik reviewed Mar 9, 2026

View reviewed changes

Conversation

fujitatomoya commented Mar 6, 2026

Description

Is this user-facing behavior change?

Did you use Generative AI?

Additional Information

Uh oh!

fujitatomoya commented Mar 6, 2026

Uh oh!

biodranik left a comment

Choose a reason for hiding this comment

Uh oh!

ahcorde commented Mar 6, 2026

Uh oh!

fujitatomoya commented Mar 8, 2026

Uh oh!

jmachowinski commented Mar 9, 2026

Uh oh!

fujitatomoya commented Mar 9, 2026

Uh oh!

jmachowinski commented Mar 9, 2026

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!

biodranik 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!

mjcarroll commented Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants