ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
548 stars 66 forks source link

Replace objects with variants #553

Closed talex5 closed 1 year ago

talex5 commented 1 year ago

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.

darrenldl commented 1 year ago

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

SGrondin commented 1 year ago

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

talex5 commented 1 year ago

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.

favonia commented 1 year ago

@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 commented 1 year ago

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

aryx commented 10 months ago

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 commented 10 months ago

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 ...