ros / roscpp_core

ros distribution sandbox
88 stars 116 forks source link

Avoid unnecessary memory allocation in Header::parse #95

Closed facontidavide closed 5 years ago

facontidavide commented 5 years ago

Hi,

this is the first time I do a PR to ROS core, therefore I am not sure about the correct "process".

I noticed performance can be improved avoiding the creation of std:.string in Header::parse. This brings a noticeable performance improvement in applications such a rosbag parsing.

dirk-thomas commented 5 years ago

this is the first time I do a PR to ROS core, therefore I am not sure about the correct "process".

The "process" looks good. The CI build seem to have failed due to some Jenkins problem. @ros-pull-request-builder retest this please

This brings a noticeable performance improvement in applications such a rosbag parsing.

Can you share steps you used to compare the performance before and after?

facontidavide commented 5 years ago

Of course it is a small change , so fon't expect huge performance improvements:

This is the test code:

#include <ros/ros.h>
#include <rosbag/bag.h>
#include <rosbag/view.h>

int main()
{
    rosbag::Bag bag;
    bag.open( "./test.bag" );
    for (int i=0; i<10; i++)
    {
        size_t msg_size = 0;
        rosbag::View bag_view ( bag );
        for(rosbag::MessageInstance msg_instance: bag_view)
        {
           msg_size += msg_instance.size();
        }
        std::cout << msg_size << std::endl;
    }
    return 0;
}

Apparently it is only a 1% improvement, but in terms of CPU it is actually more, because the UncompressedStream part is IO bound, not CPU bound, whilst Header::parse is CPU bound.

Before

perf data - hotspot_162

After

perf data - hotspot_163

facontidavide commented 5 years ago

For your information, I noticed this whilst profiling a real-world application (PlotJugger) and I got there about 3% improvement.

dirk-thomas commented 5 years ago

Thanks for this improvement.