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

[io] Improve PCD read performance (more than 50%) by reusing istringstream #4339

Merged
merged 4 commits into from
Oct 9, 2020

Conversation

kunaltyagi
Copy link
Member

Single threaded performance increases by approximate 50%

#4333 (comment)

@kunaltyagi kunaltyagi added changelog: enhancement Meta-information for changelog generation needs: code review Specify why not closed/merged yet module: io labels Aug 20, 2020
@kunaltyagi kunaltyagi added this to the pcl-1.12.0 milestone Aug 20, 2020
@kunaltyagi kunaltyagi self-assigned this Aug 20, 2020
@kunaltyagi kunaltyagi changed the title [io] Improve PCD read performance (upto 50%) by reusing istringstream [io] Improve PCD read performance (more than 50%) by reusing istringstream Aug 20, 2020
@kunaltyagi kunaltyagi marked this pull request as draft August 20, 2020 11:17
@kunaltyagi kunaltyagi marked this pull request as ready for review August 20, 2020 11:42
@SergioRAgostinho
Copy link
Member

I'm not going to review this one. I have no progress on it.

@SergioRAgostinho SergioRAgostinho removed their request for review September 7, 2020 07:27
@kunaltyagi kunaltyagi requested review from larshg and mvieth September 20, 2020 16:00
@mvieth
Copy link
Member

mvieth commented Sep 23, 2020

The changes look ok, but I feel that there has to be a nicer way to do all this. What about stod, stof, stoi, ...? Alternatively, maybe the boost:split in pcd_io.cpp can be removed and instead the stream used for the whole line?

@kunaltyagi
Copy link
Member Author

What about stod, stof, stoi, ...?

AFAIK, these functions are locale dependent. In C, the locale is global and changing locale might affect downstream users during loading of a file.

Alternatively, maybe the boost:split in pcd_io.cpp can be removed and instead the stream used for the whole line?

I've marked that issue as help-wanted because I couldn't refactor it cleanly without touching quite a few lines. I too would like to see a cleaner implementation than this low hanging fruit.

Copy link
Contributor

@larshg larshg left a comment

Choose a reason for hiding this comment

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

I agree to @mvieth observations, but as you write @kunaltyagi it can be left for another day :)

50% performance increase, is also quite nice!

detail::copyStringValue<Type> (st, cloud,point_index, field_idx, fields_count, is);
}
template <typename Type> inline void
copyStringValue (const std::string &st, pcl::PCLPointCloud2 &cloud,
Copy link
Member

Choose a reason for hiding this comment

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

This new functions lacks the documentation, especially regarding is. Also: "Uses aoti/atof to do the conversion." isn't really correct anymore.

@kunaltyagi kunaltyagi merged commit 30d6a04 into PointCloudLibrary:master Oct 9, 2020
@kunaltyagi kunaltyagi deleted the pcd_omp branch October 9, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: io needs: code review Specify why not closed/merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants