cohttp-eio: use Eio.Buf_read

[talex5]

Eio now has its own buffered reader, so cohttp can just use that. This saves a lot of code, avoids embedding another parser combinator library in cohttp, and makes the public API much smaller.

I tested it with wrk -t 4 -c 1000 -d 5s http://127.0.0.1:8080 while running the example server, and performance seems to be unaffected.

I also changed the on_error handler to not say the error happened on accept, since it's actually used for errors after accept, and to output a newline after the end. Perhaps it should instead be user configurable, or use a logging library instead of writing directly to stderr.

read_fixed now returns a string instead of bytes, which seems more logical. It also avoided a Bytes.unsafe_to_string in the example code.

The new parser doesn't support backtracking. I don't think this changes anything, but it would be worth double-checking we weren't relying on that anywhere.

I configured the reader with ~max_size:max_int, which matches what the old code did, but we might want to make that configurable.

/cc @bikallem

[avsm]

It would be lovely if this buffer could be reused somehow by the parsing fibre...

[talex5]

It's not very difficult, but we only use this for parsing chunk extensions (if any), so it has no noticeable performance impact and it makes the code a little harder to read.

[bikallem]

This was definitely in my to do list to evaluate Eio.Buf_read to replace the homegrown Reader module which is why I avoided using the back-tracking parser (<|>). If the performance is not affected, then this is definitely the way to go.

EDIT: my memory failed me here, I am still using <|> in when parsing the chunks

[bikallem]

The new parser doesn't support backtracking. I don't think this changes anything, but it would be worth double-checking we weren't relying on that anywhere.

Thinking more about this.

I haven't looked into implementation details of Eio.Buf_read yet but is Buf_read inherently not designed to support backtracking? i.e. can Buf_read implement support for backtracking <|> operator? I want to use the Reader in cohttp to write other protocols where backtracking support is crucial and/or is very convenient to have. For example, I want to port https://github.com/lemaetech/http-multipart-formdata to cohttp-eio. At the moment it uses Angstrom and more specifically its backtracking <|> operator in a few places (https://github.com/lemaetech/http-multipart-formdata/blob/master/lib/http_multipart_formdata.ml#L97-L104).

[talex5]

I haven't looked into implementation details of Eio.Buf_read yet but is Buf_read inherently not designed to support backtracking?

Yes, although there are ways to achieve it if necessary. But it's very uncommon for a network protocol to require backtracking, because it hurts performance. The example parser you linked (quoted_string) is also used in cohttp-eio, and was easy to convert to avoid backtracking.

Note that primitive parsers in Eio are atomic, so you could do things like string "bar" <|> string "baz", but it won't work if the first branch needs to consume multiple tokens before realising it has gone wrong.

In theory, it would also be possible to export Buf_read's state directly into another parser (e.g. Angstrom), so you could switch. But I suspect it's not necessary. I think Sprios said he regretted the backtracking support a bit, as people often forget the commit and end up with space leaks.

[mseri]

I haven't looked into implementation details of Eio.Buf_read yet but is Buf_read inherently not designed to support backtracking? i.e. can Buf_read implement support for backtracking <|> operator?

Seems that there is a crucial point still open right now. I think we should not merge this until this is settled if the new interface prevents the introduction of interesting features in the future

[talex5]

Seems that there is a crucial point still open right now. I think we should not merge this until this is settled if the new interface prevents the introduction of interesting features in the future

If someone finds a use for it then there's no problem adding it. You can always call Buf_read.peek to grab the reader's buffer and use that directly in a new parser. Or you could wrap Buf_read and track the number of virtual bytes consumed, but only update the real counter on submit. Or just flush the remaining bytes into a new parser (that would require copying the buffered data, though).

But generally we want to discourage backtracking. Note that cohttp-lwt doesn't support backtracking either, as far as I can see.

I'd like to get this merged because I have more patches to submit after this one (I'm converting ocaml-ci's web UI to use cohttp-eio as a test).

[mseri]

Ping @bikallem

[bikallem]

@mseri I am working with @talex5 to resolve the backtracking issue in Buf_read.

@talex5 Can we do some benchmark performance vis a vis (https://github.com/ocaml-multicore/retro-httpaf-bench). I think it would be conclusive if we could compare the current parser with the Buf_read parser on both uring and non-uring(luv) setting. The current Reader caters for scenarios where the read syscall is expensive and it reduces such expense via over fetching in the buffer.

[talex5]

The current Reader caters for scenarios where the read syscall is expensive and it reduces such expense via over fetching in the buffer.

Eio.Buf_read does that too - that's the whole point of a buffered reader.

More benchmarks are welcome, but I already used retro-httpaf-bench for testing this. If you see something interesting when running it on your own machine, please post the results here.

Reader and Eio.Buf_read are both using the same approach (both are based on Angstrom). They both read into the space at the end of a bigarray and compress it from time to time.

However, getting the API right is more important that the implementation. In the unlikely case that the Eio parser is slower, we can just fix it in a point release. Shipping a custom parser API with cohttp-eio means we have to maintain that.

[talex5]

OK, I wrote a quick benchmark to compare them (just plain parsing, without the overhead of running an HTTP server). To run it:

git clone git@github.com:talex5/cohttp-reader-bench.git
cd cohttp-reader-bench
dune exec --profile=release -- ./bin/main.exe

The test is parsing the "hello \" world" test-case you suggested above many times, using the quoted_string parsers from before and after this PR. However, the Reader version doesn't work for me. I get:

Hello " world                      
+buf_read: took 1.39s to parse 9999999 items
Fatal error: exception Reader.Parse_failure("[char] expected '\"', got 'H'")

Does this parser work for you? Perhaps you could fix my code and try it?

I also tried a simpler test using take 4 as the parser (just set the benchmark variable at the start), and that gives:

$ dune exec --profile=release -- ./bin/main.exe
"Hel                               
+buf_read: took 0.71s to parse 39999999 items
"Hel
+reader: took 1.13s to parse 39999999 items
"Hel
+buf_read: took 0.67s to parse 39999999 items
"Hel
+reader: took 0.85s to parse 39999999 items

So, at least on my machine for that test, Eio.Buf_read seems to be somewhat faster.

[bikallem]

Shouldn't Hello " world be "Hello \" world" ?

So, at least on my machine for that test, Eio.Buf_read seems to be somewhat faster.

That's quite impressive. Thanks for the benchmark. However, what I meant was: can there be the same rigor like we(both) did when we evaluated various HTTP libs in retro-http repo. I would like to see the benchmark of the whole request processing loop and the resulting chart of Reader vs Buf_read in various scenarios, 1 cpu vs multi cpu and so on.

[talex5]

That's quite impressive. Thanks for the benchmark.

So, I forgot to call commit in the benchmark and created a memory leak in the Reader case! I've fixed that now. It still crashes trying to parse a quoted string though.

To avoid that, I added the option to use my non-backtracking quoted string parser (from this PR), which doesn't trigger the Reader bug. With that, the speeds are about the same. The remaining differences seem to be mostly due to how much sanity-checking is done. e.g. Reader uses unsafe_get whereas Buf_read uses a checked get.

With the default Buf_read, Reader is slightly faster. But if I remove the sanity check on consume (to match Reader.incr_pos) then Buf_read is slightly faster.

But this is all irrelevant to this PR. It doesn't make sense to ship a slow parser in eio and then override it with a faster one in cohttp-eio. If there are performance problems then we should fix them in eio, and if we think the speed/safety tradeoff should be different then we should change it in eio.

[mseri]

I agree with @talex5, my point was more about the change preventing future development. If you say that, in case it is really needed, for example, backtracking can be added, I don't see a reason not to merge and address the optimizations at eio level

[bikallem]

If you say that, in case it is really needed, for example, backtracking can be added,

Excellent! <|> or alternate operator support added to Buf_read would be awesome. This would make implementing IETF RFCs in Buf_read a very productive endeavor without going through the ABNF to LL(1) left-factoring conversion. @talex5 could you please ping me once this support is added to Buf_read. It seems I am going to be quite occupied by the ocaml-dns eio client support work.

EDIT: @talex5 Apologies If I sounded a bit uncouth. What I meant was since you seemed to be in a bit of hurry to have this PR merged, perhaps you could explore the <|> support in Buf_read. I am keen in <|> support in Buf_read but it seems I am bit pressed for time at the moment as you are aware.

[bikallem]

It still crashes trying to parse a quoted string though.

It seems the parser is not finding the ". Hmmm ..

[bikallem]

Just want to add that I agree with the stated aim of this PR. If Buf_read - as a general purpose parser combinator library - can satisfy most parsing needs, then there is no reason for it not to be used. Additionally having <|> doesn't preclude one from implementing the LL(1) parser if one so desires. I think this is the flexibility that I was looking for in Reader. If Buf_read provides such flexibility then I am all for it, 100%. :)

[talex5]

I think all objections have been answered, so merging. If any problems are found with Buf_read, please report them in Eio.