chore: Strip debuginfo symbols for release#14843
Conversation
|
🙌🏾 One consideration re: For example, if some panic occurs during writing to an external table - do we want to abort? Or maybe it's preferable to surface the error (as I think would be the current behavior but not completely sure). |
Cargo.toml
Outdated
| [profile.release] | ||
| codegen-units = 1 | ||
| lto = true | ||
| debug = false |
There was a problem hiding this comment.
nit: I believe this is superfluous - https://doc.rust-lang.org/cargo/reference/profiles.html#debug
|
DF does not catch panics, so the process will crash anyway no matter what the setting is. Potentially DF even can gain some performance which I will check separately and share the results here |
@comphead If I'm not mistaken, DF may catch panics in certain cases. For example, if a serialization task panics during writing to some object store (see here). I confirmed this by inserting a dummy panic in the serialization task. |
|
Hey @rkrishn7 this is a good call yes, although DF does not catch panics, we cannot say the same for the dependencies. I'll experiment later with panic mode for DF crate level only, leaving other dependencies compile flags intact. But for this PR it is safer to remove abort on panics |
|
@alamb can I have a quick review for this? |
Cargo.toml
Outdated
| lto = true | ||
| debug = false | ||
| strip = true | ||
| panic = "abort" |
There was a problem hiding this comment.
I think it would be useful to add some comments here about why we chose to abort on panic (to decrease the binary size)
There was a problem hiding this comment.
I rolled back the panic behavior. the PR is for the strip only which is safe.
Panic compilation behavior is also applied to dependencies which can be shoot in the foot, and this parameter is not overridable on crate level
https://doc.rust-lang.org/cargo/reference/profiles.html#overrides
Overrides cannot specify the panic, lto, or rpath settings.
Although it saves extra 15-20% I dont think we can use it in near future
3 participants
Which issue does this PR close?
Minimizing the DF binary size from 60M to 50M by stripping debuginfo.
Removing unwind panic saves 10 more MB
The stripped binary has the biggest methods for std panic
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?