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

Update for Eio 0.7 #952

Merged
merged 1 commit into from Dec 12, 2022
Merged

Update for Eio 0.7 #952

merged 1 commit into from Dec 12, 2022

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Dec 7, 2022

(once ocaml/opam-repository#22619 is merged)

/cc @bikallem

@bikallem
Copy link
Contributor

bikallem commented Dec 9, 2022

Oops. Sorry I didn't see this before opening mine (#954). That one has a one more commit (cfbbe49) than this one.

@talex5 talex5 marked this pull request as ready for review December 9, 2022 14:44
@mseri
Copy link
Collaborator

mseri commented Dec 9, 2022

Was cfbbe49 needed as well?

@bikallem
Copy link
Contributor

bikallem commented Dec 9, 2022

Not strictly necessary but nice to have, I think. Perhaps cherry pick it?

@talex5
Copy link
Contributor Author

talex5 commented Dec 9, 2022

Not strictly necessary but nice to have, I think. Perhaps cherry pick it?

What's the advantage of cfbbe49? The new code is a little longer. Is it faster? I would have thought that allocating the Failure exception wouldn't be any quicker than allocating an empty string, and installing an exception handler on the normal path would be a little slower?

@bikallem
Copy link
Contributor

bikallem commented Dec 9, 2022

What's the advantage of cfbbe49? The new code is a little longer. Is it faster? I would have thought that allocating the Failure exception wouldn't be any quicker than allocating an empty string, and installing an exception handler on the normal path would be a little slower?

I haven't done any bench-marking to verify if using exception handling is faster than allocation. However, my reasoning for the change is that exception handler in OCaml is very fast (since it is a register in a stack) and the overhead of a handler is that of a function call. Secondly, the code before was allocating an extra string in every call to token. This by itself is not much of an issue. However token is used quite a lot in the hot path (request parsing) - httips://github.com/mirage/ocaml-cohttp/blob/master/cohttp-eio/src/rwer.ml#L38, so therefore it is of some consequence if we can minimize allocation here.

  1. https://stackoverflow.com/questions/8564025/ocaml-internals-exceptions
  2. https://stackoverflow.com/questions/12160390/ocaml-performance-of-exceptions

@talex5
Copy link
Contributor Author

talex5 commented Dec 12, 2022

I suggest merging this as-is, as it fixes the compiler warning.

The other change can happen in a separate PR if it turns out to be useful.

@mseri mseri merged commit e3b3f1b into mirage:master Dec 12, 2022
mseri added a commit to mseri/opam-repository that referenced this pull request Apr 28, 2023
…p-curl, cohttp-eio, cohttp-lwt-jsoo, cohttp-lwt-unix, cohttp-lwt, cohttp-mirage, cohttp-server-lwt-unix, cohttp-top, cohttp and http (6.0.0~alpha1)

CHANGES:

- cohttp,cohttp-async server: correctly close broken streams (reported by Stéphane Glondu, fix by samhot and anuragsoni)
- cohttp-eio: remove unused code from tests to work with Eio 0.8 (talex5 mirage/ocaml-cohttp#967)
- Upgrade dune to v3.0 (bikallem mirage/ocaml-cohttp#947)
- cohttp-eio: allow client to optionally configure request pipelining (bikallem mirage/ocaml-cohttp#949)
- cohttp-eio: update to Eio 0.7 (talex5 mirage/ocaml-cohttp#952)
- cohttp-eio: update examples to use eio 0.7 primitives (bikallem mirage/ocaml-cohttp#957)
- cohttp-eio: generate Date header in responses (bikallem mirage/ocaml-cohttp#955)
- cohttp-eio: further improve Cohttp_eio.Client ergonomics (bikallem #?)
- cohttp-eio: server api improvements (bikallem mirage/ocaml-cohttp#962)
toots pushed a commit to savonet/opam-repository that referenced this pull request May 10, 2023
…p-curl, cohttp-eio, cohttp-lwt-jsoo, cohttp-lwt-unix, cohttp-lwt, cohttp-mirage, cohttp-server-lwt-unix, cohttp-top, cohttp and http (6.0.0~alpha1)

CHANGES:

- cohttp,cohttp-async server: correctly close broken streams (reported by Stéphane Glondu, fix by samhot and anuragsoni)
- cohttp-eio: remove unused code from tests to work with Eio 0.8 (talex5 mirage/ocaml-cohttp#967)
- Upgrade dune to v3.0 (bikallem mirage/ocaml-cohttp#947)
- cohttp-eio: allow client to optionally configure request pipelining (bikallem mirage/ocaml-cohttp#949)
- cohttp-eio: update to Eio 0.7 (talex5 mirage/ocaml-cohttp#952)
- cohttp-eio: update examples to use eio 0.7 primitives (bikallem mirage/ocaml-cohttp#957)
- cohttp-eio: generate Date header in responses (bikallem mirage/ocaml-cohttp#955)
- cohttp-eio: further improve Cohttp_eio.Client ergonomics (bikallem #?)
- cohttp-eio: server api improvements (bikallem mirage/ocaml-cohttp#962)
toots pushed a commit to savonet/opam-repository that referenced this pull request May 11, 2023
…p-curl, cohttp-eio, cohttp-lwt-jsoo, cohttp-lwt-unix, cohttp-lwt, cohttp-mirage, cohttp-server-lwt-unix, cohttp-top, cohttp and http (6.0.0~alpha1)

CHANGES:

- cohttp,cohttp-async server: correctly close broken streams (reported by Stéphane Glondu, fix by samhot and anuragsoni)
- cohttp-eio: remove unused code from tests to work with Eio 0.8 (talex5 mirage/ocaml-cohttp#967)
- Upgrade dune to v3.0 (bikallem mirage/ocaml-cohttp#947)
- cohttp-eio: allow client to optionally configure request pipelining (bikallem mirage/ocaml-cohttp#949)
- cohttp-eio: update to Eio 0.7 (talex5 mirage/ocaml-cohttp#952)
- cohttp-eio: update examples to use eio 0.7 primitives (bikallem mirage/ocaml-cohttp#957)
- cohttp-eio: generate Date header in responses (bikallem mirage/ocaml-cohttp#955)
- cohttp-eio: further improve Cohttp_eio.Client ergonomics (bikallem #?)
- cohttp-eio: server api improvements (bikallem mirage/ocaml-cohttp#962)
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.

None yet

3 participants