Skip to content

Fix possible alignement issues on 32 bits CPUs #1405

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 26, 2016
Merged

Fix possible alignement issues on 32 bits CPUs #1405

merged 1 commit into from
Aug 26, 2016

Conversation

VictorLamoine
Copy link
Contributor

#1387 (comment)

apps_cloud_composer still compiles fine.

@taketwo
Copy link
Member

taketwo commented Nov 2, 2015

Hi Victor, thanks for taking care of this. I think that we don't need to get rid of make_shared everywhere. In fact, it's slightly more efficient and the syntax is definitely more succinct. So I'd vote for keeping make_shared where aligned objects are not involved.

@VictorLamoine VictorLamoine changed the title Fix possible alignement issues on 32 bits CPUs [WIP] Fix possible alignement issues on 32 bits CPUs Feb 15, 2016
@SergioRAgostinho
Copy link
Member

Can we integrate this in 1.8.1? I'm asking because of that WIP tag :)

SergioRAgostinho
SergioRAgostinho reviewed Aug 18, 2016
View reviewed changes
apps/cloud_composer/include/pcl/apps/cloud_composer/impl/merge_selection.hpp
filter.filter (*selected_points);

qDebug () << "Original minus indices is "<<original_minus_indices->width;

//Eigen::Vector4f source_origin = input_cloud_item->data (ItemDataRole::ORIGIN).value<Eigen::Vector4f> ();
//Eigen::Quaternionf source_orientation = input_cloud_item->data (ItemDataRole::ORIENTATION).value<Eigen::Quaternionf> ();
//pcl::PCLPointCloud2::Ptr cloud_blob = boost::make_shared <pcl::PCLPointCloud2> ();;
//pcl::PCLPointCloud2::Ptr cloud_blob = boost::shared_ptr <pcl::PCLPointCloud2> (new pcl::PCLPointCloud2);
//toPCLPointCloud2 (*original_minus_indices, *cloud_blob);
//CloudItem* new_cloud_item = new CloudItem (input_cloud_item->text ()
//, cloud_blob
Copy link
Member

@SergioRAgostinho SergioRAgostinho Aug 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't we erase these comments all together?

@SergioRAgostinho
Copy link
Member

In any case double check the spaces before the parenthesis and I would say to erase those comment blocks unless someone opposes.

@SergioRAgostinho SergioRAgostinho added the needs: more work Specify why not closed/merged yet label Aug 22, 2016
@VictorLamoine
Copy link
Contributor Author

VictorLamoine commented Aug 23, 2016 •
edited
Loading

So I'd vote for keeping make_shared where aligned objects are not involved.

How do I know which objects are aligned/not aligned?

@taketwo
Copy link
Member

taketwo commented Aug 23, 2016 •
edited
Loading

Aligned are objects that have Eigen members inside. Looking at the changes in this PR, I think only pcl::PointCloud<T>s are aligned. Everything else is not (note: PCLPointCloud2s are not aligned).

@VictorLamoine VictorLamoine changed the title [WIP] Fix possible alignement issues on 32 bits CPUs Fix possible alignement issues on 32 bits CPUs Aug 23, 2016
@VictorLamoine VictorLamoine added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Aug 23, 2016
@VictorLamoine
Copy link
Contributor Author

Ready for review

@taketwo
Copy link
Member

taketwo commented Aug 26, 2016

👍

@SergioRAgostinho SergioRAgostinho added this to the pcl-1.8.1 milestone Aug 26, 2016
@SergioRAgostinho SergioRAgostinho merged commit a7bcb98 into PointCloudLibrary:master Aug 26, 2016
@SergioRAgostinho
Copy link
Member

Is there a way for us to prevent people from using make_shared with PointCloudT types (overriding?) or will we just need to pay attention to all subsequent PRs?

@SergioRAgostinho SergioRAgostinho removed the needs: code review Specify why not closed/merged yet label Aug 26, 2016
@taketwo
Copy link
Member

taketwo commented Aug 26, 2016

Here is a brief explanation of what is wrong with make_shared and aligned objects (from mailing list):

Don't use make_shared() with objects that need to be aligned. [1]
The longer version is that make_shared() allocates the memory itself,
and then calls "placement new" [2] to initialize the object. By
allocating the memory itself, make_shared() circumvents the "operator
new" that EIGEN_MAKE_ALIGNED_OPERATOR_NEW creates for you. This is
similar to how std::vector works, and why you need to pass it an
allocator when using it with types that must be aligned.
[1] http://lists.boost.org/boost-users/2010/03/57713.php
[2] http://en.wikipedia.org/wiki/Placement_syntax

Now the solution might be to disable or make private the placement new operator, so that make_shared can not call it. Is this possible? No idea.

@VictorLamoine VictorLamoine deleted the make_shared branch August 26, 2016 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
pcl-1.8.1
Development

Successfully merging this pull request may close these issues.
3 participants
@VictorLamoine @taketwo @SergioRAgostinho