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

switch to hyper? #24

Open
extrawurst opened this issue May 19, 2021 · 9 comments · May be fixed by #25
Open

switch to hyper? #24

extrawurst opened this issue May 19, 2021 · 9 comments · May be fixed by #25

Comments

@extrawurst
Copy link

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?

@extrawurst
Copy link
Author

extrawurst commented May 19, 2021

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

@pimeys
Copy link
Collaborator

pimeys commented May 19, 2021

See: #22

@extrawurst
Copy link
Author

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

@extrawurst
Copy link
Author

extrawurst commented May 19, 2021

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.

@extrawurst extrawurst linked a pull request May 19, 2021 that will close this issue
@pimeys
Copy link
Collaborator

pimeys commented May 22, 2021

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.

@kate-shine
Copy link

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.

@extrawurst
Copy link
Author

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.

@kate-shine
Copy link

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)

@extrawurst
Copy link
Author

@pimeys anything else you need for this?

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 a pull request may close this issue.

3 participants