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

Optimize build size #1781

Merged
merged 2 commits into from Oct 7, 2021
Merged

Optimize build size #1781

merged 2 commits into from Oct 7, 2021

Conversation

MarinPostma
Copy link
Contributor

Remove debug symbols from the release build, and strip the binaries.

We used to need to debug symbols for sentry, but since it was removed with #1616, we don't need them anymore.

Shrinks the binary size from ~300MB to ~50MB on linux.

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Hum... I am not sure about that but the backtrace i.e. RUST_BACKTRACE=1 will not work anymore, am I right? If I'm wrong and the backtrace can always be displayed correctly without having to specify the debug = true in the Cargo.toml file, I would gladly approve this PR.

Cargo.toml Outdated Show resolved Hide resolved
@irevoire
Copy link
Member

irevoire commented Oct 7, 2021

Yeah not sure either we want to remove the debug symbol; when a user encounters an unknown panic, the symbols/backtrace can give us a hint at what is going wrong 😬

@MarinPostma
Copy link
Contributor Author

@Kerollmops none of the distribution of meilisearch comes with the RUST_BACKTRACE=1 enabled, and we never ever collect them anyway. The backtrace is rendered correctly without the debug symbols:
image

but not when the binary is stripped. I think we can put aside stripping for now.

Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

Since we can still see the panics I'm ok with this change

@irevoire irevoire mentioned this pull request Oct 7, 2021
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Same, as we see the panic backtraces, I approve this!

@curquiza curquiza added this to the v0.24.0 milestone Oct 7, 2021
@curquiza curquiza changed the title optimize build size Optimize build size Oct 7, 2021
@irevoire
Copy link
Member

irevoire commented Oct 7, 2021

Here is a screenshot of a panic before this PR:
image

Now a screenshot with this PR:
image

And finally, a screenshot of what we would have if we stripped + compressed the binary with upx as shown in #1782:
image

@Kerollmops do you still think this is ok?
We lost all the line numbers of the backtrace in this PR.
For me, the only line we can use if we merge this PR is the only line kept when stripping the binary. So why not strip everything as well? 🤔

thread 'actix-rt|system:0|arbiter:0' panicked at 'Hello', meilisearch-http/src/routes/indexes/search.rs:140:5

(I took every screenshot with the option RUST_BACKTRACE=1)

@Kerollmops
Copy link
Member

For me, the only line we can use if we merge this PR is the only line kept when stripping the binary. So why not strip everything as well? 🤔

That's no true, sometimes it panics in a function that is not quite useful (inside of [T]::split_at, <[T] as Index>::index) and sometimes not even in a function of the standard library.

@irevoire
Copy link
Member

irevoire commented Oct 7, 2021

So are we sure we want to merge this PR as-is?

@Kerollmops
Copy link
Member

What is the size gain for this PR, already?

@irevoire
Copy link
Member

irevoire commented Oct 7, 2021

On my computer, without this PR, the binary weight 326MB, and with this PR it's only 35MB

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

That's a lot! maybe we can merge this. I think that the backtrace we got without all of the line numbers is enough to debug.

@irevoire
Copy link
Member

irevoire commented Oct 7, 2021

bors merge

@bors
Copy link
Contributor

bors bot commented Oct 7, 2021

@bors bors bot merged commit 602a327 into main Oct 7, 2021
@bors bors bot deleted the optimize-binary-size branch October 7, 2021 14:03
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.

None yet

4 participants