Skip to content

Add support for selecting the libev backend#269

Closed
yallop wants to merge 2 commits intoocsigen:masterfrom
yallop:libev-backends
Closed

Add support for selecting the libev backend#269
yallop wants to merge 2 commits intoocsigen:masterfrom
yallop:libev-backends

Conversation

@yallop
Copy link
Copy Markdown
Contributor

@yallop yallop commented Sep 8, 2016

This pull request adds support for selecting the backend (poll, select, kqueue, etc.) used by the libev engine. (The libev man page has some fairly forthright notes about the tradeoffs between the various backends.)

The backends are exposed as members of an abstract type, Ev_backend.t in the Lwt engine module:

module Ev_backend :
sig
  type t
  val default : t
  val select : t
  val poll : t
  val epoll : t
  val kqueue : t
  val devpoll : t
  val port : t
end

Keeping the type abstract leaves open the possibility to extend the interface in a backwards-compatible way in the future. For example, it is sometimes useful to pass multiple backend flags, leaving libev to select the best backend from those available; this could be supported in Lwt by the addition of a function of type Ev_backend.t -> Ev_backend.t -> Ev_backend.t. For the moment there is only support for passing a single backend.

The backend is passed as an argument to the libev class:

class libev : ?backend:Ev_backend.t -> unit -> object

The addition of arguments to libev is a backwards incompatible change, and client code must be changed to add a unit argument:

new libev
new libev ()

However, this is a one-time cost: if the libev interface is extended with additional optional flags in the future, no further changes to client code will be needed.

@yallop yallop mentioned this pull request Sep 8, 2016
Closed
@aantron
Copy link
Copy Markdown
Collaborator

aantron commented Sep 12, 2016 •
edited
Loading

Thanks, looks good.

I have no strong objection to making the type abstract, but why not just expose a (maybe polymorphic) variant, and type the argument to libev at a list of this variant? It seems like this would support whatever the C interface is capable of. It might also be more natural to work with in case Lwt ever wraps ev_recommended_backends, for example, and users want to filter or compare backends. Also, there is precedent in stdlib's Unix for this style.

I've fixed the problem with the CI builds, in master. They should work if you rebase.

@yallop
Copy link
Copy Markdown
Contributor Author

yallop commented Sep 13, 2016

but why not just expose a (maybe polymorphic) variant, and type the argument to libev at a list of this variant?

That's a reasonable way to do things, too. There are a couple of reasons why I prefer to use an abstract type:

  • If the list of backends libev supports changes in future, then the type of the polymorphic variant will change, which may break code that uses Lwt. (This could perhaps be avoided by making the type private.)
  • The set of backends passed to libev isn't really a list: there's no ordering and no way to represent duplicates.

That said, I don't mind switching to a list, if you prefer. Or, since the abstract interface is compatible with lists, there's always the option of switching later if you're happy to merge this as is.

avsm
avsm reviewed Sep 16, 2016
View reviewed changes
src/unix/lwt_engine.mli
val devpoll : t
val port : t
end

Copy link
Copy Markdown
Collaborator

@avsm avsm Sep 16, 2016

Choose a reason for hiding this comment

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

It would be convenient to have a pp function here to have a human-readable representation of the abstract type, for logging purposes.

Copy link
Copy Markdown
Contributor Author

@yallop yallop Sep 16, 2016

Choose a reason for hiding this comment

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

Good idea. I've added one in 268bc37.

@aantron aantron mentioned this pull request Oct 2, 2016
Closed
@aantron aantron force-pushed the master branch 8 times, most recently from 4a60522 to ffdb93f Compare October 4, 2016 15:06
This was referenced Nov 21, 2016
Closed
Merged
@aantron aantron closed this in #294 Nov 24, 2016
@aantron
Copy link
Copy Markdown
Collaborator

aantron commented Nov 24, 2016

Merged with trivial changes in #294. Thanks!

@aantron aantron mentioned this pull request Dec 22, 2016
Closed
@aantron aantron mentioned this pull request Jan 3, 2017
Closed
aantron added a commit that referenced this pull request Apr 8, 2017
@aantron
Breaking: switch to new Lwt_engine.libev
89565da 
Originally added in #269.
aantron added a commit that referenced this pull request Apr 9, 2017
@aantron
Breaking: switch to new Lwt_engine.libev
92b408c 
Originally added in #269.
aantron added a commit that referenced this pull request Apr 10, 2017
@aantron
Breaking: switch to new Lwt_engine.libev
89d1a60 
Originally added in #269.
@aantron aantron mentioned this pull request Jun 27, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avsm avsm avsm left review comments

Assignees

No one assigned

Labels

breaking

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yallop @aantron @avsm