Skip to content

Commit

Permalink
refactor(client): make RequestBuilder non-generic
Browse files Browse the repository at this point in the history
Improve the compile-time of downstream crates that use RequestBuilder,
by not forcing them to remonomorphise and recompile its non-generic
methods when they use it: with this change, they can just call the
precompiled versions in the `hyper` object file(s). The `send` method is
the major culprit here, since it is quite large and complicated.

For an extreme example,

    extern crate hyper;
    fn main() {
        hyper::Client::new().get("x").send().unwrap();
    }

takes ~4s to compile before this patch (i.e. generic RequestBuilder) and
~2s after. (The time spent interacting with LLVM goes from 2.2s to
0.3s.)

BREAKING CHANGE: `RequestBuilder<U>` should be replaced by `RequestBuilder`.
  • Loading branch information
huonw committed Oct 14, 2015
1 parent d2e9c94 commit ff4a607
Showing 1 changed file with 24 additions and 15 deletions.
39 changes: 24 additions & 15 deletions src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,42 +151,42 @@ impl Client {
}

/// Build a Get request.
pub fn get<U: IntoUrl>(&self, url: U) -> RequestBuilder<U> {
pub fn get<U: IntoUrl>(&self, url: U) -> RequestBuilder {
self.request(Method::Get, url)
}

/// Build a Head request.
pub fn head<U: IntoUrl>(&self, url: U) -> RequestBuilder<U> {
pub fn head<U: IntoUrl>(&self, url: U) -> RequestBuilder {
self.request(Method::Head, url)
}

/// Build a Patch request.
pub fn patch<U: IntoUrl>(&self, url: U) -> RequestBuilder<U> {
pub fn patch<U: IntoUrl>(&self, url: U) -> RequestBuilder {
self.request(Method::Patch, url)
}

/// Build a Post request.
pub fn post<U: IntoUrl>(&self, url: U) -> RequestBuilder<U> {
pub fn post<U: IntoUrl>(&self, url: U) -> RequestBuilder {
self.request(Method::Post, url)
}

/// Build a Put request.
pub fn put<U: IntoUrl>(&self, url: U) -> RequestBuilder<U> {
pub fn put<U: IntoUrl>(&self, url: U) -> RequestBuilder {
self.request(Method::Put, url)
}

/// Build a Delete request.
pub fn delete<U: IntoUrl>(&self, url: U) -> RequestBuilder<U> {
pub fn delete<U: IntoUrl>(&self, url: U) -> RequestBuilder {
self.request(Method::Delete, url)
}


/// Build a new request using this Client.
pub fn request<U: IntoUrl>(&self, method: Method, url: U) -> RequestBuilder<U> {
pub fn request<U: IntoUrl>(&self, method: Method, url: U) -> RequestBuilder {
RequestBuilder {
client: self,
method: method,
url: url,
url: url.into_url(),
body: None,
headers: None,
}
Expand All @@ -201,30 +201,39 @@ impl Default for Client {
///
/// One of these will be built for you if you use one of the convenience
/// methods, such as `get()`, `post()`, etc.
pub struct RequestBuilder<'a, U: IntoUrl> {
pub struct RequestBuilder<'a> {
client: &'a Client,
url: U,
// We store a result here because it's good to keep RequestBuilder
// from being generic, but it is a nicer API to report the error
// from `send` (when other errors may be happening, so it already
// returns a `Result`). Why's it good to keep it non-generic? It
// stops downstream crates having to remonomorphise and recompile
// the code, which can take a while, since `send` is fairly large.
// (For an extreme example, a tiny crate containing
// `hyper::Client::new().get("x").send().unwrap();` took ~4s to
// compile with a generic RequestBuilder, but 2s with this scheme,)
url: Result<Url, UrlError>,
headers: Option<Headers>,
method: Method,
body: Option<Body<'a>>,
}

impl<'a, U: IntoUrl> RequestBuilder<'a, U> {
impl<'a> RequestBuilder<'a> {

/// Set a request body to be sent.
pub fn body<B: Into<Body<'a>>>(mut self, body: B) -> RequestBuilder<'a, U> {
pub fn body<B: Into<Body<'a>>>(mut self, body: B) -> RequestBuilder<'a> {
self.body = Some(body.into());
self
}

/// Add additional headers to the request.
pub fn headers(mut self, headers: Headers) -> RequestBuilder<'a, U> {
pub fn headers(mut self, headers: Headers) -> RequestBuilder<'a> {
self.headers = Some(headers);
self
}

/// Add an individual new header to the request.
pub fn header<H: Header + HeaderFormat>(mut self, header: H) -> RequestBuilder<'a, U> {
pub fn header<H: Header + HeaderFormat>(mut self, header: H) -> RequestBuilder<'a> {
{
let mut headers = match self.headers {
Some(ref mut h) => h,
Expand All @@ -242,7 +251,7 @@ impl<'a, U: IntoUrl> RequestBuilder<'a, U> {
/// Execute this request and receive a Response back.
pub fn send(self) -> ::Result<Response> {
let RequestBuilder { client, method, url, headers, body } = self;
let mut url = try!(url.into_url());
let mut url = try!(url);
trace!("send {:?} {:?}", method, url);

let can_have_body = match &method {
Expand Down

5 comments on commit ff4a607

@mhristache
Copy link

Choose a reason for hiding this comment

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

After this change it's not possible to use url::Url as input to client.get(). Instead a &str is needed. So url::Url has to be serialised first which implies an allocation.

@Ryman
Copy link
Contributor

@Ryman Ryman commented on ff4a607 Nov 21, 2015

Choose a reason for hiding this comment

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

@maximih Do you have the error message? The bounds haven't changed for get?

Are you having an issue with crates mismatching for url perhaps?

@mhristache
Copy link

Choose a reason for hiding this comment

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

You are right. It seems to have something to do with url crate. Sorry for the false alarm.

It works fine if I force the url crate version: url = "=0.2.38".

If I use url = "*" (which translates to url version 0.5.0), I get this error:

src/lib.rs:288:20: 288:34 error: the trait `hyper::client::IntoUrl` is not implemented for the type `url::Url` [E0277]
src/lib.rs:288             client.post(auth_url)

I had a quick look over the changes in the url crate and they don't seem related to this issue. So, no idea really why this happens.

@Ryman
Copy link
Contributor

@Ryman Ryman commented on ff4a607 Nov 22, 2015

Choose a reason for hiding this comment

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

Rust sees two different versions of the same crate as entirely different types, so when hyper implements a trait for the version that it imports, the trait is only implemented for that specific version of url::Url.

@mhristache
Copy link

Choose a reason for hiding this comment

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

Thanks for the clarifications. This is very good to know!
Somehow I managed to avoid this problem before even though I use wildcards for most of my dependencies so that I get latest updates using cargo update, without changing Cargo.toml. Time to move to specific versions.

Please sign in to comment.