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

Stop statically linking binaries when building with Nix. #126

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

SamirTalwar
Copy link
Contributor

What

It turns out that statically linking with musl increases our response time under load from (in a very specific benchmark, on my computer) ~10ms to ~30ms per response. Disabling static linking fixes this issue, bringing ndc-postgres in line with HGE v2.

This feels like a good enough reason not to statically link the code.

We ship the Docker image built with Nix, so this directly impacts production. It also simplifies development (in theory) because nix build and cargo build now have behavior that is more similar.

How

I deleted the Nix code that turns on static linking.

It turns out that statically linking with musl increases our response
time under load from (in a very specific benchmark, on my computer)
~10ms to ~30ms per response.

This feels like a good enough reason not to statically link the code.
@TheInnerLight
Copy link
Contributor

Did you get any further information on what the memory impact of this change was?

I think we should merge this anyway but it'd be nice to record it.

Copy link
Contributor

@danieljharvey danieljharvey left a comment

Choose a reason for hiding this comment

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

Performance is very much our focus, so this seems the thing to do.

@SamirTalwar
Copy link
Contributor Author

FWIW there doesn't seem to be any meaningful memory impact here. It's hard to tell because it throws off the statistics (virtual memory is hard), but I don't believe we use a noticeably different amount of memory.

@SamirTalwar SamirTalwar added this pull request to the merge queue Nov 1, 2023
Merged via the queue into main with commit b5cf0ad Nov 1, 2023
27 checks passed
@SamirTalwar SamirTalwar deleted the samirtalwar/dynamic-builds branch November 1, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants