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

Optimise Flow.copy with Buf_read.as_flow #663

Merged
merged 2 commits into from
Jan 2, 2024
Merged

Optimise Flow.copy with Buf_read.as_flow #663

merged 2 commits into from
Jan 2, 2024

Conversation

talex5
Copy link
Collaborator

@talex5 talex5 commented Dec 31, 2023 •
edited
Loading

By default, Flow.copy creates a 4KB buffer and copies the data through that. However, if the source of the copy is a buffered reader then it is much more efficient to use its buffer directly.

This updates the flow you get from Buf_read.as_flow to offer this optimisation, and also updates the eio_posix backend's flow to use this optimisation when available (eio_linux already supported this).

Detected in a benchmark by @leostera.

Possibly we should increase the default buffer size too.

This PR is made of two commits. The first just adds a benchmark to show the effect of the change. On my Linux machine, I get:

buf_read: 1091.79 MB/s   # Before
buf_read: 3306.98 MB/s   # After

@talex5 talex5 added the enhancement New feature or request label Dec 31, 2023
@talex5 talex5 changed the title Add Flow.copy benchmark Optimise Flow.copy with Buf_read.as_flow Dec 31, 2023
@talex5 talex5 marked this pull request as ready for review December 31, 2023 10:42
@leostera
Copy link

Excited to see this! ✨

If I find any other sore spots I'll try and be a little more proactive opening issues or tagging y'all 🙏🏼 it's been a busy week over here.

And please feel more than welcome to bench against Riot, it may help me find bottlenecks like this too!

Happy new year team, keep kicking ass 👏🏼

@leostera leostera mentioned this pull request Dec 31, 2023
Closed
SGrondin
SGrondin approved these changes Jan 1, 2024
View reviewed changes
Copy link
Collaborator

@SGrondin SGrondin left a comment

Choose a reason for hiding this comment

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

I haven't fully explored this area of the code yet but this looks good to me!

@talex5
Optimise Flow.copy with Buf_read.as_flow
19c43d7 
By default, Flow.copy creates a 4KB buffer and copies the data through
that. However, if the source of the copy is a buffered reader then it is
much more efficient to use its buffer directly.

This updates the flow you get from `Buf_read.as_flow` to offer this
optimisation, and also updates the eio_posix backend's flow to use this
optimisation when available (eio_linux already supported this).

Detected in a benchmark by Leandro Ostera.
@talex5 talex5 merged commit b942dde into ocaml-multicore:main Jan 2, 2024
4 of 5 checks passed
@talex5 talex5 deleted the flow-buf branch January 2, 2024 10:29
talex5 added a commit to talex5/opam-repository that referenced this pull request Jan 17, 2024
@talex5
[new release] eio (5 packages) (0.14)
5340789 
CHANGES:

New features / API changes:

- Add `Eio.Executor_pool` (@SGrondin ocaml-multicore/eio#639, reviewed by @talex5).
  Provides an easy way to distribute jobs across domains.

- Add `Fiber.first ~combine` and `Fiber.n_any` (@SGrondin @talex5 ocaml-multicore/eio#587).
  Allows keeping both results in the case where multiple fibers succeed.

- Add `Eio_mock.Backend.run_full` with auto-advancing mock clock (@talex5 ocaml-multicore/eio#644, reviewed by @SGrondin).
  Simplifies testing of code using clocks.

- Add `Buf_write.printf` (@SGrondin @talex5 ocaml-multicore/eio#655).

- Add `Net.listening_addr` (@mefyl ocaml-multicore/eio#555, reviewed by @patricoferris @talex5).
  Useful to get the socket's address if the OS assigns it.

- Add `Promise.try_resolve` (@talex5 ocaml-multicore/eio#646).

- Remove `Cancel_hook_failed` exception (@talex5 ocaml-multicore/eio#640).
  Didn't seem to be used and broke dscheck.

Tracing:

- Improve tracing (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#656).
  Trace cancellation contexts and OS operations, and simplify API.

- Add labels to switches (@talex5 ocaml-multicore/eio#661, reviewed by @SGrondin).

- `Fiber.all`: use the parent fiber (@talex5 ocaml-multicore/eio#665, reviewed by @SGrondin).
  Cleans up the traces a bit.

Performance:

- Faster and simpler `Lf_queue` (@talex5 ocaml-multicore/eio#647, based on work by @polytypic).

- Optimise `Flow.copy` with `Buf_read.as_flow` (@talex5 ocaml-multicore/eio#663, reviewed by @SGrondin, reported by @leostera).

Bug fixes:

- Fix handling of very long IO vectors (@talex5 ocaml-multicore/eio#653, reported by @Cjen1).

- eio_posix: use `caml_enter_blocking_section` in more places (@talex5 ocaml-multicore/eio#654, reviewed by @SGrondin).

- eio_posix: work around `caml_unix_alloc_sockaddr` bug (@talex5 ocaml-multicore/eio#651).

- Remove default backtrace from `Switch.fail` (@talex5 ocaml-multicore/eio#664).

Documentation:

- Organise eio.mli better (@talex5 ocaml-multicore/eio#667).

- Fix quoting of quotes in process error messages (@talex5 ocaml-multicore/eio#666, reviewed by @SGrondin).

- Mention Path module in File and Fs documentation (@talex5 ocaml-multicore/eio#659, requested by @clecat).

- Minor documentation updates (@SGrondin @talex5 ocaml-multicore/eio#670).

Build / internals:

- Allow closing synchronous streams (@talex5 ocaml-multicore/eio#641, reviewed by @SGrondin).
  This isn't currently exposed in the public interface.

- Fix non-idempotent tests (@SGrondin ocaml-multicore/eio#662).

- eio_windows: add explicit fmt dependency (@talex5 ocaml-multicore/eio#643).
@talex5 talex5 mentioned this pull request Jan 17, 2024
Merged
talex5 added a commit to talex5/opam-repository that referenced this pull request Jan 17, 2024
@talex5
[new release] eio (5 packages) (0.14)
6eb34f3 
CHANGES:

New features / API changes:

- Add `Eio.Executor_pool` (@SGrondin ocaml-multicore/eio#639, reviewed by @talex5).
  Provides an easy way to distribute jobs across domains.

- Add `Fiber.first ~combine` and `Fiber.n_any` (@SGrondin @talex5 ocaml-multicore/eio#587).
  Allows keeping both results in the case where multiple fibers succeed.

- Add `Eio_mock.Backend.run_full` with auto-advancing mock clock (@talex5 ocaml-multicore/eio#644, reviewed by @SGrondin).
  Simplifies testing of code using clocks.

- Add `Buf_write.printf` (@SGrondin @talex5 ocaml-multicore/eio#655).

- Add `Net.listening_addr` (@mefyl ocaml-multicore/eio#555, reviewed by @patricoferris @talex5).
  Useful to get the socket's address if the OS assigns it.

- Add `Promise.try_resolve` (@talex5 ocaml-multicore/eio#646).

- Remove `Cancel_hook_failed` exception (@talex5 ocaml-multicore/eio#640).
  Didn't seem to be used and broke dscheck.

Tracing:

- Improve tracing (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#656).
  Trace cancellation contexts and OS operations, and simplify API.

- Add labels to switches (@talex5 ocaml-multicore/eio#661, reviewed by @SGrondin).

- `Fiber.all`: use the parent fiber (@talex5 ocaml-multicore/eio#665, reviewed by @SGrondin).
  Cleans up the traces a bit.

Performance:

- Faster and simpler `Lf_queue` (@talex5 ocaml-multicore/eio#647, based on work by @polytypic).

- Optimise `Flow.copy` with `Buf_read.as_flow` (@talex5 ocaml-multicore/eio#663, reviewed by @SGrondin, reported by @leostera).

Bug fixes:

- Fix handling of very long IO vectors (@talex5 ocaml-multicore/eio#653, reported by @Cjen1).

- eio_posix: use `caml_enter_blocking_section` in more places (@talex5 ocaml-multicore/eio#654, reviewed by @SGrondin).

- eio_posix: work around `caml_unix_alloc_sockaddr` bug (@talex5 ocaml-multicore/eio#651).

- Remove default backtrace from `Switch.fail` (@talex5 ocaml-multicore/eio#664).

Documentation:

- Organise eio.mli better (@talex5 ocaml-multicore/eio#667).

- Fix quoting of quotes in process error messages (@talex5 ocaml-multicore/eio#666, reviewed by @SGrondin).

- Mention Path module in File and Fs documentation (@talex5 ocaml-multicore/eio#659, requested by @clecat).

- Minor documentation updates (@SGrondin @talex5 ocaml-multicore/eio#670).

Build / internals:

- Allow closing synchronous streams (@talex5 ocaml-multicore/eio#641, reviewed by @SGrondin).
  This isn't currently exposed in the public interface.

- Fix non-idempotent tests (@SGrondin ocaml-multicore/eio#662).

- eio_windows: add explicit fmt dependency (@talex5 ocaml-multicore/eio#643).
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@talex5 @nberth
[new release] eio (5 packages) (0.14)
c39875a 
CHANGES:

New features / API changes:

- Add `Eio.Executor_pool` (@SGrondin ocaml-multicore/eio#639, reviewed by @talex5).
  Provides an easy way to distribute jobs across domains.

- Add `Fiber.first ~combine` and `Fiber.n_any` (@SGrondin @talex5 ocaml-multicore/eio#587).
  Allows keeping both results in the case where multiple fibers succeed.

- Add `Eio_mock.Backend.run_full` with auto-advancing mock clock (@talex5 ocaml-multicore/eio#644, reviewed by @SGrondin).
  Simplifies testing of code using clocks.

- Add `Buf_write.printf` (@SGrondin @talex5 ocaml-multicore/eio#655).

- Add `Net.listening_addr` (@mefyl ocaml-multicore/eio#555, reviewed by @patricoferris @talex5).
  Useful to get the socket's address if the OS assigns it.

- Add `Promise.try_resolve` (@talex5 ocaml-multicore/eio#646).

- Remove `Cancel_hook_failed` exception (@talex5 ocaml-multicore/eio#640).
  Didn't seem to be used and broke dscheck.

Tracing:

- Improve tracing (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#656).
  Trace cancellation contexts and OS operations, and simplify API.

- Add labels to switches (@talex5 ocaml-multicore/eio#661, reviewed by @SGrondin).

- `Fiber.all`: use the parent fiber (@talex5 ocaml-multicore/eio#665, reviewed by @SGrondin).
  Cleans up the traces a bit.

Performance:

- Faster and simpler `Lf_queue` (@talex5 ocaml-multicore/eio#647, based on work by @polytypic).

- Optimise `Flow.copy` with `Buf_read.as_flow` (@talex5 ocaml-multicore/eio#663, reviewed by @SGrondin, reported by @leostera).

Bug fixes:

- Fix handling of very long IO vectors (@talex5 ocaml-multicore/eio#653, reported by @Cjen1).

- eio_posix: use `caml_enter_blocking_section` in more places (@talex5 ocaml-multicore/eio#654, reviewed by @SGrondin).

- eio_posix: work around `caml_unix_alloc_sockaddr` bug (@talex5 ocaml-multicore/eio#651).

- Remove default backtrace from `Switch.fail` (@talex5 ocaml-multicore/eio#664).

Documentation:

- Organise eio.mli better (@talex5 ocaml-multicore/eio#667).

- Fix quoting of quotes in process error messages (@talex5 ocaml-multicore/eio#666, reviewed by @SGrondin).

- Mention Path module in File and Fs documentation (@talex5 ocaml-multicore/eio#659, requested by @clecat).

- Minor documentation updates (@SGrondin @talex5 ocaml-multicore/eio#670).

Build / internals:

- Allow closing synchronous streams (@talex5 ocaml-multicore/eio#641, reviewed by @SGrondin).
  This isn't currently exposed in the public interface.

- Fix non-idempotent tests (@SGrondin ocaml-multicore/eio#662).

- eio_windows: add explicit fmt dependency (@talex5 ocaml-multicore/eio#643).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SGrondin SGrondin SGrondin approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@talex5 @leostera @SGrondin