panicbit/fcm-rust

switch to hyper?

Opened this issue ยท 9 comments

I love this crate. We are using it for months now, just one thing kept bugging me: the usage of reqwest where most of their features are irrelevant to us in this context.

I threw together a quick test and here are the comparisons:
I build the example for comparison both using cargo b --release --example simple_sender.

using reqwest:
compilation units: 138
binary size: 5.5 MB
compile time: 2m 17s

using hyper:
compilation units: 114
binary size: 3.6 MB
compile time: 1m 42s

are you interested in a PR for this?

using rustls feature (cargo b --example simple_sender --release --no-default-features --features "rustls")

reqwest:
compilation units: 137
binary size: 6.7 MB
compile time: 2m 50s

hyper:
compilation units: 124
binary size: 4.7 MB
compile time: 2m 05s

See: #22

for the sake of completeness I also compared the last feature:

using vendored-tls feature (cargo b --example simple_sender --release --no-default-features --features "vendored-tls")

reqwest:
compilation units: 139
binary size: 5.5 MB
compile time: 2m 25s

hyper:
compilation units: 114
binary size: 3.6 MB
compile time: 1m 44s

See: #22

@pimeys can you elaborate?

my suggestion is to keep everything compatible, support for rustls-feature and vendored-tls-feature but just with less dependencies, less compile time and less binary size.

We had hyper, and user @kate-shine worked a PR, switching to reqwest. I'd like you to discuss this reasoning, I'm mostly not working on this crate anymore.

The reason for me to switch to reqwest was a more stable API and prevention of unnecessary boilerplate code on the side of the library. Usage of reqwest is recommended by the authors of hyper as well.

I'm not completely opposed to switching back to pure hyper, but fighting for every dependency at all cost seems quite pointless to me. Especially if this dependency used for convenience is one of best supported ones.

The reason for me to switch to reqwest was a more stable API and prevention of unnecessary boilerplate code on the side of the library.

Looking at the code I do not see the boilerplate that was removed. Please see the PR diffs: #25

Usage of reqwest is recommended by the authors of hyper as well.

The statement says If you are looking for a convenient HTTP client, then you may wish to consider reqwest. True this applies if you make use of those convenient features. I am opposed paying costs of things I am not using and as the diff shows the code changes are minimal, the convenience gained is minimal but as my benchmarks proof the price is real.

I'm not completely opposed to switching back to pure hyper, but fighting for every dependency at all cost seems quite pointless to me. Especially if this dependency used for convenience is one of best supported ones.

The crate is stable, there is little benefit from keeping dead weight around, and I still argue that it does not return the invest.
Depending on your project size rust unfortunately becomes a dog slow compile and if I can shave of 1min off my 10min build this is life time saved, not talking about the energy saved.

Sorry, I didn't have time to look at the PR before. Yes, you are correct. Former switch to reqwest has led to leaner code, but your changes don't change that. (personally, for our purpose, it would actually make more sense and lead to less dependencies if there was an awc version, as we use it in the actix-web environment)

@pimeys anything else you need for this?