Random "header hygiene" cleanups #1880

mosra · 2022-09-30T15:37:54Z

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 · 2022-09-30T16:18:05Z

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

mosra · 2022-09-30T16:21:40Z

/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 · 2022-09-30T17:05:09Z

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!

Those have a hardcoded assumption that any mesh is a GL mesh, and that any material is a Phong material. Which is kinda true now, but it won't be going forward. Moreover, they weren't really used anywhere anyway, so there's no point in having them at all.

The magnum.h header now contains SceneGraph forward declarations typedefs and only that. It doesn't and shouldn't pull in also GL or Trade stuff.

It's still pulled into far too many places, but this way it can at least start getting gradually removed.

Not exactly sure what is it doing, but it's quite heavy. Unfortunately most of the tests need it to set things up anyway, so it only helps partially. Also, the ::ptr typedefs are nice and all, but they mean one has to include the whole class definition. Which is not necessary in 99% of cases, so I'm converting T::ptr to the verbose std::shared_ptr<T> where a forward declaration is enough.

Wow, imagine changing anything in any of those headers. Boom, everything gets recompiled.

The static will *copy* the string into each and every object file that directly or indirectly includes Asset.h.

I'm starting to run in circles, so let's stop here.

And why do you have custom PLY parsing and writing? And why are your eyes so big?

It only worked so far because <Magnum/configure.h> was not included in Logging.cpp and so MAGNUM_BUILD_STATIC_UNIQUE_GLOBALS wasn't defined. Now it is, and the anonymous namespace is in direct conflict with the export macro.

Looks like there needs to be some Clang Tidy Tidy to ensure sanity of Clang Tidy implementation. Ugh.

Why it wasn't a problem when it was in the header? Was it because you didn't know that the ::ptr type is a std::shared_ptr? That's kinda lame, no?

mosra · 2022-10-06T15:05:24Z

src/esp/assets/ResourceManager.h

@@ -1256,7  1256,7 @@ class ResourceManager {
   * being rendered, since that mesh will have its components moved into actual
   * render mesh constructs.
   */
-  std::unique_ptr<GenericSemanticMeshData> infoSemanticMeshData_{};
+  std::unique_ptr<GenericSemanticMeshData> infoSemanticMeshData_;


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 · 2022-10-06T15:29:21Z

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 · 2022-10-06T17:53:29Z

Nice cleanup!

* Remove unused and wrong Magnum typedefs. Those have a hardcoded assumption that any mesh is a GL mesh, and that any material is a Phong material. Which is kinda true now, but it won't be going forward. Moreover, they weren't really used anywhere anyway, so there's no point in having them at all. * Don't include all of Magnum everywhere for no reason. The magnum.h header now contains SceneGraph forward declarations typedefs and only that. It doesn't and shouldn't pull in also GL or Trade stuff. * Contain Eigen into a separate header, not the main main header. It's still pulled into far too many places, but this way it can at least start getting gradually removed. * Use Magnum's forward declaration headers, not manual declarations. * Don't include MetadataMediator in Simulator.h and ResourceManager.h. Not exactly sure what is it doing, but it's quite heavy. Unfortunately most of the tests need it to set things up anyway, so it only helps partially. Also, the ::ptr typedefs are nice and all, but they mean one has to include the whole class definition. Which is not necessary in 99% of cases, so I'm converting T::ptr to the verbose std::shared_ptr<T> where a forward declaration is enough. * Remove most includes from ResourceManager.h. Wow, imagine changing anything in any of those headers. Boom, everything gets recompiled. * Stop including ResourceManager from all headers. * This is not the right way to create a string constant. The static will *copy* the string into each and every object file that directly or indirectly includes Asset.h. * Disable a useless Clang Tidy check. * Fix definition of the global logging context variable. It only worked so far because <Magnum/configure.h> was not included in Logging.cpp and so MAGNUM_BUILD_STATIC_UNIQUE_GLOBALS wasn't defined. Now it is, and the anonymous namespace is in direct conflict with the export macro.

mosra requested review from jturner65, aclegg3, Skylion007 and eundersander September 30, 2022 15:37

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 30, 2022

mosra force-pushed the header-hygiene branch from 500b95c to aa693c4 Compare September 30, 2022 15:59

aclegg3 approved these changes Sep 30, 2022

View reviewed changes

Skylion007 approved these changes Sep 30, 2022

View reviewed changes

jturner65 approved these changes Sep 30, 2022

View reviewed changes

mosra added 16 commits October 6, 2022 12:36

Don't include all of Magnum everywhere for no reason.

1a4c65c

The magnum.h header now contains SceneGraph forward declarations typedefs and only that. It doesn't and shouldn't pull in also GL or Trade stuff.

Contain Eigen into a separate header, not the main main header.

18da2fc

It's still pulled into far too many places, but this way it can at least start getting gradually removed.

Use Magnum's forward declaration headers, not this.

8e1234e

Remove most includes from ResourceManager.h.

f11f9f3

Wow, imagine changing anything in any of those headers. Boom, everything gets recompiled.

Stop including ResourceManager from all headers.

9a89139

This is not the right way to create a string constant.

8ff306f

The static will *copy* the string into each and every object file that directly or indirectly includes Asset.h.

More header cleanup.

47e0bf9

I'm starting to run in circles, so let's stop here.

Clang Format Yadda Yadda

fbc27eb

Ah yes forgot to build with Bullet.

e56ab05

And the viewer, haha.

83739df

Hello, Eigen.

df2acea

Datatool, why do you need Eigen?

b2ef09b

And why do you have custom PLY parsing and writing? And why are your eyes so big?

Clean up const qualifiers in what's now just a declaration.

34cfb42

Disable a useless Clang Tidy check.

9561252

mosra force-pushed the header-hygiene branch from b5bfeae to 8ede499 Compare October 6, 2022 10:37

Fix definition of the global logging context variable.

9abad19

It only worked so far because <Magnum/configure.h> was not included in Logging.cpp and so MAGNUM_BUILD_STATIC_UNIQUE_GLOBALS wasn't defined. Now it is, and the anonymous namespace is in direct conflict with the export macro.

mosra force-pushed the header-hygiene branch from 8ede499 to 9abad19 Compare October 6, 2022 10:40

mosra added 2 commits October 6, 2022 13:09

Clang Tidy, you don't understand *some* comments in config files??

c4b03a4

Looks like there needs to be some Clang Tidy Tidy to ensure sanity of Clang Tidy implementation. Ugh.

Funny that this .get() is now a problem for Clang Tidy.

cef83aa

Why it wasn't a problem when it was in the header? Was it because you didn't know that the ::ptr type is a std::shared_ptr? That's kinda lame, no?

mosra force-pushed the header-hygiene branch from d6ec53c to d6ae319 Compare October 6, 2022 12:13

Attempt to fix GCC 7 Conda build.

a769184

mosra force-pushed the header-hygiene branch from d6ae319 to a769184 Compare October 6, 2022 14:21

mosra commented Oct 6, 2022

View reviewed changes

mosra merged commit f9d7446 into main Oct 6, 2022

mosra deleted the header-hygiene branch October 6, 2022 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Random "header hygiene" cleanups #1880

Random "header hygiene" cleanups #1880

mosra commented Sep 30, 2022

aclegg3 left a comment

Skylion007 commented Sep 30, 2022

mosra commented Sep 30, 2022 •

edited

Loading

jturner65 commented Sep 30, 2022

jturner65 left a comment

mosra Oct 6, 2022

mosra commented Oct 6, 2022

0mdc commented Oct 6, 2022

Random "header hygiene" cleanups #1880

Random "header hygiene" cleanups #1880

Conversation

mosra commented Sep 30, 2022

Motivation and Context

How Has This Been Tested

aclegg3 left a comment

Choose a reason for hiding this comment

Skylion007 commented Sep 30, 2022

mosra commented Sep 30, 2022 • edited Loading

jturner65 commented Sep 30, 2022

jturner65 left a comment

Choose a reason for hiding this comment

mosra Oct 6, 2022

Choose a reason for hiding this comment

mosra commented Oct 6, 2022

0mdc commented Oct 6, 2022

mosra commented Sep 30, 2022 •

edited

Loading