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

Aggressive inlining (+20% serialization speed boost) #147

Merged
merged 1 commit into from
Nov 30, 2024

Conversation

hoxxep
Copy link
Contributor

@hoxxep hoxxep commented Nov 28, 2024

This PR adds a roughly 20% speed improvement to serialization in my application benchmarks.

The use of #[inline(always)] is required because LLVM sees the function as "large" and avoids inlining a plain #[inline] hint. However, almost all usage of these methods in ciborium specify the enum at compile-time, which means this "large" function can be optimised to a small set of instructions and remove many branches.

I could add a ciborium-bench crate into the workspace using criterion to demonstrate this if this change is likely to be accepted? Cheers!


One minor concern is compiling with opt_level s or z, where application developers may prefer a smaller bundle size (such as WASM use cases). We could use a smaller-bundle feature flag for this case, and use #[cfg_attr(not(feature = "smaller-bundle"), inline(always)] instead. The binary bloat is likely minor though.

@hoxxep hoxxep requested a review from a team as a code owner November 28, 2024 13:28
@hoxxep hoxxep requested a review from rjzak November 28, 2024 13:28
@rjzak
Copy link
Member

rjzak commented Nov 28, 2024

What's the size increase? Maybe gate the inlining by feature or not Wasm?
Edit: maybe it doesn't matter since there's a lot of inlining anyway.

Copy link
Member

@npmccallum npmccallum left a comment

Choose a reason for hiding this comment

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

I'm very happy to have more aggressive inlining. I'm not worried about code size. If people complain, we can solve the problem then.

My biggest question is: should we be even more aggressive?

@rjzak
Copy link
Member

rjzak commented Nov 30, 2024

LGTM. I can merge, or wait for further inlining.

@hoxxep
Copy link
Contributor Author

hoxxep commented Nov 30, 2024

Perhaps merge this for now and we can investigate further inlining later?

I found inline always on the Header/Title deserialization methods had little effect and might need more of a refactor to achieve a similar improvement.

I haven't checked the WASM bundle size but agree with the rationale (and reckon it's a minor effect anyway). It might be worth adding criterion benchmarks into this repo though.

@rjzak rjzak merged commit 5d620dc into enarx:main Nov 30, 2024
53 of 56 checks passed
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