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: only include what you use #3398

Closed
wants to merge 3 commits into from
Closed

refactor: only include what you use #3398

wants to merge 3 commits into from

Conversation

memchr
Copy link
Contributor

@memchr memchr commented Sep 22, 2023

Currently, header usage in Hyprland is rather chaotic, some headers have circular dependencies, some headers or source includes have more than they actually need.

This can cause unexpected compiler/linker errors and increase maintenance costs. It also slows down PCH disabled and incremental builds. Since unnecessary files are included, the build system will recompile where it's not necessary.

Another problem is with the development tools, especially the language servers such as clangd, where it reports mysterious errors in headers included in headers included in headers included in headers included in headers...

Thus this PR propose that each source file should contain only and directly what it uses.

Advantages

Reduces incremental build time

For example, suppose we do an initial build with PCH disabled (required for compile caching), then modify KeybindManager.hpp and corresponding .cpp to introduce a new dispatcher, and finally incrementally rebuild the modified files.

  • main branch:
    • 49 files to rebuild (67 total)
    • cpu time: avg 120s (single thread)
  • PR:
    • 12 files to rebuild
    • cpu time: avg 33.3s (single thread)

Speed up: avg 350%

Clearly defined dependencies

Continuing with the example above, in the main branch it is unclear how many files depend on KeybindManager, even though there are only three instances of `#include "KeybindManager.hpp", a change in the header results in in 49 modified files.

In this PR, the dependant of KeybindManager can be easly found using a simple grep 1,

$ grep KeybindManager.hpp -lR src | grep -v pch
src/config/ConfigManager.cpp
src/debug/HyprCtl.cpp
src/events/Windows.cpp
src/helpers/Monitor.cpp
src/layout/DwindleLayout.cpp
src/layout/MasterLayout.cpp
src/layout/IHyprLayout.cpp
src/managers/input/InputManager.cpp
src/managers/KeybindManager.cpp
src/plugins/PluginAPI.cpp
src/Window.cpp
src/Compositor.cpp

Footnotes

  1. Some are not hard dependencies and further improvements can be made.

@vaxerski
Copy link
Member

yes. The problem with this is
image

idk about the "chaotic" part, if I have 30 includes on top of every file, to me, that is even more chaotic.

@memchr
Copy link
Contributor Author

memchr commented Sep 22, 2023

We could absolutely eliminate almost all the includes in each cpp file by just including a single pch.hpp file that includes everything else, but that would complicate the dependency. It is chaotic because of how the individual units are linked together.

e.g.

ninja -C build -t graph

@memchr
Copy link
Contributor Author

memchr commented Sep 22, 2023

If lines of code is important as a metric, I think another approach is to include all the use std and dependency library headers into a single common header and include it in each required file instead.

But separating the headers owned by Hyprland could still be valuable for development, as it means fewer rebuilds and fewer complaints from the language server.

@vaxerski
Copy link
Member

well, including all the stl shit into a common.hpp file would be bad the other way - increase compile times massively. Up till now I have been trying to strike a balance between the two.

@memchr
Copy link
Contributor Author

memchr commented Sep 22, 2023

There is another more conservative branch which only touches headers from /src, the diff should be half this size.

@vaxerski
Copy link
Member

where

@vaxerski
Copy link
Member

vaxerski commented Sep 24, 2023

wait, since this is just a util, can't we make it run before build, write the new source files to ./build and compile that? I believe CMake would not have issues with that

@memchr
Copy link
Contributor Author

memchr commented Sep 25, 2023

You mean something like LLVM's clang-include-cleaner/fixer or Google's iwyu?
Both run as slow as C++ compilers, if not slower.

@vaxerski
Copy link
Member

hm, never mind then, sucks.

@rtgiskard
Copy link
Contributor

rtgiskard commented May 21, 2024

Just personal opinion, I believe the best practice is still to include only and directly what it uses. Maybe step by step, but new include should follow the rule.

Share common includes in several headers maybe good enough at the beginning, but it's likely to cause more trouble as time goes by.

Tens of includes on top of every file may be chaotic to some degree, but it's still necessary code to keep things explicit and straight forward. Just like the code for other functions or classes, it's there for a reason, and can be ignored too when you're focused on something else.

@memchr memchr closed this May 21, 2024
@memchr memchr deleted the iwyu branch May 21, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants