Explicit template declaration and instantiation for scalar type

[jslee02]

This pull request adds explicit template instantiations in order to reduce the increased build time due to the templatizing (#154) as proposed by #160.

The basic idea is to instantiate template objects (class or function) in source files for scalar types that are most likely used by the user (e.g., double and float) so that the compiler create binaries for the template objects for the scalar types. This reduces compilation times because the compiler doesn't need to generate code for those instantiated template objects.

For this work, I followed the nice guide, which was written for Drake, but ran into one issue. In the example of the guide, all the object in the header is templatized for single parameter T, but many of FCL header have objects templatized for the mix of scalar type, shape type, narrowphase solver type. In this case, we should include the -inl.h file at the end of the header so that the compiler can see the definitions in -inl.h for objects are not explicitly instantiated in the source file.

// -- MyClass.h
template <typename S>
class MyClassA {
  void foo();
};

template <typename Shape>
class MyClassB {
  void bar();
};

// -- MyClass-inl.h
// definitions of MyClassA and MyClassB

// -- MyClass.cpp

// explicit template instantiation
template  // note that no brackets here
class MyClassA<double>;

However, the problem of doing this was that the compiler generates codes even for MyClassA<double>. Thanks to C++11, we have a simple solution for this issue: explicit instantiation declaration ((2) in Explicit instantiation section of here). Now the -inl.h file is modified as:

// -- MyClass-inl.h

// explicit template declaration
extern template
class MyClassA<double>;

// definitions of MyClassA and MyClassB

It seems the explicit template declaration can be placed at the end of the header (the header includes -inl.h file anyways), but I prefer to hide from header because I think it's unnecessary info in reading the header to understand the API.

One bonus of using explicit template declaration is that we don't need to include -inl.h even when we instantiate the template objects for own types (again, because the headers include -inl.h files anyways).

In this PR, the template objects are instantiated only for double. One open question is if it's worth to instantiate for float type as well. I'm inclined not to instantiate for float. A downside of having both would be the increased size of binary. We could consider having two binaries for double and float, but it would be an overkill.

Note that this pull request should be merged after #163.

[chhtz]

I'm not sure if I misunderstand something here.
It seems you are including each -inl.h header from the corresponding .h header. If I understand it correctly, the idea was not to include them (by default) but only instantiate the required template classes -- otherwise this will not be (much) different than leaving the implementation directly in the .h files.

[mamoll]

Like @chhtz, I am also confused by this.

[jslee02]

You are correct. It's technically the same with having the implementation directly in the .h files. At first, I intended not to include -inl.h header from the corresponding .h header as this guide, but I split them into .h and -inl.h for code readability. I prefer making headers as clean as possible. We could have the definitions (1) at the bottom of the corresponding header like this or even (2) in the class declaration. I'm open to this issue, but I would vote for having the implementations in separate files.

Edit: For clarification, these are the main changes of this pull request:

• moving implementations of template classes to -inl.h files for code readability (this is arguable)
• explicitly instantiating template classes whose template parameter is S (scalar type) in the corresponding .cpp files for faster build

[chhtz]

Ok, as it is, it does add to code readability and I did not intend to suggest to move the implementation back into the .h file. I'm rather suggesting not to include the -inl.h files (or guard the includes by something like #ifdef FCL_INCLUDE_INL). But of course, I'm ok if you prefer doing this as a separate step.

[jslee02]

I'm rather suggesting not to include the -inl.h files (or guard the includes by something like #ifdef FCL_INCLUDE_INL).

Could I ask what would be the benefits of doing so? I think it could reduce compilation time but don't think that's significant. Am I missing something here?

[sherm1]

@jslee02: The idea is to take advantage of explicit instantiation for (a) compile time speedup for end users, (b) reduced likelihood of indecipherable compile errors, and (c) information hiding. Assuming there are only a small number of likely combinations of template parameters, we can instantiate those in compiled library code. Normal user code then compiles against the .h files only, containing just declarations but no definitions, but they must link against an FCL library. Advanced user code that wants to instantiate some odd combination of template parameters compiles against the -inl.h files instead. Their compile times go up, but they get unique instantiations and could avoid linking against the library altogether.

To make that work the .h files should not include the -inl.h files automatically, although I like @chhtz's idea of making it conditional on FCL_INCLUDE_INL (maybe FCL_INCLUDE_IMPLEMENTATION?). That would also have the advantage that we could set that true for now and make it false after the instantiations and library are in place.

[jslee02]

To make that work the .h files should not include the -inl.h files automatically

I agree with that the goal of #160 and this pull request is what you pointed out, but I don't see why .h files shouldn't include the -inl.h files to make that work. It still gets compile time speedup thanks to the explicit template declaration. Plus, the user doesn't need to care about implementation headers at all.

[sherm1]

I don't see why .h files shouldn't include the -inl.h files to make that work. It still gets compile time speedup thanks to the explicit template declaration.

Oh, is that true? I would have thought making the compiler eat those definitions would be time consuming.

[chhtz]

You are right, sorry for the noise. I was not aware of the extern template class syntax.

[jslee02]

Yeah, I learned that while working on this. extern template class syntax was introduced in C++11.

Oh, is that true? I would have thought making the compiler eat those definitions would be time consuming.

It might not be as fast as we completely don't include the implementation headers, but they're pretty close in build time because of the extern template class (explicit template declaration). That keyword prevents from generating object files for the template objects that are already instantiated in the .cpp files. I actually didn't see the differences in build time for them.

[sherm1]

@jwnimmer-tri, possible have-cake-and-eat-it-too option re -inl.h? Use extern template class in header to prevent instantiation, but include -inl.h at the end of the .h. @jslee02 says he doesn't notice a difference in compile time vs. not including the inl.h at all. Requires committing to particular instantiations in the header files, but means non-standard instantiations work with no prep. (See last few comments of thread above.)

[jslee02]

I believe this PR is ready to merge.

[sherm1]

@jslee02: looks like build failed for 64 bit Windows Release build. Is that expected?

[jslee02]

It seems AppVeyor just failed for some reason. It passed before the last commit that I don't believe it caused the build error. It would be good to retrigger the test for sure, but I don't have access for it.

[sherm1]

Merging would trigger and could be repaired afterwards if there is a problem. (Or you could push a trivial commit.)

[jslee02]

Okay. I'll choose the first option. Merging now then.