Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Header #include Knots #5309

Merged
merged 14 commits into from
Aug 26, 2023

Conversation

sturnclaw
Copy link
Member

@sturnclaw sturnclaw commented Nov 22, 2021

This PR lowers compilation time by 15-20% on my development system by simply re-arranging our include files and not relying so heavily on the monolithic libs.h. As a result, the amount of time spent just textually including and parsing libs.h in a full rebuild has dropped by by over 75%.

@sturnclaw
Copy link
Member Author

And yes, I borked the MSVC build myself, need to refactor the name of the new low-level math file...

@sturnclaw
Copy link
Member Author

sturnclaw commented May 13, 2022

Update:
I've not fully split utils.h and libs.h at this juncture, but I have made a significant reduction in the number of files unnecessarily including the latter. I'm happy with the compilation performance improvements at this juncture, and further changes should likely be coupled with a rearchitecture of utils.h and a separation of concerns between the logging functions and string comparison/manipulation functions. I'll leave that for a separate PR.

At this point I think this PR is ready for merge (once MSVC issues have been resolved). Feel free to review the PR, but there's very little substance to the changes other than reordering/refactoring #include directives.

EDIT: I will not be resolving the clang-format failures, as they're mostly spurious whitespace issues not introduced by this PR.

@sturnclaw
Copy link
Member Author

I've updated this PR to current master and made further improvements. All numbers reported are compiled on a Ryzen 7 5800X.

Current master (68s wall-clock time):

**** Time summary:
Compilation (647 times):
  Parsing (frontend):          536.9 s
  Codegen & opts (backend):    330.0 s

Final result (46s wall-clock time):

**** Time summary:
Compilation (645 times):
  Parsing (frontend):          360.8 s
  Codegen & opts (backend):    189.2 s

Most of the codegen time improvement (~100s) came from removing the fmt::sprintf() call in the old Output functions and moving that inside a single compilation unit. The remainder of the performance improvement was reducing the number of transitory #includes of unneeded header files.

In total, this PR represents a 33% compilation time savings on my machine when compiled with 16 workers. The effect on CI builds will most likely be less, as they're comparatively less disk-bound and more cpu-bound.

@sturnclaw
Copy link
Member Author

Additionally, it looks like CI compilation time went from ~16m to ~11m for Clang, and ~23m to ~16m for GCC.

Seriously optimize our usage of large headers with significant compile time penalties.
- Removed SDL headers from libs.h (significant savings from this alone).
- Used forward-declares for Input and Pigui in GuiApplication
- Removed many uses of libs.h from header files that were being chain-included.
- Removed all uses of Json.h from header files; JsonUtils remains for use in CPP files
- Removed several unnecessary uses of libs.h from cpp files.

This has significantly reduced the time cost involved in parsing libs.h
and reduced the overall compile time of the project. Most cpp files as a
consequence have shifted towards an include-what-you-use structure,
often with only one or two includes required to replace libs.h.
- Saves ~30% overall codegen time across the entire project.
- Avoids instantiating fmt::vsprintf in every compilation unit that uses Output etc.
- Most uses of utils.h were just for logging functionality
- String formatting functions are in core/StringUtils.h
- Double-to-string munging functions moved to JsonUtils pending removal
- isqrt etc. were moved to MathUtil
- NOMINMAX is globally defined when compiling under MSVC
- utils.h now only includes a few core headers
- Generally reduce the usage of utils.h and replace with Log.h and MathUtil.h as needed
The peak of the great de-libbening; most files are now include-what-you-use.
- Fix MSVC build missing <cinttypes>
- Fix MSVC missing strncasecmp
- Ensure cmath is included for MSVC builds
@sturnclaw sturnclaw merged commit fb1c588 into pioneerspacesim:master Aug 26, 2023
4 of 5 checks passed
@sturnclaw sturnclaw deleted the header-spaghetti branch August 26, 2023 04:40
@impaktor
Copy link
Member

In total, this PR represents a 33% compilation time savings on my machine when compiled with 16 workers

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants