Random "header hygiene" cleanups

[mosra]

Motivation and Context

Original message on Slack:

I was switching among many habitat branches recently and noticed a funny thing:

• rebuilding 122 targets via ninja (just what got changed after a git checkout) takes 6 minutes 40 seconds
• while building all of magnum, which is about 1280 targets, takes 2 minutes 5 seconds, so ~30x faster(!)

So I went to improve that a bit. Mostly about using forward declarations where that's enough and deinlining functions that rely on heavy headers. Tested via Clang's -ftime-trace on ReplayBatchRenderer.cpp, top is before and bottom after. (For reference, ReplayBatchRendererTest.cpp.json.zip is the bottom trace JSON, unzip and load e.g. in https://speedscope.app.)

The total time varies depending on system load, so don't pay too much attention to that. What matters is the percentage spent parsing included headers, which is the "Frontend" time minus "Perform pending instantiations". Before it was ~77%, now it's ~64%. Ideally the majority of time would be spent with actual compilation and not redundant header parsing, but I got some improvement at least.

The biggest issue is Eigen, taking about a quarter of all header parsing time. I moved its includes into a dedicated header so it's not included everywhere, yet it's still used in many widely-used structures such as CoordinateFrame or AgentState, so it gets still dragged into basically all test files.

Another significant issue is the ESP_SMART_POINTERS() macro. It provides short and sweet T::ptr, T::cptr and T::uptr typedefs, but the consequence is that they need the full definition of T in order to be used. Where it made a difference, I changed those to std::shared_ptr<T>, which needs just a forward declaration of T, not the full definition.

This is just a start, and there's still a lot of unnecessary dependencies between the headers. But the main ones, like Simulator.h or ResourceManager.h including everything else, are mostly fixed now, so at the very least you could have slightly faster incremental builds due to not having to recompile that much.

I ran out of time I wanted to spend on this, so feel free to enable -ftime-trace yourself and continue :)

How Has This Been Tested

😅

[aclegg3]

LGTM. Thanks for cleaning things up :)

[Skylion007]

@mosra Seems like some of these forward declarations are triggering :( : bugprone-forward-declaration-namespace

[mosra]

/home/circleci/project/habitat-sim/src/deps/magnum/src/Magnum/Trade/Trade.h:100:7: error: no definition found for 'PhongMaterialData', but a definition with the same name 'PhongMaterialData' found in another namespace 'esp::gfx' [bugprone-forward-declaration-namespace,-warnings-as-errors]

Well, how am I supposed to fix this? :D This class is in both (Clang just doesn't know because it sees only the declarations), and there's many similar cases (MeshPrimitive, MeshData, ...). I'd say let's disable this check, it doesn't really prevent any errors.

Alternatively Habitat could have its own forward declaration headers but that won't really help here, I'm afraid.

[jturner65]

Another significant issue is the ESP_SMART_POINTERS() macro. It provides short and sweet T::ptr, T::cptr and T::uptr typedefs, but the consequence is that they need the full definition of T in order to be used. Where it made a difference, I changed those to std::shared_ptr, which needs just a forward declaration of T, not the full definition.

Wow! I had no idea about that.

[jturner65]

LGTM!

[mosra]

This is one of the funnier STL workarounds I came across lately.

Without the {}, the type can be forward-declared and it will work as long as all constructors, assignment operators and destructors are deinlined in the file. With the {} however, a GCC 7 build will attempt to instantiate std::default_delete<GenericSemanticMeshData> each time the header is included anywhere, failing if the type definition is not known.

[mosra]

Okay, I think all CI failures are resolved now, the remaining one is related to #1890.

Let me know if this is okay to merge -- the newly added commits are just minor things, nevertheless they happened after all approvals so I want to be sure ;)

@Skylion007 I ended up disabling the bugprone-forward-declaration-namespace Clang Tidy check if that's alright. See the comment added to .clang-tidy for details.

[0mdc]

Nice cleanup!