Move some values from DisplayServer to DSTypes namespace, to reduce includers#107492
Move some values from DisplayServer to DSTypes namespace, to reduce includers#107492Ivorforce wants to merge 1 commit intogodotengine:masterfrom
DisplayServer to DSTypes namespace, to reduce includers#107492Conversation
|
|
||
| #define STATIC_ASSERT_INCOMPLETE_TYPE(m_type) \ | ||
| class m_type; \ | ||
| static_assert(is_incomplete_type_v<m_type>, #m_type " was defined when it should not be. Please check include hierarchy of " __FILE__); |
There was a problem hiding this comment.
Ah I can see a problem here with SCU builds. It will depend on the build parameters whether in some cases display_types.h might get included before the cpp I think?
If so and you really want this to work, we could set a c++ define when in a SCU build, and omit this check.
There was a problem hiding this comment.
I think the checks are absolutely necessary, otherwise we'll be hunting include regressions forever.
An SCU define would totally work 👍
There was a problem hiding this comment.
Ok remind me tommorrow on RocketChat, I'll take a look into it if you can't work out how to (I'm not super familiar with the build files and #defines but I'm sure we can work it out).
There was a problem hiding this comment.
I'm actually away over the weekend, so feel free to take it 👍
There was a problem hiding this comment.
I think I got it working
DisplayServer to DSTypes namespace, to reduce includersDisplayServer to DSTypes namespace, to reduce includers
… reduce includers. Add `STATIC_ASSERT_INCOMPLETE_TYPE` to check includes hierarchy at compile time.
|
I had a look into making a define to let you know it's a scu build, it turns out to look pretty simple (untested): In I think we can change this: to add a line: Something like that should do the trick I think. (Your code seems to suggest |
|
All said though:
I've done quite a bit of improving compile times in various codebases, and my gut needs a bit of convincing this is a good idea. This is going to be sensitive to include order I think, and we even have things like the clang tidy stuff rearranging include order automatically. It sounds like a potential pain in the behind for contributors (although to be fair it looks intended to be! 😁 ). If I understand you, this is a guard against the problem of contributors being sloppy and includes and undoing all the build optimization you might have done. It's a difficult problem, and for myself, I just gave up early and made SCU builds instead (which to some extent mitigates this). So I kind of applaud the attempt, but I don't know whether this will be a net positive in practice.. I guess it will:
I say this because there is sometimes resistance to things that would significantly decrease compile times, but lead to some small consideration for other contributors, e.g. #106272 which decreases 4.x build times by quarter to a third. I'm personally pretty pragmatic, and to me some small housekeeping to decrease compile times is an absolute win, because the compile / test / debug cycle is the rate limiting step for development, especially for maintainers. But I will admit Godot has lots of contributors of varying levels, and their priorities may be different, a new contributor could potentially get put off by systems they aren't used to. Some alternatives:
There doesn't seem to be any ideal solution so far. But maybe your macro is worth a trial at least, we can't say it would annoy people without trying it first. 😀 So in summary:
|
Actually, no!
Yep!
This is true, but I don't think most contributors will be touching the includes of a
I love this idea! I don't think it's mutually exclusive though; both would be good to have.
Besides not actually enforcing the goal of low include counts, I don't think I will practically be able to do this 😅 (Let's continue discussing the checker in #107587, this PR is kinda bloated with other changes to be able to review the check itself properly). |
Cross-includes are our main compile time problem right now. Not only does parsing take long, also every small change to a header file leads to thousands of recompilations. It's time to do something.
With this change, the number of compile units to include
display_server.hreduces from 326 to 248. That means making changes to it should compile about 30% faster. There should also be a small improvement to clean compile times (but I did not measure this).I hope that, by incrementally reducing cross-includes, we can reduce the compile time of the engine to a fraction of what it is now, over time.
Background
This is a test run for improving compile times throughout the codebase.
For one, I created
dstypes.hto account for files that do need to interface withDisplayServesomewhat, but only through constants. This is true for most includers.In addition, I designed
STATIC_ASSERT_INCOMPLETE_TYPEto protect ourselves against regressions. If someone changes includes, such that e.g.DisplayServeris once again accessible fromrendering_server.h, this will trigger a compile error.I'm not aware of similar tricks used elsewhere, but it seems to work.