Skip checking twice for non-zero status code in `StatusCode::from_u16`

[CMDJojo]

See this code snippet, status.rs lines 72-81:

    #[inline]
    pub fn from_u16(src: u16) -> Result<StatusCode, InvalidStatusCode> {
        if !(100..1000).contains(&src) {
            return Err(InvalidStatusCode::new());
        }

        NonZeroU16::new(src)
            .map(StatusCode)
            .ok_or_else(InvalidStatusCode::new)
    }

Here, we use the safe API NonZeroU16::new to create the StatusCode, but we have already done a check that our code is at least 100 (and thus already non-zero). We should change this to use NonZeroU16::new_unchecked, which skips the zero check. It also makes it easier for the compiler to figure out that it is a very trivial operation, and will make the code run slightly faster in debug builds. (For optimised builds, the compiler seems to figure out that the second check is not needed anyways, as far as I can see ). The new_unchecked API is way older than MSRV (1.28.0) and has been const ever since.

I suggest we change this to use the unsafe API:

    #[inline]
    pub fn from_u16(src: u16) -> Result<StatusCode, InvalidStatusCode> {
        if !(100..1000).contains(&src) {
            return Err(InvalidStatusCode::new());
        }

        // SAFETY: We just checked that src is non-zero
        let code = unsafe { NonZeroU16::new_unchecked(src) };
        Ok(StatusCode(code))
    }

[seanmonstar]

If the compiler is able to optimize the second check out, then it seems like leaving it alone so we don't need unsafe is a nice option. No?

[CMDJojo]

I would say that it is an improvement that’s good to have anyways. The check is only optimised out when compiling with optimisations enabled, not in debug builds. It will most likely speed up compilation, speed up runtime on debug builds, and rely less on compiler tricks. unsafe isn't something inherently bad, as long as the developer ensures that the preconditions are met (which they clearly are here). One other example where unsafe is used is for the hard-coded status codes, to avoid the zero-check for those since they are known to be non-zero. (See status_codes macro at status.rs:311)

[sfackler]

What performance improvement to real-world code did you measure as a result of this change?

[CMDJojo]

I didn’t measure any performance improvements. I just used this library and checked the implementation of one of the methods and saw an area of improvement so I made this PR as a suggestion. No need to implement it if you don’t see fit.

[seanmonstar]

I appreciate wanting to make the library better! In this case, I think the trade-off is likely not worth it, in the end. I suspect the compiler can optimize out the second check, and even if it can't, I don't think from_u16 is a hot code path that needs us to add an unsafe block here.

Thanks for looking into this, though!