Move some values from DisplayServer to DSTypes namespace, to reduce includers

[Ivorforce]

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.h reduces 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.h to account for files that do need to interface with DisplayServe somewhat, but only through constants. This is true for most includers.

In addition, I designed STATIC_ASSERT_INCOMPLETE_TYPE to protect ourselves against regressions. If someone changes includes, such that e.g. DisplayServer is once again accessible from rendering_server.h, this will trigger a compile error.
I'm not aware of similar tricks used elsewhere, but it seems to work.

[lawnjelly]

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.

[Ivorforce]

I think the checks are absolutely necessary, otherwise we'll be hunting include regressions forever.
An SCU define would totally work 👍

[lawnjelly]

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).

[Ivorforce]

I'm actually away over the weekend, so feel free to take it 👍

[Ivorforce]

I think I got it working

[lawnjelly]

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 SConstruct (in the main folder), line 633:

I think we can change this:

# Run SCU file generation script if in a SCU build.
if env["scu_build"]:
    max_includes_per_scu = 8
    if env.dev_build:
        max_includes_per_scu = 1024

to add a line:

# Run SCU file generation script if in a SCU build.
if env["scu_build"]:
    env.Append(CPPDEFINES=["SCU_BUILD_ENABLED"])
    max_includes_per_scu = 8
    if env.dev_build:
        max_includes_per_scu = 1024

Something like that should do the trick I think.

(Your code seems to suggest SCU_BUILD is already defined, but I'm not sure where, maybe we did this already?)

[lawnjelly]

All said though:

In addition, I designed STATIC_ASSERT_INCOMPLETE_TYPE to protect ourselves against regressions. If someone changes includes, such that e.g. DisplayServer is once again accessible from rendering_server.h, this will trigger a compile error.

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:

• keep the dependencies lower
• annoy contributors, especially in cases where they need to get around this (for e.g. structural change)

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:

• Have the benchmark server monitor the build times for touching various files, and flag up any major regressions in dependencies.
• Monitor build times yourself for touching files, and when someone merges something that breaks the dependencies, do a fixup PR. 🤷‍♂️

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:

• I personally think the macro is worth a trial (at least on some small scale), and if there is pushback, it can always be reverted.
• Decision will probably be down to @akien-mga as he has a lot of experience dealing with contributor pain points.

[Ivorforce]

This is going to be sensitive to include order I think

Actually, no!
I initially had the checks in the .h file, which would have been sensitive to include order. However, I also identified this problem, and hence moved the checks to the corresponding .cpp. Since our code style guidelines require the translation unit's header to be included separately at the top, it is guaranteed that it is the only include by the time we make the checks. Only afterwards do we include the rest of the headers.
Effectively, we're communicating "If you only included rendering_server.h, you do not automatically include display_server.h along with it". I think this is a reasonable requirement to make. Even when somebody refactors the code base, these should hold (assuming we only make actually logical requirements, such as Node2D should not include Node3D and vice versa).

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.

Yep!

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.

This is true, but I don't think most contributors will be touching the includes of a .h file. If they do, they'll hopefully be knowledgable enough with the codebase and C++ to understand this imposition.

• Have the benchmark server monitor the build times for touching various files, and flag up any major regressions in dependencies.

I love this idea! I don't think it's mutually exclusive though; both would be good to have.

• Monitor build times yourself for touching files, and when someone merges something that breaks the dependencies, do a fixup PR. 🤷‍♂️

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).