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

Improve speed of msgify(point_cloud_2_np) by 115x #3

Merged
merged 3 commits into from
Apr 15, 2022

Conversation

apockill
Copy link

@apockill apockill commented Mar 10, 2022

I noticed that on large point clouds, converting from numpy to PointCloud2 was extremely time consuming. It took 2.4 seconds to convert a point cloud representing (x. y, z, rgb) for a structured array of shape (1944, 1200). This cloud is about 40 megabytes when serialized.

Background

As I looked into it, I found that the speed problem was not actually in cloud_arr.tostring(), but rather inside of the PointCloud2.data property, when it calls self._data = array.array('B', value).

Reference code:

class PointCloud2:
	...

	@data.setter
    def data(self, value):
        if isinstance(value, array.array):
            assert value.typecode == 'B', \
                "The 'data' array.array() must have the type code of 'B'"
            self._data = value
            return
        self._data = array.array('B', value)

From looking into it, it appears that the entire byte array is iterated upon painstakingly and copied by array.array('B', value),

To resolve this, I used a memoryview and array.array.frombytes() to reduce the copying and iteration. In fact, the only copy is here, which uses memcpy and is quite efficient- as opposed to currently where it iterates over each byte as the array.array object is built.

Benchmarks

Current Implementation:
msgify(large_point_cloud): 2.46 seconds

This PR:
msgify(large_point_cloud): 0.0211 seconds

@apockill apockill changed the base branch from master to rolling March 10, 2022 00:14
@velovix
Copy link

velovix commented Apr 14, 2022

@tpanzarella Would you mind taking a look at this PR? It would be super helpful to have this in 😁

@tpanzarella
Copy link
Member

Yes. For some reason I was not getting alerts about the PRs to the repo. I should be able to take a look today. Sorry for the delays. The premise of the PR sounds great.

@tpanzarella tpanzarella merged commit fd7a877 into Box-Robotics:rolling Apr 15, 2022
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