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 Point3D #1064

Merged
merged 6 commits into from
Feb 2, 2023
Merged

Speed up Point3D #1064

merged 6 commits into from
Feb 2, 2023

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Feb 2, 2023

I realized we were doing a bunch of processing related to annotations even when annotations weren't being used. This short-circuits that case which gives us a decent 33% speed-bump on NYU dataset (and any large point-clouds that don't use class-ids).

I did a bit of refactoring to remove the temporary Point-array. As far as I can tell iterating the point array and iterating the struct are "close enough" that it's not worth making the copy and we're just as good re-iterating the structure.

I also did a bit of refactoring to avoid unnecessary default initialization for points and user-data. These are now lazily added at the end if a user didn't add them.

I also patched NYUD demo to include class-ids to make sure this wasn't regressing our nominal case and it looks fine.

Before
image

After
image

Before w/ Class-ids
image

After w/ Class-Ids
image

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I've added a line to CHANGELOG.md (if this is a big enough change to warrant it)

@jleibs jleibs requested a review from Wumpf February 2, 2023 16:47
@jleibs jleibs marked this pull request as ready for review February 2, 2023 16:47
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

love it!
some cleanup suggestions

crates/re_query/src/entity_view.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/point_cloud_builder.rs Outdated Show resolved Hide resolved
crates/re_renderer/src/point_cloud_builder.rs Outdated Show resolved Hide resolved
) -> Result<(Vec<ResolvedAnnotationInfo>, Keypoints), QueryError> {
crate::profile_function!();

let mut keypoints: Keypoints = HashMap::new();

// No need to process annotations if we don't have keypoints or class-ids
if !entity_view.has_component::<KeypointId>() && !entity_view.has_component::<ClassId>() {
Copy link
Member

Choose a reason for hiding this comment

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

that makes soooo much sense, we should have that sort of thing everywhere!
Looking forward to making this a reusable pattern of sorts

Comment on lines 168 to 172
pub fn add_points(
&mut self,
count: usize,
positions: impl Iterator<Item = glam::Vec3>,
) -> PointsBuilder<'_, PerPointUserData> {
Copy link
Member

Choose a reason for hiding this comment

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

This needs a docstring now. Does it only add the first count number of positions?

Can we use ExactSizeIterator instead? When we can just call positions.len()!

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like using size_hint() from the standard iterator is a better option. This won't preclude us from using in-exact iterators, but will get us effectively the same benefit we have here.

Copy link
Member Author

Choose a reason for hiding this comment

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

sigh looks like arrow2-convert doesn't implement size-hints, and adding a wrapper and doing the book-keeping seems like it potentially adds more overhead than we're saving. Going to add a docstring for now and came back to it later.

@jleibs jleibs merged commit 2c97139 into main Feb 2, 2023
@jleibs jleibs deleted the jleibs/fast_points branch February 2, 2023 22:31
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

great improvements on top

@emilk emilk mentioned this pull request Feb 4, 2023
12 tasks
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