WIP: Split common/include/pcl/impl/point_types.hpp

[sumir0]

• Move pcl::Array* into common/include/pcl/array_types.h
• Move pcl::Vector* into common/include/pcl/vector_types.h

Work in progress. Feedback is appreciated. Will see what else can be split. I propose not to merge these small changes just yet, assuming I understood the issue correctly, because multiple merge requests will require multiple recompilations of all other modules that use common/include/pcl/impl/point_types.hpp which I think is used in many places.

[mvieth]

Thank you for your pull request. Before you work further on this, let's discuss first for which parts of the code it makes the most sense to put it into different files. A few months ago, I have also looked into how point_types.hpp could be sensibly split, and I found that some things that seemed obvious were not so beneficial after all.
In my opinion, the biggest issue with point_types.hpp, when included in a cpp file, is that it takes so long to compile (roughly one second or even longer, multiplied with the hundreds of times it is included within PCL). The pure number of source code lines would be less important to me. I think, if we would just split point_types.hpp into many small separate files without analysing the effects, this would just make things more complex and have little benefit.

I believe the main question is: looking at all the places within PCL where point_types.hpp is included (or point_types.h, which in turn includes point_types.hpp), what exactly is needed from point_types.hpp? And can we put some code from point_types.hpp into a separate header file and include that one instead, hopefully leading to faster compilation? (with the secondary benefit of making point_types.hpp smaller)

In my opinion, it would currently make the most sense to split off the code at the end of point_types.hpp, that deals with testing for specific point fields (has_field, has_xyz, HasXYZ, etc). We could put that into a different file (point_has_field.h for example, or feel free to suggest a different name), making sure that it is possible to include that file without also including point_types.hpp.

Another idea might be to put all point types with xyz fields into a separate file. This would exclude feature/descriptor types, and cpp files that only need all point types with xyz fields (very often the case) could instead include the new file.

Another thing we could look into, to make compilation faster, is precompiled headers, but that is a different topic.

[sumir0]

Thank you for your reply! I believe your propositions are well-grounded. I will look into it, maybe I could come up with something even better.

[sumir0]

So far, I have several alternatives. Though, I have not looked into all files (and usages) yet. I don't think we can choose one (without guessing) in terms of faster compilation time without actually implementing all of them and making comparison (which I could end up doing...).

Alternative 1. One file - one (struct|macro|etc.) definition where possible or some kind of it.

Example: (imagine) common/include/pcl/impl/descriptors/pfh_signature_125.hpp (maybe different namings if necessary)

/* Licence is skipped for brevity */

#pragma once

#include <pcl/descriptors/pfh_signature_125.h>
#include <pcl/descriptor_size.h> /// for descriptorSize_v
/* other necessary includes */

namespace pcl
{
  namespace detail
  {
    namespace traits
    {
      template<> struct descriptorSize<PFHSignature125> { static constexpr const int value = 125; };
    } // namespace traits
  } // namespace detail

  PCL_EXPORTS std::ostream& operator << (std::ostream& os, const PFHSignature125& p);
  /** \brief A point structure representing the Point Feature Histogram (PFH).
    * \ingroup common
    */
  struct PFHSignature125
  {
    float histogram[125] = {0.f};
    static constexpr int descriptorSize () { return detail::traits::descriptorSize_v<PFHSignature125>; }

    inline constexpr PFHSignature125 () = default;

    friend std::ostream& operator << (std::ostream& os, const PFHSignature125& p);
  };
} // namespace pcl

POINT_CLOUD_REGISTER_POINT_STRUCT (pcl::PFHSignature125,
    (float[125], histogram, pfh)
)

We can have a lot of small header files (probably, put them in subdirectories).

Pros:

• we can reduce unnecessary imports to large extent in translation units (making compilation times faster)
• users of the library can import only necessary definitions (making their compilation times, hopefully, faster)

Cons:

• compiler should handle more /* licence */s, #pragma onces, namespace {s, } // namespaces (making compilation times, probably, slower)

I don't know (yet) what is slower (probably, some experiments are needed):

• handling of more definitions in one file or
• handling of more /* licence */s, #pragma onces, namespace {s, } // namespaces

Alternative 2. More larger files.

Pros:

• we can reduce imports to some extent in translation units (making compilation times less faster)
• compiler should handle less /* licence */s, #pragma onces, namespace {s, } // namespaces (making compilation times, probably, less slower)

Cons:

• users of the library sometimes, probably, have to import unnecessary definitions (making their compilation times, probably, slower)

Alternative 3. Mixing alternatives 1 and 2.

For some definitions use alternative 1 and for other defintions use alternative 2.

Proposal

Based on @mvieth's ideas and my investigation so far, I propose:

• move (h|H)as_* traits into common/include/pcl/(field_traits.h|impl/field_traits.hpp) (alternative 2)
• move feature/descriptor types into separate common/include/pcl/features/(${feature_or_descriptor_name}.h|impl/${feature_or_descriptor_name}.hpp) files (alternative 1), for example: PPFSignature is used in the following files without much of other things from point_types.hpp
  • common/include/pcl/common/point_tests.h
  • common/include/pcl/point_representation.h
  • features/src/ppf.cpp
  • registration/include/pcl/registration/ppf_registration.h
  • registration/src/ppf_registration.cpp
  • test/features/test_ppf_estimation.cpp
  • test_registration/test_registration.cpp
• leave other things in common/include/pcl/(point_types.h|impl/point_types.hpp) for now
• import all moved declarations/definitions in common/include/pcl/(point_types.h|impl/point_types.hpp) for backward-compatibility

Suggestions are welcome.

Questions I left unanswered for now:

• should feature/descriptor types be in features/include/pcl/features/...
• is there any point splitting point types (xyz, rgb, etc.) into small files (I haven't looked into their usage good enough)
• if we include point_types.h in ~375 files and every include costs us ~1-2 second, does it mean we potentially could reduce compilation time by 374-748 seconds (~6-12 minutes) out of total 1-2 hours of compilation (still not bad, though)

[mvieth]

move (h|H)as_* traits into common/include/pcl/(field_traits.h|impl/field_traits.hpp) (alternative 2)

Sounds good. In my opinion, the division into .h and .hpp files do not have to be strictly followed for new files, if the .h files only contain the struct declaration and the include for the .hpp file -- in that case, I am fine with putting everything into the .h file.

move feature/descriptor types into separate common/include/pcl/features/(${feature_or_descriptor_name}.h|impl/${feature_or_descriptor_name}.hpp) files (alternative 1)

I am okay with that, but I would also be fine with putting all "feature types" into one .h file. Should not make a big difference compile-time wise, but could make handling PCL_FEATURE_POINT_TYPES, descriptorSize, and descriptorSize_v easier.

import all moved declarations/definitions in common/include/pcl/(point_types.h|impl/point_types.hpp) for backward-compatibility

Yes, that is a good point.

should feature/descriptor types be in features/include/pcl/features/...

I think I would keep them in common because some feature/descriptor types are also used by other PCL modules, e.g. registration.

is there any point splitting point types (xyz, rgb, etc.) into small files (I haven't looked into their usage good enough)

I think, eventually, it would be good to move all point types that contain x, y, z fields to their own (single) .h file (e.g. xyz_point_types.h). These point types are the most widely used types in PCL, and it would be beneficial to have one file containing only those, and no other feature types, (h|H)as_* structs etc.

if we include point_types.h in ~375 files and every include costs us ~1-2 second, does it mean we potentially could reduce compilation time by 374-748 seconds (~6-12 minutes) out of total 1-2 hours of compilation (still not bad, though)

Well, depends on how much time reduction per include of point_types.h we can achieve in the end. One things that is rather costly is the <Eigen/Core> include, and we cannot completely remove that due its usage in PCL_ADD_POINT4D and PCL_ADD_EIGEN_MAPS_POINT4D. My guess would be that a reduction of up to 5 minutes can be achieved alone by splitting point_types.h and replacing includes of point_types.h, and another 5 minutes by introducing precompiled headers (and reusing the precompiled headers while compiling PCL).

Please make sure to not put all changes into one big pull request, but to create 2-3 smaller ones that we can merge individually.