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

Link hash table primes externally to prevent data duplication in binary #88178

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

Listwon
Copy link
Contributor

@Listwon Listwon commented Feb 10, 2024

I've started analyzing Godot binaries on Windows using SizeBench and on top of the duplicate data results were hash_table_size_primes and hash_table_size_primes_inv.
SizeBench

As visible above the tool shows around 168KB of wasted memory (tested on commit 36e943b scons target=template_release debug_symbols=yes)

This PR reduces build size by 547KB ( scons target=template_release).

File scoped const arrays are linked internally which means each time hashfuncs.h are included, those two arrays are not merged into single data point (which would be fine and expected for const, read only data). I've moved their initialization to hashfuncs.cpp and marked extern in hashfuncs.h. Thanks to @Malcolmnixon suggestion I've made it simpler by using inline constexpr from C++17 standard.

I've not benchmarked it yet. In theory it can improve the CPU cache, because all of the hash_maps and hash_sets reuse the data from the same address in memory (meaning - there is higher chance that it will be still present in cache when needed).

@Listwon Listwon requested a review from a team as a code owner February 10, 2024 16:54
@Listwon Listwon force-pushed the hash-table-build-size branch from 1ec7020 to 690b751 Compare February 10, 2024 16:56
@AThousandShips AThousandShips added this to the 4.x milestone Feb 10, 2024
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I don't see how this functionally changes the code, so it seems like an decent optimization.

@Malcolmnixon
Copy link
Contributor

That's a good catch.

Since C++17 is now supported and the guidelines promote using constexpr, you might want to see if keeping the table in the header and marking them constexpr (and possibly inline) works as well. In theory this should also merge multiple instances of the data across different translation units, but also expose the values as constants to the compiler allowing constant-folding if accessed discretely.

@Listwon
Copy link
Contributor Author

Listwon commented Feb 10, 2024

I think I tried constexpr already and it didn't change anything in this case, but I'll try inline constexpr. As you said, in theory it should work the same as the solution with extern.

Edit: I've just confirmed it - constexpr gives the same output as const. inline constexpr reduces the size - it merges the data.

@Listwon Listwon force-pushed the hash-table-build-size branch from 690b751 to 3fca4d0 Compare February 10, 2024 20:43
Copy link
Contributor

@Malcolmnixon Malcolmnixon left a comment

Choose a reason for hiding this comment

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

This looks good. Nice change!

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.

Looks great! Thanks for the initiative looking into optimizing binary size and memory usage, that's very welcome :)

@akien-mga akien-mga merged commit b30373e into godotengine:master Feb 12, 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.

6 participants