Transition to standard smart pointers

[taketwo]

Now that PCL compiles with C++14 by default, it's high time to make a transition from Boost to standard smart pointers. This transition should not break source code level compatibility with previous releases. In other words, users should be able to compile their downstream projects with both PCL 1.9 and PCL master.

Grepping for shared_ptr in PCL code base I find well over a thousand matches. We can divide these in two groups: where it is a part of public API and where it is not. For the second group the transition is straightforward. For the first group... it depends. As far as I can see, not all shared pointers in API are hidden behind typedefs. This is bad. We will need to think about it and come up with a good strategy how to smoothly convert these.

In the meantime, as a first step I propose to take care of the non-API shared pointers. I think it's not purely mechanical change and we'd always need to carefully double check the context. So I see this as a series of medium-sized PRs. This way it's a) not as daunting a task to do actual changes b) and to review; c) plus we'll have fewer conflicts with other PRs, especially the modernization ones that tend to produce large scattered diffs.

I can (slowly) start with this. @SunBlack your help would surely be appreciated if you are interested. We'll need to orchestrate our efforts to avoid conflicts and double work.

[SergioRAgostinho]

As far as I can see, not all shared pointers in API are hidden behind typedefs. This is bad. We will need to think about it and come up with a good strategy how to smoothly convert these.

Other than lack of consistency and ease of migration, I'm failing to grasp the motive behind your increased concern. What's the failure case you have in mind?

Other than that the plans sounds good.

[SunBlack]

I can (slowly) start with this. @SunBlack your help would surely be appreciated if you are interested. We'll need to orchestrate our efforts to avoid conflicts and double work.

I still don't reached my target to cleanup code this far, that I can cleanup CMake code. So maybe I will replace only small parts of boost, but not the big things.

In general it could be hard to be fully compatible to 1.9.1.
Example: Try to replace boost::mt19937 with std::mt19937. In most cases no one will notice. But sometimes this class is used header files, so in case someone is inherits from a class like this, ... And typedef is not enough, because usage is a bit other in std than in boost (variate_generator was only part of tr1, but not of final). And I don't know if it is worth to have hundreds of #ifdef in code just to handle this.
Maybe it is a compromise to say: Signature of public function should be not be changed (just CMake flag for std vs boost), but protected function and members can be changed without a deprecation release.

[taketwo]

Other than lack of consistency and ease of migration, I'm failing to grasp the motive behind your increased concern. What's the failure case you have in mind?

No, I don't have other concerns besides migration. So by "this is bad" I rather meant "this is bad when it comes to migration". Consider this public API method:

pcl/surface/include/pcl/surface/surfel_smoothing.h

Lines 96 to 99 in 99a5b28

	void
	extractSalientFeaturesBetweenScales (PointCloudInPtr &cloud2,
	NormalCloudPtr &cloud2_normals,
	boost::shared_ptr<std::vector<int> > &output_features);

It uses boost::shared_ptr explicitly. Somewhere in downstream project we will find the following usage:

boost::shared_ptr<std::vector<int>> output_features(new std::vector<int>);
obj.extractSalientFeaturesBetweenScales(cloud, normals, output_features);

Now suppose we switch to std. The user will need to rewrite his code as well. Result: it's not possible to build user project against new and old PCL without ugly #if/#else guards. This is something I absolutely want to avoid.

I still don't reached my target to cleanup code this far, that I can cleanup CMake code.

I was not talking about CMake here, only code.

Example: Try to replace boost::mt19937 with std::mt19937.

Also not other Boost classes. Let's keep this issue focused on smart pointers.

[SergioRAgostinho]

Thanks for the clarification.

For cases like the one you mentioned. I would opt to simply do the breaking transition without a deprecation cycle because I really do not want to have if/else guards all around.

[kunaltyagi]

Will there be future installments or is this task complete for 1.10 checklist?

[taketwo]

This task is by no means complete, but I'm currently struggling to find time to finish this. I'll try to have another look at it and put together a list of remaining work items.

[SergioRAgostinho]

Most private replacements are now done. I haven't gone through the examples/tools/apps yet. I suspect that most uses of shared pointers in those apps are to interface with our public API.

Hence I would propose to start going through the public API and start making the replacements. Turns out I was wrong. There are still valid replacements to be performed in examples/apps/tools.

[kunaltyagi]

I wanted to ask if it makes sense to add another level of indirection in the smart pointer names. We are already using Class::VariablePtr as first layer which points to another class's name alias or directly to boost::shared_ptr. If both the possibilities were indirect, we could have a cleaner switch.

Proposal

Create template <class T> using pcl::shared_ptr = boost::shared_ptr<T>; which makes it easier to finally switch from boost to std (and for people to distribute PCL by applying a single line patch)

[taketwo]

I understand the idea, but I don't get what are the benefits. With the current plan we'll eventually have a commit which replaces boost::shared_ptr with std::shared_ptr in some 1000 lines of code scattered around the library. With your proposal we'll have two commits, first replaces boost::shared_ptr with pcl::shared_ptr in some 1000 lines of code scattered around the library, and the second one changing what pcl::shared_ptr is aliased to. So I don't see haw it makes the transition easier. Is there any benefits from end-user perspective that I overlook?

[deni64k]

I would rather propose to switch to more meaningful and flexible semantics.
There is no need in shared_ptr and its overhead it brings when there's always one owner.
For instance, PCLSurfaceBase::tree_ could have been a reference or a non-owning pointer. It doesn't make sense to destroy the tree before you have done all computation depending on it.

There's a huge section in C++ Core Guidelines dedicated to smart pointers.
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rsmart-smart-pointers

Take also a look at:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r3-a-raw-pointer-a-t-is-non-owning

The problem is by using shared_ptr you force all users to use it when they actually don't need it. Note, shared_ptr brings an overhead: atomics and a reference counter. Atomics are bad for CPU cache, especially for multi-threaded applications. The reference counter requires an extra allocation on the heap.
One more problem, I see a lot of bad examples of initialization of the pointers: instead of using naked new it is better to use make_shared since it saves one allocation and stores the counter next to an object.

[kunaltyagi]

So I don't see haw it makes the transition easier. Is there any benefits from end-user perspective that I overlook?

I assumed having only one shared_ptr would help in providing one consistent class for shared_ptr. That one line can also be patched and then pcl redistributed to work in other environments. We already have a pcl::make_shared so it made sense to me that pcl::shared_ptr is returned by pcl::make_shared.

In future, this approach also allows customization points such as dense_hashmap instead of std::map for faster access (which suits my use-case but not common) [Sidenote: I started with replacement in one class, then 2. Sometime later I had to replace in 3rd and I gave up and just replaced all std::map because it was easier than to deal with errors. Got ~5% faster overall speed for negligible memory increases].

Eigen has some nice compile time customization points that I like (but don't make sense for PCL, just giving a comparison)

For the future: If someone wants to use PCL with custom internals (say boost pointers), they can supply an otherwise generated file say pcl_config.h (or macros) to choose their flavor. Switching becomes as easy as supplying a header file and adding some link libraries in CMake (or marcos and linker command to cmake incovation).

I would rather propose to switch to more meaningful and flexible semantics.

@deni64k I don't think it's possible before the 1.10 release. You should start an independent discussion regarding those changes being accepted after this release for PCL 1.11 or for PCL 2.0 (PCL 2 is essentially rewritten PCL with modern semantics for an API). Time is rather scarce right now.

[SergioRAgostinho]

We already have a pcl::make_shared so it made sense to me that pcl::shared_ptr is returned by pcl::make_shared.

pcl::make_shared role is to lift the responsibility from users of knowing when to use make_shared vs allocate_shared. We had problems with this in the past and this new construct was created to solve that specific issue.

[SergioRAgostinho]

A follow-up from ethz-asl/pcl_catkin#20 (comment)

ROS community is perhaps the most important user of PCL. As it stands, we have broken the compilation of their perception_pcl package without providing a sane migration path that supports releasing the said package compiled against multiple PCL versions.

Our goal with the migration is to preserve source-level compatibility. In the ideal case, this would mean that a downstream project should be compilable against old and new PCL versions without any changes. Taking into account that this may not be achievable in practice, our relaxed goal is to at least provide a painless migration path for downstream projects that keep them source-compatible with multiple PCL versions.

(For example, we can say: "Look, you use plain boost::shared_ptr, so you can not compile against master PCL anymore. However, if you replace these plain pointers with Ptr typedefs that have been around for ages, your code will be compatible with both old and new PCL).

I believe this is not possible anymore for MsgFieldMap. This is an alias to a std::vector<FieldMapping> type that doesn't have the Ptr alias. Even if you create it now, it won't exist for versions < 1.10.0.

Edit: I just remembered chat we had in the past about API visibility. I argued that as long as it is usable and visible to the users, they would use it and thus putting things inside pcl::detail was still dangerous. You said something like "users should not rely on implementation details exposed in pcl::detail". And now here we are 😆

[taketwo]

It's true that such an approach won't work. But in my opinion, this does not mean that we should give up on preserving compatibility.

I did not study that code in pcl_ros deeply, but at the first glance it seems like a remnant from the old days when PCL was part of ROS. Just think about it, we have a shared_ptr<MsgFieldMap> mapping_ field in our point clouds, but it's not used anywhere in the library! The only use is inside ROS code pertaining to message serialization (to cache the mapping so that it does not have to be recomputed for every message). If you don't know the history, it looks crazy, a fundamental data type in a library has a field that is needed by one particular downstream project.

Ideally, we need to break this knot. But, as usual, there are other priorities at the moment, so it's not gonna happen soon. Now that we know that mapping_ field is an ugly hack, it does not really matter which smart pointer it is using (boost vs. std). There is no point in making a transition there, especially if this transition will break the only client of this hack. Let's just leave it alone.

You said something like "users should not rely on implementation details exposed in pcl::detail". And now here we are 😆

Well, I still stand by this. I would mercilessly break any code that uses pcl::detail functions that we added in the last years. But again, this pcl_ros situation is a very special historical thing. They started to use detail back when there was no "they" and "us", only "we". So, in a sense, they were using their own implementation detail, which is fine.

[SergioRAgostinho]

With #3497 merged I will now start performing changes in the public api.

[taketwo]

Sounds good to me 👍

There is no point in making a transition there, especially if this transition will break the only client of this hack. Let's just leave it alone.

What about this though?

[SergioRAgostinho]

There is no point in making a transition there, especially if this transition will break the only client of this hack. Let's just leave it alone.

I believe the same argument can be made in reverse. If it only breaks one consumer, why are we not simply breaking the API for this one case. I would rather have pcl_ros update its interface based on our exported version and let the project handle this from their side than leaving a boost shared pointer and header in our most used class. Ultimately they are using a method from a pcl::detail namespace which is not considered API stable.

I still believe that API breakage will be inevitable and between doing it in two separate occasions or simply doing it now, I would rather do it now.

That being said. I don't have a strong opinion about this. I would still would like to see proposals for a good deprecation strategy. But if there end up being none, we might as well rupture things now.

[taketwo]

I would rather have pcl_ros update its interface based on our exported version and let the project handle this from their side

I'm just not sure how well-maintained perception_pcl stack is. I want to avoid a situation that we break it and nobody has time to fix it. And then the entire ROS community damns us for breaking something that has been working well for the last 10 years.

[taketwo]

We had a few smart pointers related discussions and updates in various places, I'll try to summarize everything here and outline the next steps. @PointCloudLibrary/maintainers please 👍/👎 this so we can move on with the release as soon as possible.

1. Transition timeline

In #2772 (see #2772 (comment) and ensuing comments) there was a rough consensus that the transition should not happen before 1.10. Once the release is done we'll switch to std pointers.

2. pcl::shared_ptr

Earlier in this thread (see #2792 (comment)) @kunaltyagi proposed to introduce pcl::shared_ptr to make the conversion easier. I'm not fully convinced if this is needed, but don't have a strong opinion after all. PR #3546 is ready; we can merge it.

3. Breakage of (semi-public) API used by pcl_ros

Also earlier in this thread (see #2792 (comment)) we discussed the breakage of API that pcl_ros relies upon. I've sent a PR to that repository that fixes the problem in a backwards-compatible way. Nevertheless, I'd prefer to unbreak the API (i.e. switch back to boost pointer) and immediately deprecate it. Given the new transition timeline and the fact that we are not getting rid of boost pointers anyway, I don't see a reason why we shouldn't to this.