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

Status codes 100-999 without macros #443

Merged
merged 8 commits into from
Dec 8, 2020

Conversation

dekellum
Copy link
Contributor

Motivation

I started this as a simple review of #438, as I agree with the motivation of supporting status codes that exist on the internet, and was intrigued by the claim that it actually reduced binary size while adding more static status code strings! I made some changes, initially for testing that I think are worth keeping here, then found some associated unexpected compiler behavior, and ended up further optimizing. @quininer might have considered some or all of this, but avoided it in the spirit of minimal changes. I've included their commits here as is.

Changes

(1) The StatusCode::as_str method was never being unit tested across the entire range of supported values and it was growing difficult to eyeball the linear lists of codes in source to confirm there wasn't a typo. I first tried adding this check to the existing macro_rules! test_round_trip but found the compile time blew up when I did! So I rewrote this (8166eab) as a simple test with a for loop, and was able to remove the testing list of status codes entirely.

(2) I confirmed that binary size is reduced, even with the additional 500 status codes. However I also found compile time much increased with #438, apparently due to the increasingly taxing macro expansion that is involved? With (1) in place, it was simple enough to just inline the entire set of status codes as a single string constant (b9efebf) and this actually improves both binary size and compile time vs master. See Test Findings below.

I also experimented with building the CODE_DIGITS via a const fn (1115e74) which is a bit nicer on the eyes in the source code, but that requires rust 1.46 and so I don't think its worth the trouble yet.

Test Findings

Scripted:

for br in master quininer-438 direct-status-codes; do
    echo
    git checkout $br
    echo "timed release build of libhttp.rlib and status_code test"
    rm target/release/libhttp.rlib target/release/deps/status_code-3d7620cc7678908c
    time cargo build -q --release --lib --tests
    du -b target/release/libhttp.rlib
    du -b target/release/deps/status_code-3d7620cc7678908c
done

Output with rustc 1.49.0-nightly (1773f60ea 2020-11-08) on linux dev host:

Switched to branch 'master'
timed release build of libhttp.rlib and status_code test

real	0m18.733s
user	0m51.882s
sys	0m1.449s
1778714	target/release/libhttp.rlib
5395000	target/release/deps/status_code-3d7620cc7678908c

Switched to branch 'quininer-438'
timed release build of libhttp.rlib and status_code test

real	0m40.281s
user	1m13.319s
sys	0m2.313s
1752384	target/release/libhttp.rlib
5427768	target/release/deps/status_code-3d7620cc7678908c

Switched to branch 'direct-status-codes'
timed release build of libhttp.rlib and status_code test

real	0m11.428s
user	0m38.490s
sys	0m1.040s
1744832	target/release/libhttp.rlib
5359464	target/release/deps/status_code-3d7620cc7678908c

So this PR is saving about 7s+ of build time and 33KiB in release rlib size vs master, which doesn't hurt. I think this makes the case for adding the additional status codes quite reasonable.

@dekellum
Copy link
Contributor Author

This is an alternative fix for #144.

@seanmonstar
Copy link
Member

I greatly appreciate the effort you put into this, this is really cool! As I've said in the issue and other PR, I assert that any server actually generating a status code in the 600-999 range is bad. But I'm willing to merge one of these in.

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).

@dekellum
Copy link
Contributor Author

I don't disagree that >599 is bad form, but we've also had historic vagueness in the specs and leniency including in current implementations referenced in #144. I think we've assumed there was adequate reason to want to serialize the StatusCode over full range without allocation? There was an existing compile time cost to the macro expansion to support this, which this PR now removes. Thanks for offering that a breaking change to remove as_str entirely might be okay, but would that be better (e.g. less code) on balance when including the impact of the change downstream?

Note that #380 is also compatible with this PR.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Sorry about the lag, I'm ready to accept this (and some other PRs). I think there's just one point I left inline we should comment on.

src/status.rs Show resolved Hide resolved
@dekellum
Copy link
Contributor Author

dekellum commented Dec 8, 2020

Also improved the StatusCode documentation for the supported range, in 9c772d7.

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