cohttp.headers: use faster comparison #778
Conversation
|
It's weird that the standard implementation of |
Actually I did not check that 😂 let me try |
Signed-off-by: Marcello Seri <[email protected]>
|
These are very unscientific and unrealistic benchmarks, since they run on my laptop and test a very dumb server, but here they give practically equivalent results 🤷 I added a commit to use |
Signed-off-by: Marcello Seri <[email protected]>
I have recently switched to using Cohttp types for my http 1.0/1.1 parser and I ran a small benchmark on one of my parser tests. The change in your PR does seem to offer improvements in my little microbenchmark when compared to the previous version on the headersv2 branch. That said there could potentially be further improvements if breaking changes are acceptable, ex: using case insensitive comparison for header keys instead of modifying user input via The parser is defined at: https://github.com/anuragsoni/h1/blob/d659ad37f1222523725e8f37f6250de549f64112/h1_parser/src/h1_parser.ml and the benchmark code is https://github.com/anuragsoni/h1/blob/d659ad37f1222523725e8f37f6250de549f64112/bench/main.ml Note: Tests were run on an m1 macbook air The payload is: let req = "GET /wp-content/uploads/2010/03/hello-kitty-darth-vader-pink.jpg HTTP/1.1\r\n\
Host: www.kittyhell.com\r\n\
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; ja-JP-mac; \
rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 Pathtraq/0.9\r\n\
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8\r\n\
Accept-Language: ja,en-us;q=0.7,en;q=0.3\r\n\
Accept-Encoding: gzip,deflate\r\n\
Accept-Charset: Shift_JIS,utf-8;q=0.7,*;q=0.7\r\n\
Keep-Alive: 115\r\n\
Connection: keep-alive\r\n\
Cookie: wp_ozh_wsa_visits=2; wp_ozh_wsa_visit_lasttime=xxxxxxxxxx; \
__utma=xxxxxxxxx.xxxxxxxxxx.xxxxxxxxxx.xxxxxxxxxx.xxxxxxxxxx.x; \
__utmz=xxxxxxxxx.xxxxxxxxxx.x.x.utmccn=(referral)|utmcsr=reader.livedoor.com|utmcct=/reader/|utmcmd=referral\r\n\
\r\n"Results were as follows:
The caseless comparison change can be seen at https://github.com/mirage/ocaml-cohttp/compare/feature/headersV2...anuragsoni:caseless-compare-header-key?expand=1 |
|
Thanks a lot for these tests. Since we are breaking the header module anyway, given the small difference, I think this is one of the instances in which we could go with it if we can justify it. I don't personally tend to rely on the case of the keys, since I use the Header module functions for comparisons in general. I hope I am not too biased thinking that this is what most people do. And seeing how little needed to change in the tests, I think this breakage is subtle but should be minimal in practice (and we could still be very vocal and explicit about it). I would like to hear some other bell on this. Is a 200 nanoseconds improvement over this PR worth the extra semantic change? Since it seems that a lot of the latency cost is hidden somewhere in the guts of the IO module and Lwt, are we overdoing for the header module? |
|
PS If I had to cast a vote, I would vote for using @anuragsoni case-insensitive patch. |
There are definitely other improvements that can be made to cohttp that'll make more of an impact :) I think your |
|
I think your solution is also low maintenance, it is very close to my first commit fwiw :) |
|
It seems like a good (and cheap !) improvement 👍. |
Signed-off-by: Marcello Seri <[email protected]>
Signed-off-by: Marcello Seri <[email protected]>
Signed-off-by: Marcello Seri <[email protected]>
Signed-off-by: Marcello Seri <[email protected]>
|
I have added your change here, and now I also remember why I had ditched the case-insensitive comparison when working on my initial PR. There are a number of property tests that need to be fixed :P |
Thanks for pulling in the commit here. I can help updating the necessary tests later in the week if that works :) |
Signed-off-by: Marcello Seri <[email protected]>
|
Oh, thanks a lot. I fixed the immediate one. We only need to update the |
I proposed a PR to this branch on your fork mseri#2 |
adapt tests to work with case insensitive comparison
|
Thanks all for the help |
…wt-unix, cohttp-lwt-jsoo and cohttp-async (5.0.0) CHANGES: - Cohttp.Header: new implementation (lyrm mirage/ocaml-cohttp#747) + New implementation of Header modules using an associative list instead of a map, with one major semantic change (function ```get```, see below), and some new functions (```clean_dup```, ```get_multi_concat```) + More Alcotest tests as well as fuzzing tests for this particular module. ### Purpose The new header implementation uses an associative list instead of a map to represent headers and is focused on predictability and intuitivity: except for some specific and documented functions, the headers are always kept in transmission order, which makes debugging easier and is also important for [RFC7230§3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2) that states that multiple values of a header must be kept in order. Also, to get an intuitive function behaviour, no extra work to enforce RFCs is done by the basic functions. For example, RFC7230§3.2.2 requires that a sender does not send multiple values for a non list-value header. This particular rule could require the ```Header.add``` function to remove previous values of non-list-value headers, which means some changes of the headers would be out of control of the user. With the current implementation, an user has to actively call dedicated functions to enforce such RFCs (here ```Header.clean_dup```). ### Semantic changes Two functions have a semantic change : ```get``` and ```update```. #### get ```get``` was previously doing more than just returns the value associated to a key; it was also checking if the searched header could have multiple values: if not, the last value associated to the header was returned; otherwise, all the associated values were concatenated and returned. This semantics does not match the global idea behind the new header implementation, and would also be very inefficient. + The new ```get``` function only returns the last value associated to the searched header. + ```get_multi_concat``` function has been added to get a result similar to the previous ```get``` function. #### update ```update``` is a pretty new function (mirage/ocaml-cohttp#703) and changes are minor and related to ```get``` semantic changes. + ```update h k f``` is now modifying only the last occurrences of the header ```k``` instead of all its occurrences. + a new function ```update_all``` function has been added and work on all the occurrences of the updated header. ### New functions : + ```clean_dup``` enables the user to clean headers that follows the {{:https://tools.ietf.org/html/rfc7230#section-3.2.2} RFC7230§3.2.2} (no duplicate, except ```set-cookie```) + ```get_multi_concat``` has been added to get a result similar to the previous ```get``` function. - Cohttp.Header: performance improvement (mseri, anuragsoni mirage/ocaml-cohttp#778) **Breaking** the headers are no-longer lowercased when parsed, the headers key comparison is case insensitive instead. - cohttp-lwt-unix: Adopt ocaml-conduit 5.0.0 (smorimoto mirage/ocaml-cohttp#787) **Breaking** `Conduit_lwt_unix.connect`'s `ctx` param type chaged from `ctx` to `ctx Lazy.t` - cohttp-mirage: fix deprecated fmt usage (tmcgilchrist mirage/ocaml-cohttp#783) - lwt_jsoo: Use logs for the warnings and document it (mseri mirage/ocaml-cohttp#776) - lwt: Use logs to warn users about leaked bodies and document it (mseri mirage/ocaml-cohttp#771) - lwt, lwt_unix: Improve use of logs and the documentation, fix bug in the Debug.enable_debug function (mseri mirage/ocaml-cohttp#772) - lwt_jsoo: Fix exception on connection errors in chrome (mefyl mirage/ocaml-cohttp#761) - lwt_jsoo: Fix `Lwt.wakeup_exn` `Invalid_arg` exception when a js stack overflow happens in the XHR completion handler (mefyl mirage/ocaml-cohttp#762). - lwt_jsoo: Add test suite (mefyl mirage/ocaml-cohttp#764).
…wt-unix, cohttp-lwt-jsoo and cohttp-async (5.0.0) CHANGES: - Cohttp.Header: new implementation (lyrm mirage/ocaml-cohttp#747) + New implementation of Header modules using an associative list instead of a map, with one major semantic change (function ```get```, see below), and some new functions (```clean_dup```, ```get_multi_concat```) + More Alcotest tests as well as fuzzing tests for this particular module. ### Purpose The new header implementation uses an associative list instead of a map to represent headers and is focused on predictability and intuitivity: except for some specific and documented functions, the headers are always kept in transmission order, which makes debugging easier and is also important for [RFC7230§3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2) that states that multiple values of a header must be kept in order. Also, to get an intuitive function behaviour, no extra work to enforce RFCs is done by the basic functions. For example, RFC7230§3.2.2 requires that a sender does not send multiple values for a non list-value header. This particular rule could require the ```Header.add``` function to remove previous values of non-list-value headers, which means some changes of the headers would be out of control of the user. With the current implementation, an user has to actively call dedicated functions to enforce such RFCs (here ```Header.clean_dup```). ### Semantic changes Two functions have a semantic change : ```get``` and ```update```. #### get ```get``` was previously doing more than just returns the value associated to a key; it was also checking if the searched header could have multiple values: if not, the last value associated to the header was returned; otherwise, all the associated values were concatenated and returned. This semantics does not match the global idea behind the new header implementation, and would also be very inefficient. + The new ```get``` function only returns the last value associated to the searched header. + ```get_multi_concat``` function has been added to get a result similar to the previous ```get``` function. #### update ```update``` is a pretty new function (mirage/ocaml-cohttp#703) and changes are minor and related to ```get``` semantic changes. + ```update h k f``` is now modifying only the last occurrences of the header ```k``` instead of all its occurrences. + a new function ```update_all``` function has been added and work on all the occurrences of the updated header. ### New functions : + ```clean_dup``` enables the user to clean headers that follows the {{:https://tools.ietf.org/html/rfc7230#section-3.2.2} RFC7230§3.2.2} (no duplicate, except ```set-cookie```) + ```get_multi_concat``` has been added to get a result similar to the previous ```get``` function. - Cohttp.Header: performance improvement (mseri, anuragsoni mirage/ocaml-cohttp#778) **Breaking** the headers are no-longer lowercased when parsed, the headers key comparison is case insensitive instead. - cohttp-lwt-unix: Adopt ocaml-conduit 5.0.0 (smorimoto mirage/ocaml-cohttp#787) **Breaking** `Conduit_lwt_unix.connect`'s `ctx` param type chaged from `ctx` to `ctx Lazy.t` - cohttp-mirage: fix deprecated fmt usage (tmcgilchrist mirage/ocaml-cohttp#783) - lwt_jsoo: Use logs for the warnings and document it (mseri mirage/ocaml-cohttp#776) - lwt: Use logs to warn users about leaked bodies and document it (mseri mirage/ocaml-cohttp#771) - lwt, lwt_unix: Improve use of logs and the documentation, fix bug in the Debug.enable_debug function (mseri mirage/ocaml-cohttp#772) - lwt_jsoo: Fix exception on connection errors in chrome (mefyl mirage/ocaml-cohttp#761) - lwt_jsoo: Fix `Lwt.wakeup_exn` `Invalid_arg` exception when a js stack overflow happens in the XHR completion handler (mefyl mirage/ocaml-cohttp#762). - lwt_jsoo: Add test suite (mefyl mirage/ocaml-cohttp#764).
…wt-unix, cohttp-lwt-jsoo and cohttp-async (5.0.0) CHANGES: - Cohttp.Header: new implementation (lyrm mirage/ocaml-cohttp#747) + New implementation of Header modules using an associative list instead of a map, with one major semantic change (function ```get```, see below), and some new functions (```clean_dup```, ```get_multi_concat```) + More Alcotest tests as well as fuzzing tests for this particular module. ### Purpose The new header implementation uses an associative list instead of a map to represent headers and is focused on predictability and intuitivity: except for some specific and documented functions, the headers are always kept in transmission order, which makes debugging easier and is also important for [RFC7230§3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2) that states that multiple values of a header must be kept in order. Also, to get an intuitive function behaviour, no extra work to enforce RFCs is done by the basic functions. For example, RFC7230§3.2.2 requires that a sender does not send multiple values for a non list-value header. This particular rule could require the ```Header.add``` function to remove previous values of non-list-value headers, which means some changes of the headers would be out of control of the user. With the current implementation, an user has to actively call dedicated functions to enforce such RFCs (here ```Header.clean_dup```). ### Semantic changes Two functions have a semantic change : ```get``` and ```update```. #### get ```get``` was previously doing more than just returns the value associated to a key; it was also checking if the searched header could have multiple values: if not, the last value associated to the header was returned; otherwise, all the associated values were concatenated and returned. This semantics does not match the global idea behind the new header implementation, and would also be very inefficient. + The new ```get``` function only returns the last value associated to the searched header. + ```get_multi_concat``` function has been added to get a result similar to the previous ```get``` function. #### update ```update``` is a pretty new function (mirage/ocaml-cohttp#703) and changes are minor and related to ```get``` semantic changes. + ```update h k f``` is now modifying only the last occurrences of the header ```k``` instead of all its occurrences. + a new function ```update_all``` function has been added and work on all the occurrences of the updated header. ### New functions : + ```clean_dup``` enables the user to clean headers that follows the {{:https://tools.ietf.org/html/rfc7230#section-3.2.2} RFC7230§3.2.2} (no duplicate, except ```set-cookie```) + ```get_multi_concat``` has been added to get a result similar to the previous ```get``` function. - Cohttp.Header: performance improvement (mseri, anuragsoni mirage/ocaml-cohttp#778) **Breaking** the headers are no-longer lowercased when parsed, the headers key comparison is case insensitive instead. - cohttp-lwt-unix: Adopt ocaml-conduit 5.0.0 (smorimoto mirage/ocaml-cohttp#787) **Breaking** `Conduit_lwt_unix.connect`'s `ctx` param type chaged from `ctx` to `ctx Lazy.t` - cohttp-mirage: fix deprecated fmt usage (tmcgilchrist mirage/ocaml-cohttp#783) - lwt_jsoo: Use logs for the warnings and document it (mseri mirage/ocaml-cohttp#776) - lwt: Use logs to warn users about leaked bodies and document it (mseri mirage/ocaml-cohttp#771) - lwt, lwt_unix: Improve use of logs and the documentation, fix bug in the Debug.enable_debug function (mseri mirage/ocaml-cohttp#772) - lwt_jsoo: Fix exception on connection errors in chrome (mefyl mirage/ocaml-cohttp#761) - lwt_jsoo: Fix `Lwt.wakeup_exn` `Invalid_arg` exception when a js stack overflow happens in the XHR completion handler (mefyl mirage/ocaml-cohttp#762). - lwt_jsoo: Add test suite (mefyl mirage/ocaml-cohttp#764).
|
I came here from the Discuss release announcement, always curious about some performance hacking. It looks like the fun is in the following let caseless_equal a b =
if a == b then true
else
let len = String.length a in
len = String.length b
&&
let stop = ref false in
let idx = ref 0 in
while (not !stop) && !idx < len do
let c1 = String.unsafe_get a !idx in
let c2 = String.unsafe_get b !idx in
if Char.lowercase_ascii c1 <> Char.lowercase_ascii c2 then stop := true;
incr idx
done;
not !stopI have a suggestion and an idle idea:
Word-sized reads are available in the standard library as |
|
(Technically it's possible to go even further as those functions perform bound checks and also assume that the memory-access is not word-aligned, so one could generate simpler unsafe code, but it's probably not worth it, and certainly not before trying with the exposed, safe |
|
Hacking on a quick benchmark is less demanding or less boring than most of my other day tasks, so I couldn't resist. module Orig = struct
let caseless_equal a b =
if a == b then true
else
let len = String.length a in
len = String.length b
&&
let stop = ref false in
let idx = ref 0 in
while (not !stop) && !idx < len do
let c1 = String.unsafe_get a !idx in
let c2 = String.unsafe_get b !idx in
if Char.lowercase_ascii c1 <> Char.lowercase_ascii c2 then stop := true;
incr idx
done;
not !stop
end
module New = struct
let caseless_equal a b =
if a == b then true
else
let len = String.length a in
len = String.length b
&&
let rec word_loop a b i len =
if i = len then true
else
let i' = i + 8 in
if i' > len then byte_loop a b i len
else begin
if String.get_int64_ne a i = String.get_int64_ne b i
then word_loop a b i' len
else
byte_loop a b i i'
&& word_loop a b i' len
end
and byte_loop a b i len =
let c1 = String.unsafe_get a i in
let c2 = String.unsafe_get b i in
Char.lowercase_ascii c1 = Char.lowercase_ascii c2
&& (let i' = i + 1 in i' = len || byte_loop a b i' len)
in
word_loop a b 0 len
end
let impls = [
"orig", Orig.caseless_equal;
"new", New.caseless_equal;
]
let () =
match List.assoc Sys.argv.(1) impls,
int_of_string Sys.argv.(2),
Sys.argv.(3),
Sys.argv.(4)
with
| exception _ ->
prerr_endline
"Usage:\n./test [orig|new] <iter_count> '<input1>' '<input2>'";
exit 2
| caseless_equal, iter_count, input1, input2 ->
(* some quick correctnss testing *)
assert (caseless_equal "foo" "foo");
assert (caseless_equal "transfer-encoding" "traNsfer-eNcoding");
assert (caseless_equal "transfer-encoding" "transfer-Encoding");
assert
(* dash missing on the right-hand side*)
(not (caseless_equal "transfer-encoding" "traNsfer eNcoding"));
(* actual benchmark *)
for i = 1 to iter_count do
for i = 1 to 1_000 do
ignore (caseless_equal input1 input2);
done;
done;
Printf.printf "%b\n%!" (caseless_equal input1 input2) |
|
Would you like to throw up a PR gasche? |
|
@rgrinberg sure, done. But I don't know how to benchmark cohttp, so it would help if someone else could do that. |
|
Okay, we'll take care of the benchmarking for you.
Rudi.
…On Jan 6, 2022, 1:28 PM -0700, Gabriel Scherer ***@***.***>, wrote:
@rgrinberg sure, done. But I don't know how to benchmark cohttp, so it would help if someone else could do that.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I have benchmarked the latency of cohttp using https://github.com/ocaml-multicore/retro-httpaf-bench/tree/master/cohttp-lwt-unix and, with the new headers module, the measures are ~33% slower on my machine.
With this change, we seem to get ~10% slowdown compared to the old metrics on my laptop. Still suboptimal but it is an improvement.
Signed-off-by: Marcello Seri [email protected]