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

Experimenting reducing stack size of Error #658

Closed
wants to merge 1 commit into from

Conversation

RCasatta
Copy link
Contributor

@RCasatta RCasatta commented Mar 12, 2024

This draft PR reduce the stack size of the crate::Error type from 72 bytes to 32 bytes by boxing bigger variants.

I noticed a very noticeable reduction in binary size produced which I can't explain
in comparison to master the rlib produced is reduced by 0.5% and big example of about 2%.

At the same time llvm lines emitted are almost the same.

Can someone explain this?

@apoelstra
Copy link
Member

If I had to guess, boxing error variants means that some of the error subtypes don't need to be compiled at all anymore.

@RCasatta
Copy link
Contributor Author

ah, okay...

It seems the savings are enough to justify the annoying boxing, should I polish this to pursue merging?

@apoelstra
Copy link
Member

@RCasatta I'd suggest not, unless you urgently want this work in, because I am working toward cleaning up the error types with my recent refactoring PRs. So the top-level Error type should hopefully go away and be replaced by smaller more specific errors.

Having said that -- in my own code, I haven't decided yet whether to use boxing. I think probably I should, in particular to deal with the generic parsing errors (<Pk as FromStr>::Error etc). Right now I think we convert these to strings lol. The alternatives are to box the errors (still requires alloc, plausibly loses some information users might want) or to make our error types generic (which will blow up codegen and is super annoying to use).

So my plan is also to box errors.

Conveniently, our existing error framework is so bad that even a "bad" solution like this is definitely an improvement :).

@sanket1729 do you have any opinions?

@RCasatta
Copy link
Contributor Author

I have no hurry, we can close for now and revisit after refactoring if needed

@RCasatta RCasatta closed this Mar 14, 2024
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