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

Improve build speed [manually] (forward-declarations/clean up includes/etc.) #57641

Merged

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented Feb 4, 2022

This PR aims to improve compilation time by forward-declaring some classes whenever possible/when it makes sense and cleaning up/moving some include statements. There is still some more room to improve, but I think this should be done in subsequent PRs as this manual process of finding optimization opportunities can be very time consuming.

Changes:

  • Add forward-declarations of EditorFileDialog and EditorNode to several classes. This is already done in some cases, but not consistently.
  • Remove unused includes of file_dialog.h
  • Remove forward-declaration of Container in flow_container.h as it was used as a base class which can cause problems. (could be moved to a separate PR)
  • Optimize includes in EditorNode and sort forward declarations
  • Make missing relative includes (primarily editor includes) absolute for consistency

Some benchmarks (as of Feb 11 2022):
[Time of rebuild. before -> this PR]

Change in progress_dialog.h
28s -> 23s (~17% reduction)

Change in dependency_editor.h
25s -> 23s (~8% reduction)

Change in editor_data.h
3:55m -> 3:01m (~23% reduction)

Change in file_dialog.h
3:47m -> 21s (~91% reduction)

Change in line_edit.h
4:37m -> 3:50m (~17% reduction)

Changed all files above:
4:30m -> 3:56 (~13% reduction)

@Calinou
Copy link
Member

Calinou commented Feb 4, 2022

This is a good start, nice work 🙂

Did you see #57569 by the way?

@Geometror Geometror force-pushed the compilation-time-improvements-1 branch from f3e8ed7 to a0a7f43 Compare February 5, 2022 00:03
@Geometror
Copy link
Member Author

@Calinou Thats quite interesting, will see what I can do with that :)
I am currently trying to create a dependency graph of the godot codebase to find some more optimization opportunities. Does anyone know a good tool (Windows/Linux) to render large/complex graphs and analyze them? I already have the DOT data.

@Calinou
Copy link
Member

Calinou commented Feb 5, 2022

There's include-what-you-use but I don't know how difficult it would be to run on a SCons-using codebase.

@Geometror Geometror force-pushed the compilation-time-improvements-1 branch 4 times, most recently from 851b17d to 2582b0f Compare February 5, 2022 16:36
@akien-mga
Copy link
Member

There's include-what-you-use but I don't know how difficult it would be to run on a SCons-using codebase.

It's not too difficult, it just needs a wrapper script. I ran it a few years ago, but it's not trivial to make good use of it as it affects the whole codebase.

When using it, a good first step would be to try to sort includes in core only (and anything else that requires changes due to core include changes).

@Geometror
Copy link
Member Author

Geometror commented Feb 7, 2022

@Calinou @akien-mga
As you said, getting it to work was straight-forward, however there are problems which makes it quite difficult to make good use of it:

  • Sometimes it removes includes of generated headers
  • platform specific includes are added (this can be easily fixed though)
  • it fails at some complex templates (with specializations) which are declared across multiple files

Tried it in core but couldn't get it to work because of the last point above. It always fails at the GetTypeInfo template and I don't know why. If someone wants to have a look at it:
https://github.com/Geometror/godot/tree/iwyu-core-1

EDIT: Works now. See #57760.

@Geometror Geometror force-pushed the compilation-time-improvements-1 branch 4 times, most recently from b2ff322 to a93301d Compare February 11, 2022 14:28
@Geometror Geometror marked this pull request as ready for review February 11, 2022 14:32
@Geometror Geometror requested review from a team as code owners February 11, 2022 14:32
editor/editor_toaster.h Outdated Show resolved Hide resolved
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work overall! I reviewed the whole diff and commented on some style issues and a couple problems, but that's a big improvement already as is.

@Geometror Geometror force-pushed the compilation-time-improvements-1 branch from 220ce3b to b396fd4 Compare February 12, 2022 01:46
@Geometror
Copy link
Member Author

Fixed all issues, except the ones regarding the dependency editor header as suggested. I was tempted to fix those, but there are still many places left where a similar cleanup could be done so this is indeed material for another PR :)

@akien-mga akien-mga merged commit 7a7fabe into godotengine:master Feb 12, 2022
@akien-mga
Copy link
Member

Thanks a lot! I know this was tedious work but it's really appreciated :)

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.

4 participants