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

core: reduce monomorphization of the futures code #2406

Merged
merged 1 commit into from
Mar 3, 2025

Conversation

yukibtc
Copy link
Contributor

@yukibtc yukibtc commented Jan 17, 2025

These changes will reduce the monomorphization of the futures code, resulting in a reduction of the binary size.

In my case, the binaries for android targets have become 100/200kb smaller.

@yukibtc yukibtc requested a review from a team as a code owner January 17, 2025 16:06
@yukibtc yukibtc requested review from badboy and removed request for a team January 17, 2025 16:06
@bendk
Copy link
Contributor

bendk commented Jan 21, 2025

Thanks for the patch it's a really interesting idea. I think we should try to think through the trade-offs here a bit more before landing this one.

I hope to put together a bunchmark on async call overhead. This is my main question: how much overhead does boxing the futures add per-call? The other half of the trade-off is the binary size, can you share some details about your project so we can understand that better? Like, how many async functions are you exporting?

Once we know both of those, I think we'll be in a much better position to judge this one. Maybe the boxing overhead will be small enough that we can just hit the merge button. Maybe it's a real trade-off and we should add a config option for this.

@yukibtc
Copy link
Contributor Author

yukibtc commented Jan 22, 2025

I hope to put together a bunchmark on async call overhead. This is my main question: how much overhead does boxing the futures add per-call?

I'll try to add some benchmark soon.

The other half of the trade-off is the binary size, can you share some details about your project so we can understand that better? Like, how many async functions are you exporting?

This is the project: https://github.com/rust-nostr/nostr/tree/d030b776f521fc2bb032f9490bab89e3bfbb5108/bindings/nostr-sdk-ffi

I tried to count the async fn with a script and the result is 117 functions.

This is the result of cargo bloat --release -n 1000 of my project compiled for x86_64-unknown-linux-gnu target using uniffi b46ab54:

cargo-bloat-uniffi-b46ab54.txt

While this is the result using the patch (uniffi a6c2f67):

cargo-bloat-uniffi-a6c2f67.txt

@yukibtc
Copy link
Contributor Author

yukibtc commented Jan 22, 2025

I'll try to add some benchmark soon.

I tried to take a look at the current benches but I'm not sure how the new ones should be added

@bendk
Copy link
Contributor

bendk commented Jan 23, 2025

Thanks for those details, it's very interesting to go through. I'll try to put together an async benchmark in the next week or two.

@bendk
Copy link
Contributor

bendk commented Feb 28, 2025

Thanks for those details, it's very interesting to go through. I'll try to put together an async benchmark in the next week or two.

Well, one month later and those benchmarks still aren't up. I still really want to put them together, but I've had to focus on other priorities. OTOH, you have solid numbers that this is an improvement for your use case. I'm leaning towards merge, but I'd like to get the opinion of some other UniFFI devs. @badboy @mhammond what do you think?

@bendk
Copy link
Contributor

bendk commented Feb 28, 2025

@yukibtc can you update the PR. I think #2418 indroduced some conflicts, but I don't think they're hard to fix.

@mhammond
Copy link
Member

I think I lean towards taking them simply because there's a known problem fixed by it, and only a lack of certainty without real evidence about side-effects. However, that lack of certainty means that I don't think it should be in a 0.29 version (or at least not the next one, but instead it should have time to bake on main)

These changes will reduce the monomorphization of the futures, resulting in a reduction of the binary size.

Signed-off-by: Yuki Kishimoto <[email protected]>
@yukibtc
Copy link
Contributor Author

yukibtc commented Mar 1, 2025

Rebased and fixed the conflicts

Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

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

Thanks for the code, let's merge this one.

@bendk bendk merged commit de3d65e into mozilla:main Mar 3, 2025
5 checks passed
@yukibtc yukibtc deleted the futures branch March 4, 2025 10:21
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