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 computation in RawData::unpack #155

Closed
wants to merge 8 commits into from

Conversation

facontidavide
Copy link
Contributor

Hi, I noticed that the method RawData::unpack_vlp16 could be optimized.

  1. calibration.laser_corrections_ is a std::map. This means that the find() operation is relatively expensive and not really necessary for each point. I stored the pointer to the laser correction in a simple array with 16 elements.

  2. Since a point that won't pass the pointInRange(distance) condition is not stored, I do this check as soon as possible to avoid unnecessary computation.

The speed up improvement on my machine is considerable. The CPU usage decreases from 25% to 15% (I have a VLP16).

Cheers

Davide

@JWhitleyWork JWhitleyWork self-assigned this Apr 26, 2018
@JWhitleyWork
Copy link
Contributor

It looks like this could also be done for the other lasers. Would you mind pulling in the latest master updates and modifying RawData::unpack in a similar manner? I will test on several Velodynes at my office once this is done.

@facontidavide
Copy link
Contributor Author

facontidavide commented May 10, 2018

Take a look to f59ea36

Is this what you meant?

I am not able to test it on my side, because I only have the VLP16, so... fingers crossed!

If it works, can you please tell me how much difference in CPU usage you notice on you computer, using an app such as top or htop ?

Cheers

@JWhitleyWork
Copy link
Contributor

Derp. Totally missed that one. Thanks. I'll give it a test as soon as I can get some time on the Velodynes.

@facontidavide
Copy link
Contributor Author

facontidavide commented May 10, 2018

Looking forward for it. With so many velodynes all around the world, imagine how much energy can be saved :P

@facontidavide
Copy link
Contributor Author

for the records. This can be further improve if we don't use std::map in first place, instead of converting from std::map to std::vector in every call of unpack.

Therefore, if you test these changes and it works, please let me know before merging.

@facontidavide
Copy link
Contributor Author

facontidavide commented May 12, 2018

@jlblancoc, you are right.

This commit 799dd49 will solve the source of the problem ;)

Less code and same result.

@facontidavide
Copy link
Contributor Author

@JWhitleyAStuff this is a friendly ping? Did you have the opportunity to test this PR?

@JWhitleyWork
Copy link
Contributor

Sorry, we have not. We have a handful of high-priority projects so this probably won't get tested here until the end of June.

@JWhitleyWork JWhitleyWork requested a review from a team June 6, 2018 21:20
@JWhitleyWork
Copy link
Contributor

@facontidavide - Would you mind resolving the conflicts generated when I pulled in another PR? I'll also go ahead and pull this one in after that. My testing is taking far too long and we'll rely on the community for testing this one if you aren't able to.

@facontidavide
Copy link
Contributor Author

I will be happy to solve the conflict, BUT I do think that the changes should be tested on the hardware by SOMEONE before being merged.

@facontidavide
Copy link
Contributor Author

conflict fixed, but I didn't have the opportunity to test it, because my Velodyne is broken :(

@JWhitleyWork
Copy link
Contributor

@facontidavide - Please see PR #194. This makes it easier for me (and others) to test the changes. Sorry again for the lack of recent updates. I'll see if I can get this moved up in priority.

@facontidavide
Copy link
Contributor Author

ok, if you need any action from my side let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants