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.

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

add Gzip_io.string_lwt #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add Gzip_io.string_lwt #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
022a17c
add Gzip_io.string_lwt
tatchi May 6, 2024
ba084d4
more efficient compression
tatchi May 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dune
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
(preprocess
(per_module
((pps lwt_ppx)
gzip_io
httpev
logstash
lwt_flag
Expand Down
17 changes: 17 additions & 0 deletions gzip_io.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,23 @@ let string s =
IO.nwrite out (Bytes.unsafe_of_string s); (* IO wrong type *)
IO.close_out out

let string_lwt ?(chunk_size = 3000) ?(yield = Lwt.pause) s =
Copy link

Choose a reason for hiding this comment

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

I've realized that Gzip_stream imposes a 1024-byte buffer size, so we should not give the illusion of control.
i.e. can use chunk_size = 4096 or whatever internally, so that there's no problem if buffer_size were changed in Gzip_stream, but not allow to pass ?chunk_size since it'll normally be ineffective.

let out = output (IO.output_string ()) in
let b = Bytes.unsafe_of_string s in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this ok to use unsafe_of_string here ?

Copy link

Choose a reason for hiding this comment

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

AFAICS it's only read from so so it's fine.

It's a bit strange IO.output expects a Bytes.t though.

let len = Bytes.length b in
let rec loop offset =
let written =
let len_to_write = Int.min chunk_size (len - offset) in
IO.output out b offset len_to_write in
if offset + written >= len then Lwt.return_unit
else (
(* Yield after processing a chunk *)
let%lwt () = yield () in
Copy link

Choose a reason for hiding this comment

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

With a buffer of size 1024 we can compress maybe around 30000 chunks/s, which means yielding every ~30 us. At this point the overhead of returning to the event loop could start to become measurable. So yielding after processing around say 4096 bytes could be better. Probably not a big deal though.

(This depends on the cost of the switch, which I guesstimated at a few us...)

loop (offset + written))
in
let%lwt () = loop 0 in
Lwt.return @@ IO.close_out out

let to_string s =
let inp = input (IO.input_string s) in
let out = IO.output_string () in
Expand Down
8 changes: 5 additions & 3 deletions httpev.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,12 +928,14 @@ let send_reply c cout reply =
end
in
(* possibly apply encoding *)
let (hdrs,body) =
let%lwt (hdrs,body) =
(* TODO do not apply encoding to application/gzip *)
(* TODO gzip + chunked? *)
match body, code, c.req with
| `Body s, `Ok, Ready { encoding=Gzip; _ } when String.length s > 128 -> ("Content-Encoding", "gzip")::hdrs, `Body (Gzip_io.string s)
| _ -> hdrs, body
| `Body s, `Ok, Ready { encoding=Gzip; _ } when String.length s > 128 ->
Copy link

Choose a reason for hiding this comment

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

We allocate a 1024-byte buffer in Gzip_stream, so maybe a higher threshold makes sense (it feels a bit silly to go ahead and allocate 1K for only 129 bytes of response).

let%lwt body = Gzip_io.string_lwt s in
Lwt.return (("Content-Encoding", "gzip")::hdrs, `Body body)
| _ -> Lwt.return (hdrs, body)
in
let hdrs = match body with
| `Body s -> ("Content-Length", string_of_int (String.length s)) :: hdrs
Expand Down