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

Speed up improvement in LaserScan and PointCloud2 #525

Merged
merged 4 commits into from
Nov 7, 2017

Conversation

facontidavide
Copy link
Contributor

Hi,

it is possible to reduce dramatically the memory allocation in both std::vector::reserve or recycling instances popped from the scan_ buffers.

In my benchmarks, these change roughly reduce the amount of CPU used by these two plugins to 1/3rd.

@facontidavide facontidavide changed the title Laser scan Speed up improvement in LaserScan and PointCloud2 Nov 6, 2017
Copy link
Contributor

@evenator evenator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look like great changes. I haven't tested them yet, but most of them are common sense memory management. I did note a stylistic gripe with your implementation of the trig cache.

Actually, if you wouldn't mind factoring that out into a separate PR, I think that speed up the process for us to merge this in.

void LaserScanPlugin::updatePreComputedTriginometic(const sensor_msgs::LaserScanConstPtr& msg)
{
static size_t prev_size = 0;
static float prev_angle_min = msg->angle_min;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid the use of local static variables by refactoring these to class variables or creating a new object to encapsulate the entire trigonometry cache.

Copy link
Contributor Author

@facontidavide facontidavide Nov 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is just a matter of "personal taste". Or is there any technical reason to avoid static?
No problem, though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you point now. You are right

@facontidavide
Copy link
Contributor Author

facontidavide commented Nov 6, 2017

EDIT: I stand corrected.
You might indeed have more than 1 laser plugin running.
I believe it would still be safe in most of the cases, but better to play it safe ;)

Cheers and thanks again for the amazing tool.

@facontidavide
Copy link
Contributor Author

For the records:

  1. using std::vector::reserve or std::vector::resize is always good.
  2. In laserscan, the main performance killer was mostly the calculation of cos() and sin().
  3. the object LaserScanPlugin::Scan is relatively fast to copy in comparison to PointCloud2Plugin::Scan, which is pretty slow.
  4. The worst in terms of performance in PointCloud2Plugin was that ANY single point required a memory allocation because of PointCloud2Plugin::StampedPoint::features. Recycling an already allocated instance avoid that.

@pjreed
Copy link
Contributor

pjreed commented Nov 7, 2017

This looks good to me. Tested and works in my Kinetic Docker environment. Thanks!

@pjreed pjreed merged commit cdd1b06 into swri-robotics:kinetic-devel Nov 7, 2017
pjreed pushed a commit to pjreed/mapviz that referenced this pull request Nov 7, 2017
* improve speed of laserScanCallback (2.5X faster)
* improve speed of pointcloud2 plugin (3X faster)
evenator pushed a commit that referenced this pull request Nov 9, 2017
* improve speed of laserScanCallback (2.5X faster)
* improve speed of pointcloud2 plugin (3X faster)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants