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

Extract SafeBinaryMutex to separate header #87893

Merged

Conversation

vittorioromeo
Copy link
Contributor

This change simply extracts 'SafeBinaryMutex' from 'mutex.h' to 'safe_binary_mutex.h', in an effort to reduce the compilation speed impact of including mutex.h.

Similarly in nature to #87871, this is a pure refactoring PR that shouldn't affect the behavior or Godot in any way or form.

@vittorioromeo vittorioromeo requested a review from a team as a code owner February 3, 2024 09:33
@AThousandShips
Copy link
Member

Can you show some data to show this reduces compile times?

core/os/safe_binary_mutex.h Outdated Show resolved Hide resolved
core/os/safe_binary_mutex.h Show resolved Hide resolved
core/os/safe_binary_mutex.h Show resolved Hide resolved
@vittorioromeo
Copy link
Contributor Author

Can you show some data to show this reduces compile times?

I've benchmarked the compilation of the entire engine using ClangBuildAnalyzer and -ftime-trace. Here's the original report:

https://gist.github.com/vittorioromeo/fa153c494ce05da08c988a27130a79e3

There are a few heavy-hitters here and there, and not everything is easy to optimize, so I am starting with some low-hanging fruits. The "expensive headers" section of the report shows that mutex.h is included quite often and takes a considerable amount of time.

Reducing the amount of code that mutex.h contains is a way to reduce the impact of including that header. Furthermore, splitting a specialized components (SafeBinaryMutex) that is only used in one place to its own header seems like good engineering practice to me regardless of what the impact on compilation time might be.

@AThousandShips
Copy link
Member

AThousandShips commented Feb 3, 2024

Can you show after this change as well? Showing the problem without indicating this is a solution would be missing the important part IMO, i.e. the showing it improves compile time

@vittorioromeo
Copy link
Contributor Author

Can you show after this change as well? Showing the problem without indicating this is a solution would be missing the important part IMO, i.e. the showing it improves compile time

https://gist.github.com/vittorioromeo/f59a1f1e312beb603977256059681717

# BEFORE
779723 ms: core/os/mutex.h (included 1104 times, avg 706 ms), included via:

# AFTER
758503 ms: core/os/mutex.h (included 1110 times, avg 683 ms), included via:

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Looks fine to me, you just need to run clang-format or sort the includes manually by lexicographical order.

    This change simply extracts 'SafeBinaryMutex' from 'mutex.h' to
    'safe_binary_mutex.h', in an effort to reduce the compilation
    speed impact of including `mutex.h`.
@vittorioromeo
Copy link
Contributor Author

vittorioromeo commented Feb 4, 2024

@Geometror: should be good to go now! :)
Can you also please look at #87871 ?

@akien-mga akien-mga changed the title Extract 'SafeBinaryMutex' to separate header Extract SafeBinaryMutex to separate header Feb 5, 2024
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.

Makes sense to me.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 5, 2024
@akien-mga akien-mga merged commit 79539cb into godotengine:master Feb 5, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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