Reduce the symbol load caused by feature templates #41
Conversation
chapulina
force-pushed
the
reduce_symbols
branch
from
d18b156 to
346ee84
Compare
|
I don't understand the template stuff. I don't know if @azeey has a better handle on that. It would be nice to add the example program from https://github.com/mxgrey/test_ign_physics to either the |
|
We could add that as a test to this repo, although the point of it is to make the most excruciating compilation time possible. I don't think it would be a good idea to add it to the |
There was a problem hiding this comment.
Looks good to me! The reduction in compile time is ridiculous 🎉 . From what I can tell, this still checks for duplicate Features, but I remember you mentioned that in order to reduce compile times, we have to assume that there are no duplicate Features in a FeatureList. Is that still the case?
Also, I see that the DeterminePlugin template uses ComposePlugin, which looks similar to Aggregate in the way it iterates through Features. Can ComposePlugin be updated to work like the new ExtractAPI? Would that save any more compile time?
| public: template<typename... T> | ||
| class type : public virtual | ||
| Aggregate<Selector, FeatureListT>::template type<T...> { }; | ||
| template <typename FeatureTuple, typename... T> |
There was a problem hiding this comment.
Can the Aggregate template be removed now that ExtractAPI doesn't use it?
There was a problem hiding this comment.
From what I can tell, this still checks for duplicate Features, but I remember you mentioned that in order to reduce compile times, we have to assume that there are no duplicate Features in a FeatureList. Is that still the case?
That is not the case, fortunately! There should be no problem at all with having duplicate features.
Can ComposePlugin be updated to work like the new ExtractAPI? Would that save any more compile time?
I considered that, but I didn't have enough time to really explore it. When I inspect the symbols that get generated, there are relatively few symbols related to ComposePlugin, most likely because all entity types for the same set of features share the same plugin type, so cutting down on ComposePlugin probably won't offer much. It's certainly worth trying if someone has cycles to spare.
Can the Aggregate template be removed now that ExtractAPI doesn't use it?
For some reason I thought Aggregate was still being used somewhere, but I can no longer find wherever I thought it was being used. So sure, I guess we can throw it out now.
|
Can this be backported to ign-physics1? It looks like the compile times there are also getting worse. |
I'm willing to review a pull request; I'm not sure if it will cause a problem with API/ABI |
Reopening the pull request with @mxgrey 's work.
Resolves #28.
Original description
This PR should greatly reduce the size and quantity of symbols that get generated by templates, likely by some orders of magnitude. It should also help make the size and quantity of symbols scale much much better (should be nearly constant [size] and linear [quantity] respectively now instead of linear [size] and squared [quantity]).
I believe this has been the leading cause of slow compilation times, especially for GCC.
I made a simple program here that instantiates every entity type with every existing feature in ign-physics, and using all headers provided by ign-physics. Before this PR, using gcc-8 it spent over an hour trying to compile on my XPS15 with no success. After this PR, using gcc-8 it compiles in just under 30 seconds. Clang is significantly better, compiling in 11 seconds after this PR.
There is still some cleanup to do, but I wanted to open this PR so the compilation times of ign-gazebo can be tested against it.