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

Replace objects with variants #553

Merged
merged 1 commit into from
Aug 10, 2023
Merged

Conversation

talex5
Copy link
Collaborator

@talex5 talex5 commented Jun 8, 2023

Jane Street and other users have requested that Eio not use objects. This PR proposes a new scheme for representing OS resources using variants instead. The changes for users of the library are minimal - only the types change. The exception to this is if you want to provide your own implementations of resources, in which case you now provide a module rather than a class. The (small) changes to the README give a good idea of the user-facing effect.

An 'a Resource.t is a resource implementing at least the interfaces in 'a. e.g. [`Close | `Shutdown] Resource.t is a resource that can be closed and shut down. This should also give slightly shorter error messages, since we only list interfaces, not all the methods and their types.

I added type 'a r = 'a Resource.t to Eio.Std to keep the new types fairly short, plus various aliases. e.g. we have:

type source_ty = [`R | `Flow]
type 'a source = ([> source_ty] as 'a) r

With that, the signature of Flow.copy changes like this:

-val copy : #source -> #sink -> unit
+val copy : _ source -> _ sink -> unit

Internally, there are lots of changes. For now, I have only updated the flow and socket types. These are the most complicated types as they have lots of sub-types (source, sink, close, shutdown, ro-file, rw-file, unix, etc), and are used in lots of places (random numbers, buffered reading and writing, processes). I think changing over the remaining types will be straight-forward.

As with objects, calling an operation does not require any allocation, and initial benchmarks show no noticable change in performance (unsurprisingly, since method dispatch is very fast compared to a system call). More optimisations are possible if necessary.

@talex5 talex5 added the api API design decision label Jun 8, 2023
This was referenced Jun 8, 2023
@darrenldl
Copy link

Did Jane Street cite any specific reasons? (Just out of curiosity.)

@SGrondin
Copy link
Collaborator

I'm assuming Eio users should use Eio.Flow.single_read src instead of src#read_into to future-proof their code?

@talex5
Copy link
Collaborator Author

talex5 commented Jun 20, 2023

I'm assuming Eio users should use Eio.Flow.single_read src instead of src#read_into to future-proof their code?

Yes, that's right. It's a good idea anyway as you get extra checks and better compiler errors.

@talex5 talex5 mentioned this pull request Jul 7, 2023
@talex5 talex5 force-pushed the variants branch 17 times, most recently from 1c1f35f to 4d629ff Compare August 9, 2023 12:49
@talex5 talex5 marked this pull request as ready for review August 9, 2023 12:51
@talex5 talex5 force-pushed the variants branch 3 times, most recently from 4c526e4 to f6bde9a Compare August 10, 2023 08:39
Jane Street have requested that Eio not use objects. This commit
switches to an alternative scheme for representing OS resources using
variants instead. The changes for users of the library are minimal -
only the types change. The exception to this is if you want to provide
your own implementations of resources, in which case you now provide a
module rather than a class. The (small) changes to the README give a
good idea of the user-facing effect.
@talex5 talex5 merged commit bb47407 into ocaml-multicore:main Aug 10, 2023
4 checks passed
@talex5 talex5 deleted the variants branch August 10, 2023 13:58
talex5 added a commit to talex5/eio that referenced this pull request Aug 17, 2023
talex5 added a commit to talex5/eio that referenced this pull request Aug 17, 2023
talex5 added a commit to talex5/eio that referenced this pull request Aug 17, 2023
avsm pushed a commit to patricoferris/eio that referenced this pull request Aug 26, 2023
talex5 added a commit to talex5/opam-repository that referenced this pull request Aug 29, 2023
CHANGES:

New features / API changes:

- Replace objects with variants (@talex5 @patricoferris ocaml-multicore/eio#553 ocaml-multicore/eio#605 ocaml-multicore/eio#608, reviewed by @avsm).
  Some potential users found object types confusing, so we now use an alternative scheme for OS resources.
  For users of the resources, the only thing that changes is the types:

  - Instead of taking an argument of type `#foo`, you should now take `_ foo`.
  - Instead of returning a value of type `foo`, you should now return `foo_ty Eio.Resource.t`.

  To provide your own implementation of an interface, you now provide a module rather than an object.
  For example, to provide your own source flow, use `Eio.Flow.Pi.source (module My_source)`.

  If you want to define your own interfaces, see the `Eio.Resource` module documentation.

- Add `Eio.Pool` (@talex5 @darrenldl ocaml-multicore/eio#602, reviewed by @patricoferris).
  A lock-free pool of resources. This is similar to `Lwt_pool`.

- Add `Eio.Lazy` (@talex5 ocaml-multicore/eio#609, reviewed by @SGrondin).
  If one fiber tries to force a lazy value while another is already doing it,
  this will wait for the first one to finish rather than raising an exception (as `Stdlib.Lazy` does).

- Add `Eio.Path.native` (@talex5 ocaml-multicore/eio#603, reviewed by @patricoferris).
  This is useful when interacting with non-Eio libraries, for spawning sub-processes, and for displaying paths to users.

- Add `Flow.single_write` (@talex5 ocaml-multicore/eio#598).

- Add `Eio.Flow.Pi.simple_copy` (@talex5 ocaml-multicore/eio#611).
  Provides an easy way to implement the `copy` operation when making your own sink.

- Eio_unix: add FD passing (@talex5 ocaml-multicore/eio#522).
  Allows opening a file and passing the handle over a Unix-domain socket.

- Add `Process.run ?is_success` to control definition of success (@SGrondin ocaml-multicore/eio#586, reviewed by @talex5).

- Add `Eio_mock.Domain_manager` (@talex5 ocaml-multicore/eio#610).
  This mock domain manager runs everything in a single domain, allowing tests to remain deterministic.

- Add `Eio.Debug.with_trace_prefix` (@talex5 ocaml-multicore/eio#610).
  Allows prefixing all `traceln` output. The mock domain manager uses this to indicate which fake domain is running.

Bug fixes:

- Fork actions must not allocate (@talex5 ocaml-multicore/eio#593).
  When using multiple domains, child processes could get stuck if they forked while another domain held the malloc lock.

- eio_posix: ignore some errors writing to the wake-up pipe (@talex5 ocaml-multicore/eio#600).
  If the pipe is full or closed, the wake-up should simply be ignored.

Build/test fixes:

- Fix some MDX problems on Windows (@polytypic ocaml-multicore/eio#597).

- The README depends on kcas (@talex5 ocaml-multicore/eio#606).

- Clarify configuration for lib_eio_linux and enable tests on other arches (@dra27 ocaml-multicore/eio#592).

- eio_linux tests: skip fixed buffer test if not available (@talex5 ocaml-multicore/eio#604).

- eio_windows: update available line to win32 (@talex5 ocaml-multicore/eio#588 ocaml-multicore/eio#591).
talex5 added a commit to talex5/opam-repository that referenced this pull request Aug 29, 2023
CHANGES:

New features / API changes:

- Replace objects with variants (@talex5 @patricoferris ocaml-multicore/eio#553 ocaml-multicore/eio#605 ocaml-multicore/eio#608, reviewed by @avsm).
  Some potential users found object types confusing, so we now use an alternative scheme for OS resources.
  For users of the resources, the only thing that changes is the types:

  - Instead of taking an argument of type `#foo`, you should now take `_ foo`.
  - Instead of returning a value of type `foo`, you should now return `foo_ty Eio.Resource.t`.

  To provide your own implementation of an interface, you now provide a module rather than an object.
  For example, to provide your own source flow, use `Eio.Flow.Pi.source (module My_source)`.

  If you want to define your own interfaces, see the `Eio.Resource` module documentation.

- Add `Eio.Pool` (@talex5 @darrenldl ocaml-multicore/eio#602, reviewed by @patricoferris).
  A lock-free pool of resources. This is similar to `Lwt_pool`.

- Add `Eio.Lazy` (@talex5 ocaml-multicore/eio#609, reviewed by @SGrondin).
  If one fiber tries to force a lazy value while another is already doing it,
  this will wait for the first one to finish rather than raising an exception (as `Stdlib.Lazy` does).

- Add `Eio.Path.native` (@talex5 ocaml-multicore/eio#603, reviewed by @patricoferris).
  This is useful when interacting with non-Eio libraries, for spawning sub-processes, and for displaying paths to users.

- Add `Flow.single_write` (@talex5 ocaml-multicore/eio#598).

- Add `Eio.Flow.Pi.simple_copy` (@talex5 ocaml-multicore/eio#611).
  Provides an easy way to implement the `copy` operation when making your own sink.

- Eio_unix: add FD passing (@talex5 ocaml-multicore/eio#522).
  Allows opening a file and passing the handle over a Unix-domain socket.

- Add `Process.run ?is_success` to control definition of success (@SGrondin ocaml-multicore/eio#586, reviewed by @talex5).

- Add `Eio_mock.Domain_manager` (@talex5 ocaml-multicore/eio#610).
  This mock domain manager runs everything in a single domain, allowing tests to remain deterministic.

- Add `Eio.Debug.with_trace_prefix` (@talex5 ocaml-multicore/eio#610).
  Allows prefixing all `traceln` output. The mock domain manager uses this to indicate which fake domain is running.

Bug fixes:

- Fork actions must not allocate (@talex5 ocaml-multicore/eio#593).
  When using multiple domains, child processes could get stuck if they forked while another domain held the malloc lock.

- eio_posix: ignore some errors writing to the wake-up pipe (@talex5 ocaml-multicore/eio#600).
  If the pipe is full or closed, the wake-up should simply be ignored.

Build/test fixes:

- Fix some MDX problems on Windows (@polytypic ocaml-multicore/eio#597).

- The README depends on kcas (@talex5 ocaml-multicore/eio#606).

- Clarify configuration for lib_eio_linux and enable tests on other arches (@dra27 ocaml-multicore/eio#592).

- eio_linux tests: skip fixed buffer test if not available (@talex5 ocaml-multicore/eio#604).

- eio_windows: update available line to win32 (@talex5 ocaml-multicore/eio#588 ocaml-multicore/eio#591).
@favonia
Copy link

favonia commented Aug 31, 2023

@talex5 As someone who is already using eio, I found the old OO interface easier to use, and hope the developers may reconsider it before finalizing the API. For example, to save a sink in an OCaml record, instead of writing

output : Eio_unix.sink

I now have to write one of these:

output : Eio_unix.sink_ty Eio.Std.r
output : Eio_unix.sink_ty Eio_unix.sink
output : 'a Eio_unix.sink (* and introduce a phantom variable *)

The OO approach elegantly hides the difference between sink_ty and [> sink_ty], and its syntax is better than first-class modules (which can also achieve the same thing). Overall I feel it gives the best API and hope the developers may reconsider it. Thank you.

@jonsterling
Copy link

Let me comment hat i fully agree with what @favonia said above. This is certainly an API quality regression.

@aryx
Copy link

aryx commented Oct 25, 2023

What is your answer @talex5 to this API quality regression?
Maybe OO was better than the alternatives? Why Jane street didn't want objects?

@aryx
Copy link

aryx commented Oct 25, 2023

From the PR:

... This requires using a GADT. However, GADT's don't support sub-typing.
To get around this, we use an extensible GADT to get the correct typing
(but which will raise an exception if the interface isn't supported),
and then wrap this with a polymorphic variant phantom type to help ensure
it is used correctly.

Seems far more complicated than using simple objects ...

nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

New features / API changes:

- Replace objects with variants (@talex5 @patricoferris ocaml-multicore/eio#553 ocaml-multicore/eio#605 ocaml-multicore/eio#608, reviewed by @avsm).
  Some potential users found object types confusing, so we now use an alternative scheme for OS resources.
  For users of the resources, the only thing that changes is the types:

  - Instead of taking an argument of type `#foo`, you should now take `_ foo`.
  - Instead of returning a value of type `foo`, you should now return `foo_ty Eio.Resource.t`.

  To provide your own implementation of an interface, you now provide a module rather than an object.
  For example, to provide your own source flow, use `Eio.Flow.Pi.source (module My_source)`.

  If you want to define your own interfaces, see the `Eio.Resource` module documentation.

- Add `Eio.Pool` (@talex5 @darrenldl ocaml-multicore/eio#602, reviewed by @patricoferris).
  A lock-free pool of resources. This is similar to `Lwt_pool`.

- Add `Eio.Lazy` (@talex5 ocaml-multicore/eio#609, reviewed by @SGrondin).
  If one fiber tries to force a lazy value while another is already doing it,
  this will wait for the first one to finish rather than raising an exception (as `Stdlib.Lazy` does).

- Add `Eio.Path.native` (@talex5 ocaml-multicore/eio#603, reviewed by @patricoferris).
  This is useful when interacting with non-Eio libraries, for spawning sub-processes, and for displaying paths to users.

- Add `Flow.single_write` (@talex5 ocaml-multicore/eio#598).

- Add `Eio.Flow.Pi.simple_copy` (@talex5 ocaml-multicore/eio#611).
  Provides an easy way to implement the `copy` operation when making your own sink.

- Eio_unix: add FD passing (@talex5 ocaml-multicore/eio#522).
  Allows opening a file and passing the handle over a Unix-domain socket.

- Add `Process.run ?is_success` to control definition of success (@SGrondin ocaml-multicore/eio#586, reviewed by @talex5).

- Add `Eio_mock.Domain_manager` (@talex5 ocaml-multicore/eio#610).
  This mock domain manager runs everything in a single domain, allowing tests to remain deterministic.

- Add `Eio.Debug.with_trace_prefix` (@talex5 ocaml-multicore/eio#610).
  Allows prefixing all `traceln` output. The mock domain manager uses this to indicate which fake domain is running.

Bug fixes:

- Fork actions must not allocate (@talex5 ocaml-multicore/eio#593).
  When using multiple domains, child processes could get stuck if they forked while another domain held the malloc lock.

- eio_posix: ignore some errors writing to the wake-up pipe (@talex5 ocaml-multicore/eio#600).
  If the pipe is full or closed, the wake-up should simply be ignored.

Build/test fixes:

- Fix some MDX problems on Windows (@polytypic ocaml-multicore/eio#597).

- The README depends on kcas (@talex5 ocaml-multicore/eio#606).

- Clarify configuration for lib_eio_linux and enable tests on other arches (@dra27 ocaml-multicore/eio#592).

- eio_linux tests: skip fixed buffer test if not available (@talex5 ocaml-multicore/eio#604).

- eio_windows: update available line to win32 (@talex5 ocaml-multicore/eio#588 ocaml-multicore/eio#591).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API design decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants