add support for multiple backend (epoll, libevent...)

[aconchillo]

trafficstars

[civodul]

Bon dia @aconchillo, awesome work! :+1:

FWIW, I'm planning to use Fibers in the Shepherd, the init used on Guix System. It has to work on GNU/Hurd, which currently lacks epoll but has good'ol poll that libevent must be using there. In short: I'm very interested in seeing this merged. :-)

[emixa-d]

    printf("Max events fired. Ignoring\n");

I don't think that applications expect fibers to write to stdout. (current-error-port) would probably be fine though.

[emixa-d]

Also, can this ignoring cause some events to be missed?

[aconchillo]

Bon dia @aconchillo, awesome work! 👍

Hola! Thanks! @Habush was the one to do all the initial work, I just took the torch 😄

FWIW, I'm planning to use Fibers in the Shepherd, the init used on Guix System. It has to work on GNU/Hurd, which currently lacks epoll but has good'ol poll that libevent must be using there. In short: I'm very interested in seeing this merged. :-)

Hopefully some time before summer (I hope you can wait a bit longer). I've been very busy lately (actually in the middle of job switching right now) but I started working on the autotools changes a couple of months ago even though I couldn't do much.

Bear with me! 😅

[emixa-d]

If we add a fallback for clock_nanosleep, shouldn't it be static (maybe not possible with the FFI) or at least namespaces (e.g., _fibers_clock_nanosleep or such)? Otherwise, there's a potential risk with interfering with other libraries' or the program's replacement of clock_nanosleep.

Edit: alternatively, IIUC, there are a number of flags you could set when loading a library or maybe when compiling to keep the symbol both visible and that-library-only (not visible to other libraries or the program unless specifically requested from that library).

[emixa-d]

I don't think unconditionally adding a replacement for clock_nanosleep is good -- what if Darwin improves later and adds a clock_nanosleep? I think some AC_CHECK_FUN would be appropriate.

[emixa-d]

Looking at my local Guile REPL, we already have 'getaffinity' / 'setaffinity'.

[aconchillo]

Looking at my local Guile REPL, we already have 'getaffinity' / 'setaffinity'.

Not on macOS REPL.

[emixa-d]

On epoll platforms, the epoll library is not loadeded when cross-compiling

(eval-when (eval load compile)
  ;; When cross-compiling, the cross-compiled 'epoll.so' cannot be loaded by
  ;; the 'guild compile' process; skip it.
  (unless (getenv "FIBERS_CROSS_COMPILING")
    (dynamic-call "init_fibers_epoll"
                  (dynamic-link (extension-library "fibers-epoll")))))

However, AFAICT, the libevent and darwin code doesn't have anything equivalent, is this somehow unnecessary when cross-compiling to non-Linux?

[aconchillo]

@emixa-d hi! you made some comments to a particular commit. it might be too much to ask, but would you mind adding the comments in this PR? otherwise it's very hard to keep track of those comments. thank you!

[aconchillo]

@emixa-d hi! you made some comments to a particular commit. it might be too much to ask, but would you mind adding the comments in this PR? otherwise it's very hard to keep track of those comments. thank you!

also, just realized your comment are for a very old commit and some of them don't apply anymore (e.g. posix-clock ones).

[emixa-d]

I put the comments on the PR this time, and added a few more.

[aconchillo]

@Habush @wingo @civodul an initial pass of this PR is done. we now have all unit tests passing with epoll and libevent on Linux and libevent on macOS. There are a couple of tests I had to disable on macOS because of the lack of SOCK_NONBLOCK (which btw, we should probably support?).

Also, the macOS implementation is considerably slower (because the nanosleep clock is not implemented properly yet).

I still have to go through @emixa-d's comments, but right now you can do:

./configure

which will use epoll on Linux, and:

./configure --with-libevent=yes

which will enable the libevent bakend both on Linux or macOS.

[aconchillo]

On epoll platforms, the epoll library is not loadeded when cross-compiling

(eval-when (eval load compile)
  ;; When cross-compiling, the cross-compiled 'epoll.so' cannot be loaded by
  ;; the 'guild compile' process; skip it.
  (unless (getenv "FIBERS_CROSS_COMPILING")
    (dynamic-call "init_fibers_epoll"
                  (dynamic-link (extension-library "fibers-epoll")))))

However, AFAICT, the libevent and darwin code doesn't have anything equivalent, is this somehow unnecessary when cross-compiling to non-Linux?

I assume we would need to do the same. Otherwise the library will be loaded and will of course fail, I assume that was the intention here.

[aconchillo]

Also, can this ignoring cause some events to be missed?

I believe that only one callback will be triggered for a registered event. So, in theory, if we add 3 events and all of them are active we will get 3 callbacks only.

However, one problem would be adding more FDs to monitor. In that case, we would have more events than what we previously anticipated. So, I think the fix would be to increase the events vector when we add new events. Since libevent doesn't have the notion of maxevents like epoll_wait has.

[aconchillo]

If we add a fallback for clock_nanosleep, shouldn't it be static (maybe not possible with the FFI) or at least namespaces (e.g., _fibers_clock_nanosleep or such)? Otherwise, there's a potential risk with interfering with other libraries' or the program's replacement of clock_nanosleep.

Edit: alternatively, IIUC, there are a number of flags you could set when loading a library or maybe when compiling to keep the symbol both visible and that-library-only (not visible to other libraries or the program unless specifically requested from that library).

I will go the _fibers_clock_nanosleep route.

[aconchillo]

I don't think unconditionally adding a replacement for clock_nanosleep is good -- what if Darwin improves later and adds a clock_nanosleep? I think some AC_CHECK_FUN would be appropriate.

I think we can do this later after this PR.

[aconchillo]

On epoll platforms, the epoll library is not loadeded when cross-compiling

(eval-when (eval load compile)
  ;; When cross-compiling, the cross-compiled 'epoll.so' cannot be loaded by
  ;; the 'guild compile' process; skip it.
  (unless (getenv "FIBERS_CROSS_COMPILING")
    (dynamic-call "init_fibers_epoll"
                  (dynamic-link (extension-library "fibers-epoll")))))

However, AFAICT, the libevent and darwin code doesn't have anything equivalent, is this somehow unnecessary when cross-compiling to non-Linux?

I assume we would need to do the same. Otherwise the library will be loaded and will of course fail, I assume that was the intention here.

This is now done.

[aconchillo]

If we add a fallback for clock_nanosleep, shouldn't it be static (maybe not possible with the FFI) or at least namespaces (e.g., _fibers_clock_nanosleep or such)? Otherwise, there's a potential risk with interfering with other libraries' or the program's replacement of clock_nanosleep. Edit: alternatively, IIUC, there are a number of flags you could set when loading a library or maybe when compiling to keep the symbol both visible and that-library-only (not visible to other libraries or the program unless specifically requested from that library).

I will go the _fibers_clock_nanosleep route.

This is now done. Also, I took the clock_nanosleep implementation from glibc instead of just calling nanosleep.

[aconchillo]

    printf("Max events fired. Ignoring\n");

I don't think that applications expect fibers to write to stdout. (current-error-port) would probably be fine though.

Done.

[aconchillo]

@wingo @civodul @emixa-d @Habush Not planning to make any more changes, so this should be ready for review. Let me know what you think.

Thank you again @Habush for all the initial work!

[gdt]

I tried to build (NetBSD 9) and got a lot of errors from autogen.sh, including

autoreconf: running: aclocal --force -I m4
autoreconf: running: /usr/pkg/bin/autoconf --force
configure:13201: error: possibly undefined macro: AC_LIB_LINKFLAGS_FROM_LIBS
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
autoreconf: error: /usr/pkg/bin/autoconf failed with exit status: 1

but I think this happens on master too. I do have guile3 installed, and, while not listed as dependencies in README, I have autoconf, automake, libtool (and generally those are fine with most things). I can't find any definition of that macro, and am guessing there is a dependency I don't have. Feel free to tell me to file a new issue, guile-user, etc.

[emixa-d]

I tried to build (NetBSD 9) and got a lot of errors from autogen.sh, including

autoreconf: running: aclocal --force -I m4
autoreconf: running: /usr/pkg/bin/autoconf --force
configure:13201: error: possibly undefined macro: AC_LIB_LINKFLAGS_FROM_LIBS
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
autoreconf: error: /usr/pkg/bin/autoconf failed with exit status: 1

but I think this happens on master too. I do have guile3 installed, and, while not listed as dependencies in README, I have autoconf, automake, libtool (and generally those are fine with most things). I can't find any definition of that macro, and am guessing there is a dependency I don't have. Feel free to tell me to file a new issue, guile-user, etc.

Going by https://www.gnu.org/software/gnulib/manual/html_node/Searching-for-Libraries.html , it is part of gnulib. As such, I propose to bundle the relevant m4 locally, or preferably document the dependency and package gnulib for the relevant distributions.

[gdt]

Thanks. There is no gnulib in pkgsrc, it seems -- which strikes me as odd. I should fix that anyway.

See #67 #68.

[aconchillo]

Thanks. There is no gnulib in pkgsrc, it seems -- which strikes me as odd. I should fix that anyway.

See #67 #68.

As mentioned in https://github.com/wingo/fibers/issues/68#issuecomment-1324403214 this also comes with gettext. But yes, we should probably remove this dependency and add the corresponding file in the repo.

[aconchillo]

I opted for including the necessary m4 files. For review #70.

[aconchillo]

Thanks. There is no gnulib in pkgsrc, it seems -- which strikes me as odd. I should fix that anyway.

See #67 #68.

I just removed gnulib/gettext dependency and rebased this branch on top of master. Let me know if this worked. Thanks!

[gdt]

I checked out the #70 branch, and was able to run autogen without errors. configure mostly worked and didn't find guile, but that's because we namespace it into /usr/pkg/guile/3.0 and /usr/pkg/guile/2.2 and surely I can fix it, but just wanted to say that thye new macro includes appear totally ok for me. Thank you very much for adjusting!

[aconchillo]

I checked out the #70 branch, and was able to run autogen without errors. configure mostly worked and didn't find guile, but that's because we namespace it into /usr/pkg/guile/3.0 and /usr/pkg/guile/2.2 and surely I can fix it, but just wanted to say that thye new macro includes appear totally ok for me. Thank you very much for adjusting!

Fantastic. Please your next findings, thanks!

[gdt]

So I will need to add a posix-clocks file, and adjust configure.ac. That looks not really that hard, at least until the next thing I don't understand :-) But, I would say that if you have this branch tested on a new platform (that doesn't have epoll and uses poll), then it may be best to merge it, if you are sure it does not cause trouble on previously-working platforms. Then people can base PRs off master and things can be a bit simpler.