Skip to content

Replace shuttle with http-async #24

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.

Sign up for GitHub

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

Jump to bottom
Merged
talex5 merged 3 commits into from
May 18, 2022
Merged

Replace shuttle with http-async #24

talex5 merged 3 commits into from
May 18, 2022

Conversation

@anuragsoni
Copy link
Contributor

@anuragsoni anuragsoni commented Apr 22, 2022 •
edited
Loading

I'm switching the shuttle implementation to use http_async which was published to opam recently. http_async uses shuttle internally.

image

The image is from a benchmark run under docker on my machine with RUN_DURATION=20

I don't think I can catch up to eio when it uses io-uring in polling mode, but I'm glad to see that the refactoring I did in shuttle (I switched to a higher level simpler API), hasn't resulted in my http server falling too far behind others :-)

anuragsoni
anuragsoni commented Apr 22, 2022
View reviewed changes
run_benchmarks.sh
Comment on lines +6 to +9
export GOMAXPROCS=1
export COHTTP_DOMAINS=1
export HTTPAF_EIO_DOMAINS=1
export RUST_CORES=1
Copy link
Contributor Author

@anuragsoni anuragsoni Apr 22, 2022

Choose a reason for hiding this comment

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

https://github.com/ocaml-multicore/retro-httpaf-bench/blob/6d7aaeac4a7d00be05b5f81d5ceead14de4facac/run_benchmarks_mult.sh exists for multicore benchmarks, so I switched the number of domains back to 1 in this script.

@talex5
Copy link
Collaborator

talex5 commented May 13, 2022

CI is failing with:

# File "cohttp-async/src/input_channel.mli", line 13, characters 16-34:
# 13 | val to_reader : Core_kernel.Info.t -> t -> Reader.t Deferred.t
#                      ^^^^^^^^^^^^^^^^^^
# Error: Unbound module Core_kernel

@talex5 talex5 mentioned this pull request May 13, 2022
Closed
@anuragsoni
Copy link
Contributor Author

anuragsoni commented May 13, 2022

@talex5 the Cohttp errors might be related to the 0.15 series of Janestreet libraries. Core_kernel was renamed to Core. I’m a little surprised to see cohttp_async in the errors as from what I can tell there are no cohttp_async examples in this repository.

@anuragsoni
Copy link
Contributor Author

anuragsoni commented May 13, 2022

Cohttp's current trunk works well with 0.15 series of janestreet libraries (mirage/ocaml-cohttp#867). I'm guessing the eio version of cohttp that's included in this repository as a submodule will need to be updated to sync with the new cohttp commits.

talex5 added 2 commits May 13, 2022 16:45
@talex5
Fix submodule URL
5537862 
Error was:

    Cloning into 'retro-httpaf-bench/cohttp-eio/vendor/ocaml-cohttp'...
    [email protected]: Permission denied (publickey).
    fatal: Could not read from remote repository.
@talex5
cohttp-lwt-unix: only install the packages we need
8c751da 
It was installing every package in the cohttp repository, bringing in a
dependency on async, etc.
@talex5
Copy link
Collaborator

talex5 commented May 15, 2022

I made a PR (#26) to fix the cohttp-lwt-unix build to avoid that. It looks like httpaf-shuttle-async has a similar problem, but maybe rebasing this PR on it will fix both.

@anuragsoni
Copy link
Contributor Author

anuragsoni commented May 16, 2022

Rebasing this PR on top of #26 should indeed help. The error in #26 is related to a breaking change in shuttle. Another option that should work is to pin shuttle to an older release as part of #26 shuttle.0.3.1 should work.

@anuragsoni anuragsoni force-pushed the replace-shuttle-example branch from 320b6b8 to f88be1d Compare May 18, 2022 03:06
@anuragsoni
Copy link
Contributor Author

anuragsoni commented May 18, 2022

@talex5 I have rebased this on top of #26 , and the build succeeds now.

@talex5 talex5 merged commit af0a3d4 into ocaml-multicore:master May 18, 2022
@talex5
Copy link
Collaborator

talex5 commented May 18, 2022

Thanks!

@anuragsoni anuragsoni deleted the replace-shuttle-example branch May 18, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@anuragsoni @talex5