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

chore: Strip debuginfo symbols for release #14843

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

comphead
Copy link
Contributor

@comphead comphead commented Feb 23, 2025

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?

@rkrishn7
Copy link
Contributor

🙌🏾

One consideration re: panic=abort is that it will cause the program to crash upon some task panicking. Unclear on whether this is more/less desirable than bubbling up the error to the user.

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
@@ -159,19 +159,20 @@ url = "2.5.4"
[profile.release]
codegen-units = 1
lto = true
debug = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

@comphead
Copy link
Contributor Author

DF does not catch panics, so the process will crash anyway no matter what the setting is.
Although there are some test cases which does catch_unwind but we do run tests in debug mode.
Another thing for abort it doesn't call object destructors if process crashed which for DF imho it is not an issue.

Potentially DF even can gain some performance which I will check separately and share the results here

@rkrishn7
Copy link
Contributor

DF does not catch panics, so the process will crash anyway no matter what the setting is.

@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.

@comphead
Copy link
Contributor Author

comphead commented Feb 24, 2025

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

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.

2 participants