Add Buf_read.BE/LE parsers by Cjen1 · Pull Request #399 · ocaml-multicore/eio

Cjen1 · 2023-01-04T16:01:24Z

This PR adds the relevant parsers for integer primitives, equivalent to Buf_write.BE/LE to Buf_read.

It resolves #390.

Completed portions:

talex5

Looks good so far!

fuzz/fuzz_buf_read.ml

tests/buf_reader.md

Cjen1 · 2023-01-05T15:50:42Z

@talex5 This should be a completed PR.

I think that the uint48 implementation may not be fully correct but I'm testing to make sure now.
(I think Int64.of_int32 does not preserve the msb)

Cjen1 · 2023-01-05T16:28:29Z

I think that the uint48 implementation may not be fully correct but I'm testing to make sure now. (I think Int64.of_int32 does not preserve the msb)

It does not preserve the sign (or rather it preserves the value rather than the bits).
This is now fixed by doing everything in the int64 representation.

talex5

Looks right to me, but some simplifications/speedups are possible I think.

lib_eio/buf_read.mli

talex5 · 2023-01-06T08:49:49Z

lib_eio/buf_read.ml

+    let lower_16 = Bigstringaf.get_int16_le t.buf t.pos |> Int64.of_int in
+    let middle_16 = Bigstringaf.get_int16_le t.buf (t.pos + 2) |> Int64.of_int in


This might be better (haven't benchmarked it though):

Suggested change

let lower_16 = Bigstringaf.get_int16_le t.buf t.pos |> Int64.of_int in

let middle_16 = Bigstringaf.get_int16_le t.buf (t.pos + 2) |> Int64.of_int in

let lower_32 = Bigstringaf.get_int32_le t.buf t.pos |> Int64.of_int |> Int64.log_and 0xffffffffL in

Then the total is just Int64.(logor lower_32 (shift_left upper_16 32)).

I'm not sure this is correct for -1L since the Int64.of_int32 moves the sign bit such that the value is the same (but not the bits).
This was the correctness bug I was concerned about previously.

logor may be faster though than just adding.

It doesn't move the sign bit, it just replicates it into the extra bits (which is correct for signed numbers but not for unsigned ones). The logand gets rid of the extra copies:

# Int64.of_int32 0xe7654321l |> Printf.sprintf "0x%Lx";; - : string = "0xffffffffe7654321" # Int64.of_int32 0xe7654321l |> Int64.logand 0xffffffffL |> Printf.sprintf "0x%Lx";; - : string = "0xe7654321"

Good to know! I've left the fuzz model using the 3 * 16 bit one for validation.

fuzz/fuzz_buf_read.ml

talex5 · 2023-01-06T16:22:14Z

Thanks! I squashed and rebased it on top of #402 to avoid needing the Stdlib. prefix.

@bikallem

CHANGES: New features: - Add `Eio.Net.run_server` (@bikallem @talex5 ocaml-multicore/eio#408). Runs an accept loop in one or more domains, with cancellation and graceful shutdown, and an optional maximum number of concurrent connections. - Add `Buf_read.BE` and `LE` parsers (@Cjen1 ocaml-multicore/eio#399). Parse numbers in various binary formats. - Add `Eio.Buf_read.uint8` (@talex5 ocaml-multicore/eio#418). Performance: - Make `Eio.Condition` lock-free (@talex5 ocaml-multicore/eio#397 ocaml-multicore/eio#381). In addition to being faster, this allows using conditions in signal handlers. - Make `Eio.Semaphore` lock-free (@talex5 @polytypic ocaml-multicore/eio#398). - Make `Eio.Stream` lock-free when the capacity is zero (@talex5 ocaml-multicore/eio#413 ocaml-multicore/eio#411). - Make `Eio.Promise` lock-free (@talex5 ocaml-multicore/eio#401). Bug fixes: - eio_linux: call `Uring.submit` as needed (@talex5 @bikallem ocaml-multicore/eio#428). Previously, we could fail to submit a job promptly because the SQE queue was full. - Fix luv signals (@haesbaert ocaml-multicore/eio#412). `libuv` automatically retries polling if it gets `EINTR`, without giving OCaml signal handlers a chance to run. - eio_luv: fix some resource leaks (@talex5 @patricoferris ocaml-multicore/eio#421). - eio_luv: fix "unavailable signal" error on Windows (@talex5 ocaml-multicore/eio#420, reported by @nojb). - Fix `Buf_write.BE.uint48` and `LE.uint48` (@adatario ocaml-multicore/eio#418). Documentation: - Add example programs (@talex5 ocaml-multicore/eio#389). - Update network examples to use `run_server` (@talex5 ocaml-multicore/eio#417). - Add a warning to the tutorial about `Fiber.first` (@talex5 ocaml-multicore/eio#394). - Clarify the epoch used for `Eio.Time.now` (@bikallem ocaml-multicore/eio#395). - Describe `secure_random` as an infinite source (@patricoferris ocaml-multicore/eio#426). - Update README for OCaml 5 release (@talex5 ocaml-multicore/eio#384 ocaml-multicore/eio#391 ocaml-multicore/eio#393). Other changes: - Delay setting `SIGPIPE` handler until the `run` function is called (@talex5 ocaml-multicore/eio#420). - Remove debug-level logging (@talex5 ocaml-multicore/eio#403). - eio-luv: improve `process.md` test (@smondet ocaml-multicore/eio#414). - Update to Dune 3 (@talex5 ocaml-multicore/eio#410). - Remove test dependency on Astring (@talex5 ocaml-multicore/eio#402 ocaml-multicore/eio#404). - Simplify cancellation logic (@talex5 ocaml-multicore/eio#396). - time: `Mtime.Spand.to_s` has been deprecated in mtime 2.0.0 (@bikallem ocaml-multicore/eio#385).

@bikallem

CHANGES: New features: - Add `Eio.Net.run_server` (@bikallem @talex5 ocaml-multicore/eio#408). Runs an accept loop in one or more domains, with cancellation and graceful shutdown, and an optional maximum number of concurrent connections. - Add `Buf_read.BE` and `LE` parsers (@Cjen1 ocaml-multicore/eio#399). Parse numbers in various binary formats. - Add `Eio.Buf_read.uint8` (@talex5 ocaml-multicore/eio#418). Performance: - Make `Eio.Condition` lock-free (@talex5 ocaml-multicore/eio#397 ocaml-multicore/eio#381). In addition to being faster, this allows using conditions in signal handlers. - Make `Eio.Semaphore` lock-free (@talex5 @polytypic ocaml-multicore/eio#398). - Make `Eio.Stream` lock-free when the capacity is zero (@talex5 ocaml-multicore/eio#413 ocaml-multicore/eio#411). - Make `Eio.Promise` lock-free (@talex5 ocaml-multicore/eio#401). Bug fixes: - eio_linux: call `Uring.submit` as needed (@talex5 @bikallem ocaml-multicore/eio#428). Previously, we could fail to submit a job promptly because the SQE queue was full. - Fix luv signals (@haesbaert ocaml-multicore/eio#412). `libuv` automatically retries polling if it gets `EINTR`, without giving OCaml signal handlers a chance to run. - eio_luv: fix some resource leaks (@talex5 @patricoferris ocaml-multicore/eio#421). - eio_luv: fix "unavailable signal" error on Windows (@talex5 ocaml-multicore/eio#420, reported by @nojb). - Fix `Buf_write.BE.uint48` and `LE.uint48` (@adatario ocaml-multicore/eio#418). Documentation: - Add example programs (@talex5 ocaml-multicore/eio#389). - Update network examples to use `run_server` (@talex5 ocaml-multicore/eio#417). - Add a warning to the tutorial about `Fiber.first` (@talex5 ocaml-multicore/eio#394). - Clarify the epoch used for `Eio.Time.now` (@bikallem ocaml-multicore/eio#395). - Describe `secure_random` as an infinite source (@patricoferris ocaml-multicore/eio#426). - Update README for OCaml 5 release (@talex5 ocaml-multicore/eio#384 ocaml-multicore/eio#391 ocaml-multicore/eio#393). Other changes: - Delay setting `SIGPIPE` handler until the `run` function is called (@talex5 ocaml-multicore/eio#420). - Remove debug-level logging (@talex5 ocaml-multicore/eio#403). - eio-luv: improve `process.md` test (@smondet ocaml-multicore/eio#414). - Update to Dune 3 (@talex5 ocaml-multicore/eio#410). - Remove test dependency on Astring (@talex5 ocaml-multicore/eio#402 ocaml-multicore/eio#404). - Simplify cancellation logic (@talex5 ocaml-multicore/eio#396). - time: `Mtime.Spand.to_s` has been deprecated in mtime 2.0.0 (@bikallem ocaml-multicore/eio#385).

@bikallem

CHANGES: New features: - Add `Eio.Net.run_server` (@bikallem @talex5 ocaml-multicore/eio#408). Runs an accept loop in one or more domains, with cancellation and graceful shutdown, and an optional maximum number of concurrent connections. - Add `Buf_read.BE` and `LE` parsers (@Cjen1 ocaml-multicore/eio#399). Parse numbers in various binary formats. - Add `Eio.Buf_read.uint8` (@talex5 ocaml-multicore/eio#418). Performance: - Make `Eio.Condition` lock-free (@talex5 ocaml-multicore/eio#397 ocaml-multicore/eio#381). In addition to being faster, this allows using conditions in signal handlers. - Make `Eio.Semaphore` lock-free (@talex5 @polytypic ocaml-multicore/eio#398). - Make `Eio.Stream` lock-free when the capacity is zero (@talex5 ocaml-multicore/eio#413 ocaml-multicore/eio#411). - Make `Eio.Promise` lock-free (@talex5 ocaml-multicore/eio#401). Bug fixes: - eio_linux: call `Uring.submit` as needed (@talex5 @bikallem ocaml-multicore/eio#428). Previously, we could fail to submit a job promptly because the SQE queue was full. - Fix luv signals (@haesbaert ocaml-multicore/eio#412). `libuv` automatically retries polling if it gets `EINTR`, without giving OCaml signal handlers a chance to run. - eio_luv: fix some resource leaks (@talex5 @patricoferris ocaml-multicore/eio#421). - eio_luv: fix "unavailable signal" error on Windows (@talex5 ocaml-multicore/eio#420, reported by @nojb). - Fix `Buf_write.BE.uint48` and `LE.uint48` (@adatario ocaml-multicore/eio#418). Documentation: - Add example programs (@talex5 ocaml-multicore/eio#389). - Update network examples to use `run_server` (@talex5 ocaml-multicore/eio#417). - Add a warning to the tutorial about `Fiber.first` (@talex5 ocaml-multicore/eio#394). - Clarify the epoch used for `Eio.Time.now` (@bikallem ocaml-multicore/eio#395). - Describe `secure_random` as an infinite source (@patricoferris ocaml-multicore/eio#426). - Update README for OCaml 5 release (@talex5 ocaml-multicore/eio#384 ocaml-multicore/eio#391 ocaml-multicore/eio#393). Other changes: - Delay setting `SIGPIPE` handler until the `run` function is called (@talex5 ocaml-multicore/eio#420). - Remove debug-level logging (@talex5 ocaml-multicore/eio#403). - eio-luv: improve `process.md` test (@smondet ocaml-multicore/eio#414). - Update to Dune 3 (@talex5 ocaml-multicore/eio#410). - Remove test dependency on Astring (@talex5 ocaml-multicore/eio#402 ocaml-multicore/eio#404). - Simplify cancellation logic (@talex5 ocaml-multicore/eio#396). - time: `Mtime.Spand.to_s` has been deprecated in mtime 2.0.0 (@bikallem ocaml-multicore/eio#385).

@bikallem

CHANGES: New features: - Add `Eio.Net.run_server` (@bikallem @talex5 ocaml-multicore/eio#408). Runs an accept loop in one or more domains, with cancellation and graceful shutdown, and an optional maximum number of concurrent connections. - Add `Buf_read.BE` and `LE` parsers (@Cjen1 ocaml-multicore/eio#399). Parse numbers in various binary formats. - Add `Eio.Buf_read.uint8` (@talex5 ocaml-multicore/eio#418). Performance: - Make `Eio.Condition` lock-free (@talex5 ocaml-multicore/eio#397 ocaml-multicore/eio#381). In addition to being faster, this allows using conditions in signal handlers. - Make `Eio.Semaphore` lock-free (@talex5 @polytypic ocaml-multicore/eio#398). - Make `Eio.Stream` lock-free when the capacity is zero (@talex5 ocaml-multicore/eio#413 ocaml-multicore/eio#411). - Make `Eio.Promise` lock-free (@talex5 ocaml-multicore/eio#401). Bug fixes: - eio_linux: call `Uring.submit` as needed (@talex5 @bikallem ocaml-multicore/eio#428). Previously, we could fail to submit a job promptly because the SQE queue was full. - Fix luv signals (@haesbaert ocaml-multicore/eio#412). `libuv` automatically retries polling if it gets `EINTR`, without giving OCaml signal handlers a chance to run. - eio_luv: fix some resource leaks (@talex5 @patricoferris ocaml-multicore/eio#421). - eio_luv: fix "unavailable signal" error on Windows (@talex5 ocaml-multicore/eio#420, reported by @nojb). - Fix `Buf_write.BE.uint48` and `LE.uint48` (@adatario ocaml-multicore/eio#418). Documentation: - Add example programs (@talex5 ocaml-multicore/eio#389). - Update network examples to use `run_server` (@talex5 ocaml-multicore/eio#417). - Add a warning to the tutorial about `Fiber.first` (@talex5 ocaml-multicore/eio#394). - Clarify the epoch used for `Eio.Time.now` (@bikallem ocaml-multicore/eio#395). - Describe `secure_random` as an infinite source (@patricoferris ocaml-multicore/eio#426). - Update README for OCaml 5 release (@talex5 ocaml-multicore/eio#384 ocaml-multicore/eio#391 ocaml-multicore/eio#393). Other changes: - Delay setting `SIGPIPE` handler until the `run` function is called (@talex5 ocaml-multicore/eio#420). - Remove debug-level logging (@talex5 ocaml-multicore/eio#403). - eio-luv: improve `process.md` test (@smondet ocaml-multicore/eio#414). - Update to Dune 3 (@talex5 ocaml-multicore/eio#410). - Remove test dependency on Astring (@talex5 ocaml-multicore/eio#402 ocaml-multicore/eio#404). - Simplify cancellation logic (@talex5 ocaml-multicore/eio#396). - time: `Mtime.Spand.to_s` has been deprecated in mtime 2.0.0 (@bikallem ocaml-multicore/eio#385).

talex5 reviewed Jan 5, 2023

View reviewed changes

fuzz/fuzz_buf_read.ml Outdated Show resolved Hide resolved

tests/buf_reader.md Outdated Show resolved Hide resolved

Cjen1 marked this pull request as ready for review January 5, 2023 13:04

Cjen1 marked this pull request as draft January 5, 2023 15:50

Cjen1 marked this pull request as ready for review January 5, 2023 16:27

talex5 reviewed Jan 6, 2023

View reviewed changes

talex5 mentioned this pull request Jan 6, 2023

Remove test dependency on Astring #402

Merged

Add big and little endian parsers

9998c86

talex5 force-pushed the add-bigstringaf-ser branch from f892b4c to 9998c86 Compare January 6, 2023 16:21

talex5 approved these changes Jan 6, 2023

View reviewed changes

talex5 merged commit dfa3076 into ocaml-multicore:main Jan 6, 2023

talex5 mentioned this pull request Feb 1, 2023

[new release] eio_main, eio_luv, eio_linux and eio (0.8) ocaml/opam-repository#23208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Buf_read.BE/LE parsers#399

Add Buf_read.BE/LE parsers#399
talex5 merged 1 commit intoocaml-multicore:mainfrom
Cjen1:add-bigstringaf-ser

Cjen1 commented Jan 4, 2023 •

edited

Loading

Uh oh!

talex5 left a comment

Uh oh!

Uh oh!

Uh oh!

Cjen1 commented Jan 5, 2023

Uh oh!

Cjen1 commented Jan 5, 2023

Uh oh!

talex5 left a comment

Uh oh!

Uh oh!

talex5 Jan 6, 2023

Uh oh!

Cjen1 Jan 6, 2023 •

edited

Loading

Uh oh!

talex5 Jan 6, 2023

Uh oh!

Cjen1 Jan 6, 2023

Uh oh!

Uh oh!

talex5 commented Jan 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let lower_16 = Bigstringaf.get_int16_le t.buf t.pos \|> Int64.of_int in
		let middle_16 = Bigstringaf.get_int16_le t.buf (t.pos + 2) \|> Int64.of_int in

	let lower_16 = Bigstringaf.get_int16_le t.buf t.pos \|> Int64.of_int in
	let middle_16 = Bigstringaf.get_int16_le t.buf (t.pos + 2) \|> Int64.of_int in
	let lower_32 = Bigstringaf.get_int32_le t.buf t.pos \|> Int64.of_int \|> Int64.log_and 0xffffffffL in

Conversation

Cjen1 commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Cjen1 commented Jan 5, 2023

Uh oh!

Cjen1 commented Jan 5, 2023

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

talex5 Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

Cjen1 Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talex5 Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

Cjen1 Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

talex5 commented Jan 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Cjen1 commented Jan 4, 2023 •

edited

Loading

Cjen1 Jan 6, 2023 •

edited

Loading