hyperium / http

Rust HTTP types
Apache License 2.0
1.16k stars 292 forks source link

Future of StatusCode::as_str #468

Open seanmonstar opened 3 years ago

seanmonstar commented 3 years ago

I wanted to write-up some thoughts on the future direction for the StatusCode::as_str method. The as_str method returns the numeric part of the status code as a string slice. So, StatusCode::OK.as_str() returns "200", and StatusCode::NOT_FOUND.as_str() returns "404", and so forth. This works for all StatusCodes, from 100-999. Since the StatusCode only stores an integer, there's no field to actually borrow the string from, so it is in reality returning a &'static str, even though the method signature doesn't expose that.

There have been several PRs filed to make that part of the signature (that is, change from fn as_str(&self) -> &str to fn as_str(&self) -> &'static str) (#380 #436 #466).

I've personally questioned the value of the method in it's entirety:

I was thinking that perhaps we just axe StatusCode::as_str, because really, how useful can it be? There's the Display impl to get a string, and if you want a fast path for some common status codes, a user can just do that manually (hyper has a fast path for HTTP/1.1 200 OK, for instance).

That comment was prompted as I wondered about the cost of including all the static strings in the binary, when certainly most applications will never need them. Because I don't see value in the method, I've held off from changing it return a &'static str. Instead, I've thought we may just want to deprecate it and chop it for 1.0.

However, I realize I could be wrong, and not considering a large use case for this method. If so, we can just merge one of the PRs and carry on. If I have overlooked something, hopefully we can collect the reasons why to keep the method, with some details about the value it provides versus the cost of having a bunch of extra strings in the binary for everyone.

dekellum commented 3 years ago

Thanks for taking the time to write up your thoughts. FWIW: I just want to (re-)state that the rationale and spirit of [#438] -> #443 (comment) was that the ship has sailed on the feature, and its costly for downstream to remove it, with little practical gain in removing it achieved here (since we improved it in #443, anyway).

Now, that said, I totally get that the feature as it stands may have very limited value when you consider that you are just sidestepping it when in hyper, for example, you have a static constant for "HTTP/1.1 200 OK" and you don't really need the others, perf. wise.

Your writeup also suggests that it was perhaps in some ways wise that I didn't just add the change for 'static to #443, in the hope of getting it merged with the larger changes! (I'm almost always surprised with what actually gets merged and what gets ignored for 2 years, here! And then there is release numbers!)

Arnavion commented 3 years ago

I use .as_str() in my web server for serializing responses. I like that it doesn't require a formatter or allocating a String, since the response serialization part of the server is heavily geared towards not allocating.

If it was removed from http, I would end up having to implement the table that as_str uses today myself, which sort of defeats the purpose of having a common crate for HTTP things.

tesaguri commented 3 years ago

If you really want a str slice rather than an impl Display, you can allocate a buffer on the stack and write the status code into it, since we know the exact size of the output.

use std::io::Write;
use std::str;

use http::StatusCode;

let mut buf = [0u8; 3];
write!(&mut buf[..], "{:03}", StatusCode::OK.as_u16()).unwrap();
let s = str::from_utf8(&buf).unwrap();

assert_eq!(s, "200");

Edit: By the way, I've found that you can get a &'static str out of a &StatusCode if it is a standard one:

use http::StatusCode;

pub fn status_code_as_static_str(s: &StatusCode) -> Option<&'static str> {
    match *s {
        StatusCode::CONTINUE => Some(StatusCode::CONTINUE.as_str()),
        // ...
        _ => None,
    }
}

Is this an expected (or desirable) behavior?

Arnavion commented 3 years ago

As I said, "doesn't require a formatter or allocating a String". write!() requires a formatter.

robjtede commented 2 years ago

FWIW, Actix Web doesn't use this method and opts for as_u16().

I think if someone really wants to avoid using a formatter then going with itoa is a good and very fast solution. If you're looking for things to cut in v1.0 to reduce API surface this might is a contender, IMO. Although the odds of someone opening a PR to add it back are high, it'd be good to ask those people what their use case is.

Arnavion commented 2 years ago

it'd be good to ask those people what their use case is.

My use case has not changed since my first comment.

let io_slices = [
    std::io::IoSlice::new(b"HTTP/1.1 "),
    std::io::IoSlice::new(status.as_str().as_bytes()),
    std::io::IoSlice::new(b" \r\n"),
    /* headers */,
    std::io::IoSlice::new(b"\r\n"),
    /* body */,
];

stream.write_all_vectored(io_slices);

I think if someone really wants to avoid using a formatter then going with itoa is a good and very fast solution.

itoa produces good code for a const http::StatusCode - the itoa::Buffer::new().format(const_status_code) sequence compiles to a str directly created within a 40-byte buffer, which is very efficient (though it wastes many instructions filling the whole 40-byte buffer even though it ends up using only three bytes from it). But it's not as efficient as the current http::StatusCode::as_str which is just subsliced from the giant constant. And it's even worse for a non-const http::StatusCode - the itoa version does a ton of integer math while the as_str is again just a subslice.

Does the overall efficiency matter significantly? Not really; CPUs are very fast, and serializing the first line of an HTTP response is a tiny share of work that a web server does for generating that response. But http::StatusCode::as_str has already been written. It already works. It's already used. I'm not sure why it needs to be removed in the first place.

robjtede commented 2 years ago

4th PR to change return type to &'static str: https://github.com/hyperium/http/pull/569

fasterthanlime commented 2 years ago

4th PR to change return type to &'static str: #569

That's me! My use case is similar to @Arnavion - but because I'm using tokio-uring I need not just a &str, but &'static str (the buffer being written must have a stable address, because it's owned by the kernel while the write is in-flight).

LucioFranco commented 2 years ago

That comment was prompted as I wondered about the cost of including all the static strings in the binary, when certainly most applications will never need them.

Do we actually know the cost of this?