Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid unnecessary memory allocation in Header::parse #95

Merged
merged 1 commit into from Oct 31, 2018

Conversation

@facontidavide
Copy link
Contributor

facontidavide commented Oct 30, 2018

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

This comment has been minimized.

Copy link
Member

dirk-thomas commented Oct 30, 2018

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

This comment has been minimized.

Copy link
Contributor Author

facontidavide commented Oct 30, 2018

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

This comment has been minimized.

Copy link
Contributor Author

facontidavide commented Oct 30, 2018

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

@dirk-thomas

This comment has been minimized.

Copy link
Member

dirk-thomas commented Oct 31, 2018

Thanks for this improvement.

@dirk-thomas dirk-thomas merged commit 53fbad3 into ros:kinetic-devel Oct 31, 2018
3 checks passed
3 checks passed
Kpr__roscpp_core__ubuntu_xenial_amd64 Build finished.
Details
Lpr__roscpp_core__ubuntu_xenial_amd64 Build finished.
Details
Mpr__roscpp_core__ubuntu_bionic_amd64 Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.