update HTTP.default_status_message_for to use official IANA Registry #4540
Conversation
|
Can you benchmark |
Yes, it's significantly slower on release. https://gist.github.com/ujifgc/479e0261bda871923f82efa4715342ab Should I just extract this method to |
|
@ujifgc could you run that benchmark again using We could use macros to create the case from the hash, which would allow applications to access the hash flexibly if needed but keep the performance of the current method. |
|
|
I guess as the number of codes increases, the case will only stay significantly faster if we order the common codes (200, 301, 302, 404, 403, 201, 500, 422, ...) to the top. |
|
I'm new to macros. It this a proper way to generate this method? def self.default_status_message_for2(status_code : Int) : String
{% for code, message in STATUS_CODES %}
return {{message}} if status_code == {{code}}
{% end %}
""
end |
|
|
|
@jhass that's assuming that llvm doesn't optimise it to a lookup table. I'm not sure if llvm can perform that optimization at all or if it would be up to crystal to do that. @ujifgc please only put one iteration in the Benchmark.ips block so we can see the speed of one operation not many. ips is a full micro benchmark harness ich does everything for you. |
The Benchmark code is like this: Benchmark.ips do |x|
keys = HTTP::STATUS_CODES.keys
x.report("case sample") { HTTP.default_status_message_for(keys.sample) }
x.report("hash sample") { HTTP.default_status_message_for1(keys.sample) }
x.report("macro sample") { HTTP.default_status_message_for2(keys.sample) }
end
[100, 200, 404, 500].each do |status|
Benchmark.ips do |x|
x.report("case #{status}") { HTTP.default_status_message_for(status) }
x.report("hash #{status}") { HTTP.default_status_message_for1(status) }
x.report("macro #{status}") { HTTP.default_status_message_for2(status) }
end
endhttps://gist.github.com/ujifgc/479e0261bda871923f82efa4715342ab |
|
Hash is significantly slower; using a macro increases complexity, and impacts compilation time. What did this PR intended to achieve? |
|
I'm a little confused at why the macro-generated if statement is so much faster than the case here. As far as I recall, case statements were lowered to chained |
|
For now, let's just add/fix the status code list without removing that method, moving it to a constant or using a hash literal. Please open a separate issue discussing how we can better model that, but just using the IANA registry list should be good for now. |
ujifgc
changed the title
|
I updated this PR so it just updates status codes, adds the comment on the authority info and a spec. |
|
Thanks ❤️ ! |
This PR moves HTTP status codes from 'http/common.cr' to 'http/status.cr' and updates them to conform with the official IANA registry of HTTP status codes: https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml.
Inspired by https://bugs.ruby-lang.org/issues/12935