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

refactor(client): replace futures-cpupool with tokio's thread pool #1514

Closed
wants to merge 1 commit into from

Conversation

bluetech
Copy link
Contributor

The tokio thread pool is already transitively used by the tokio runtime,
so by using it for the DNS as well, the futures-cpupool dependency is
removed without adding a new one.

The code uses tokio-threadpool's blocking function which makes the
code very straightforward.

Tokio's thread pool is supposedly faster than futures-cpupool's one.
According to its README, "Why not futures-cpupool? It's 10x slower.".

The user of the library can configure the maximum number of threads for
executing blocking tasks as part of configuring tokio's runtime, which
also holds for any other blocking code besides hyper (e.g. blocking file
operations using tokio-fs ). Hyper no longer needs to concern itself
with that.

BREAKING CHANGE: The threads argument is removed from
hyper::client::HttpConnector::new() and new_with_handle(). To set
the maximum amount of threads to use for DNS resolving, configure the
max_blocking parameter of the tokio runtime instead.

The function hyper::client::HttpConnector::new_with_executor() is
removed. The thread pool of the tokio runtime is now always used. Use
new() or new_with_handle() instead.

The tokio thread pool is already transitively used by the tokio runtime,
so by using it for the DNS as well, the futures-cpupool dependency is
removed without adding a new one.

The code uses tokio-threadpool's `blocking` function which makes the
code very straightforward.

Tokio's thread pool is supposedly faster than futures-cpupool's one.
According to its README, "Why not futures-cpupool? It's 10x slower.".

The user of the library can configure the maximum number of threads for
executing blocking tasks as part of configuring tokio's runtime, which
also holds for any other blocking code besides hyper (e.g. blocking file
operations using tokio-fs ). Hyper no longer needs to concern itself
with that.

BREAKING CHANGE: The `threads` argument is removed from
  `hyper::client::HttpConnector::new()` and `new_with_handle()`. To set
  the maximum amount of threads to use for DNS resolving, configure the
  `max_blocking` parameter of the tokio runtime instead.

  The function `hyper::client::HttpConnector::new_with_executor()` is
  removed. The thread pool of the tokio runtime is now always used. Use
  `new()` or `new_with_handle()` instead.
@bluetech
Copy link
Contributor Author

This is something I've noticed. I don't know if it's desirable but it was easy enough to make the changes.

I have only performed basic tests. If someone has e.g. a web crawler program which uses hyper, that could be a good stress-test, and comparison before/after.

I should also note that tokio-threadpool's README says "Note: This library isn't quite ready for use.". That said, the dependency is private so updating it will not be a breaking change.

@bluetech
Copy link
Contributor Author

Travis fails on nightly due to this rustc bug: rust-lang/rust#50707

@seanmonstar
Copy link
Member

Thanks! I'll look at this more in the morning.

As for performance/stress, I know @pimeys pushes a lot of requests through the client. Though, might be skipping DNS, not sure.

@seanmonstar
Copy link
Member

One thought is that using blocking means it only works with the threadpool executor, while there is now also a current_thread runtime.

@pimeys
Copy link

pimeys commented May 14, 2018

We skip the DNS after forming the connection so I doubt this has any effect on our use case...

@bluetech
Copy link
Contributor Author

@seanmonstar Hmm indeed, I did not think of that. This seems to make blocking unusable from libraries, since libraries cannot assume which runtime they are running under. I'll open an issue about that in the tokio repo, I think it is worth mentioning in the docs.

I suppose that the "remove a dependency" advantage can be salvaged by creating a tokio_threadpool::ThreadPool, if someone's up to it.

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