1
\$\begingroup\$

This seems to be working fine, but it performance testing with wrk is poor compared to what I've come up in other languages. Shouldn't Rust excel in serving static files?

  1. How can I make this faster?
  2. Are there any other potential problems here?
extern crate hyper;
extern crate futures;

use std::fs::File;
use std::io::Read;
use std::path::Path;
use std::ffi::OsStr;
use hyper::StatusCode;
use futures::future::FutureResult;
use hyper::server::{Http, Service, Request, Response};

mod api;

struct Router;
impl Service for Router {
    type Request = Request;
    type Response = Response;
    type Error = hyper::Error;
    type Future = FutureResult<Response, hyper::Error>;

    fn call(&self, req: Request) -> Self::Future {
        futures::future::ok(match req.path() {
            "/json" => api::default::json(req),
            _ => static_file(req.path()),
        })
    }
}

fn main() {
    let addr = "127.0.0.1:5000".parse().unwrap();
    let server = Http::new().bind(&addr, || Ok(Router)).unwrap();
    println!("Listening on http://{} with 1 thread...", server.local_addr().unwrap());
    server.run().unwrap();
}

pub fn static_file(path: &str) -> Response {
    let mut f = match File::open("src/static".to_owned() + path) {
        Ok(f) => f,
        Err(_) => {
            return Response::new().with_status(StatusCode::NotFound)
        },
    };

    let mut data = String::new();
    match f.read_to_string(&mut data) {
        Ok(_) => {},
        Err(_) => {
            return Response::new().with_status(StatusCode::NotFound)
        },
    };

    let mut headers = hyper::header::Headers::new();
    match Path::new(&path).extension().and_then(OsStr::to_str) {
        Some(ext) => match ext {
            "html" => headers.set_raw("Content-Type", "text/html"),
            "css" => headers.set_raw("Content-Type", "text/css"),
            "js" => headers.set_raw("Content-Type", "application/javascript"),
            _ => headers.set_raw("Content-Type", "text/plain"),
        },
        None => headers.set_raw("Content-Type", "text/plain"),
    };

    Response::new()
        .with_status(StatusCode::Ok)
        .with_headers(headers)
        .with_body(data)
}
\$\endgroup\$
5
  • \$\begingroup\$ Did you compile in release mode? \$\endgroup\$
    – Shepmaster
    Commented Dec 25, 2017 at 0:53
  • \$\begingroup\$ While not strictly required for code review, your code doesn't include the api module so no answerer will be able to actually compile and test your code so see if it's slow or fast. \$\endgroup\$
    – Shepmaster
    Commented Dec 25, 2017 at 0:56
  • \$\begingroup\$ Yes in release mode \$\endgroup\$
    – twharmon
    Commented Dec 25, 2017 at 0:56
  • \$\begingroup\$ The api module shouldn’t have anything to do with serving static files. \$\endgroup\$
    – twharmon
    Commented Dec 25, 2017 at 0:57
  • \$\begingroup\$ is poor compared to what I've come up in other languages — you seem to have forgotten to specify what performance numbers you expect. \$\endgroup\$
    – Shepmaster
    Commented Dec 25, 2017 at 3:17

2 Answers 2

3
\$\begingroup\$

Initial feedback:

  1. Reading to a string is wasteful if you just want to dump data. A Vec is simpler since you don't care about UTF-8 validity; this will also allow for images to be served.
  2. Returning "Not found" for a failure to read seems like a lie. Should be a HTTP 5xx-series error.
  3. Avoid code duplication by returning the content type from the match and setting it afterward.
  4. text/plain is an unconventional default content type, generally I see application/octet-stream.
  5. Extract a constant for the default content type instead of repeating it.

Turning towards performance, start by performing a base-line performance test of a failure URL:

$ wrk http://127.0.0.1:5000/nothing
Running 10s test @ http://127.0.0.1:5000/nothing
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   290.75us   44.99us   1.50ms   90.46%
    Req/Sec    17.00k   537.99    18.05k    73.76%
  341686 requests in 10.10s, 26.72MB read
  Non-2xx or 3xx responses: 341686
Requests/sec:  33831.33
Transfer/sec:      2.65MB

Then a test with an existing but blank file:

$ wrk http://127.0.0.1:5000/exists
Running 10s test @ http://127.0.0.1:5000/exists
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   432.23us   77.26us   1.67ms   85.00%
    Req/Sec    11.49k   586.77    12.74k    65.84%
  231023 requests in 10.10s, 25.34MB read
Requests/sec:  22874.91
Transfer/sec:      2.51MB

Then a test with a 1 MiB file:

$ wrk http://127.0.0.1:5000/big
Running 10s test @ http://127.0.0.1:5000/big
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    38.58ms    5.60ms  94.66ms   95.67%
    Req/Sec   129.41     16.53   171.00     85.50%
  2595 requests in 10.06s, 2.53GB read
Requests/sec:    257.93
Transfer/sec:    257.96MB

This isn't terrible. The biggest problem I see is that you are using an asynchronous library in a completely synchronous manner. Using a threadpool to offload loading the files increases throughput to ~3x of the previous value:

$ wrk http://127.0.0.1:5000/big
Running 10s test @ http://127.0.0.1:5000/big
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    13.60ms    3.75ms  62.03ms   84.27%
    Req/Sec   370.48     43.96   415.00     89.50%
  7403 requests in 10.04s, 7.23GB read
Requests/sec:    737.26
Transfer/sec:    737.35MB

Its possible caching might help even further, but I didn't attempt it.

extern crate futures;
extern crate futures_spawn;
extern crate futures_threadpool;
extern crate hyper;

use futures::Future;
use futures_spawn::SpawnHelper;
use futures_threadpool::ThreadPool;
use hyper::StatusCode;
use hyper::server::{Http, Request, Response, Service};
use std::ffi::OsStr;
use std::fs::File;
use std::io::Read;
use std::path::Path;
use std::sync::Arc;

struct Router {
    pool: ThreadPool,
}

impl Service for Router {
    type Request = Request;
    type Response = Response;
    type Error = hyper::Error;
    type Future = Box<Future<Item = Self::Response, Error = Self::Error>>;

    fn call(&self, req: Request) -> Self::Future {
        let full_path = "src/static".to_owned() + req.path();

        let data = self.pool.spawn_fn(move || {
            let mut f = File::open(full_path)?;

            let mut data = Vec::new();
            f.read_to_end(&mut data)?;
            Ok(data)
        });

        let mut headers = hyper::header::Headers::new();

        const UNKNOWN_CONTENT_TYPE: &str = "text/plain";
        let content_type = match Path::new(req.path()).extension().and_then(OsStr::to_str) {
            Some(ext) => match ext {
                "html" => "text/html",
                "css" => "text/css",
                "js" => "application/javascript",
                _ => UNKNOWN_CONTENT_TYPE,
            },
            None => UNKNOWN_CONTENT_TYPE,
        };
        headers.set_raw("Content-Type", content_type);

        let r = data.map(|data| {
            Response::new()
                .with_status(StatusCode::Ok)
                .with_headers(headers)
                .with_body(data)
        });

        Box::new(r)
    }
}

fn main() {
    let addr = "127.0.0.1:5000".parse().unwrap();

    let router = Arc::new(Router {
        pool: ThreadPool::new_num_cpus(),
    });

    let server = Http::new()
        .bind(&addr, move || Ok(Arc::clone(&router)))
        .unwrap();

    println!(
        "Listening on http://{} with 1 thread...",
        server.local_addr().unwrap()
    );
    server.run().unwrap();
}
\$\endgroup\$
0
\$\begingroup\$

As always when looking at performance, profile the app and see where the time is spent. My guess from looking at the code would be that you're not using buffered IO, try something like this:

let mut f = match File::open("src/static".to_owned() + path) {
    ...
};

let mut buf_reader = BufReader::new(file);
let mut data = String::new();
match buf_reader.read_to_string(&mut data) {
    ...
};

Apart from that, I would probably make static_file return a Result-type instead of having all the matches inline. Don't think it matters wrt to performance, but it makes the code quite a bit cleaner, I think.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.