-
Notifications
You must be signed in to change notification settings - Fork 32
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
add support for multiple backend (epoll, libevent...) #53
Conversation
Bon dia @aconchillo, awesome work! 👍 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 |
I don't think that applications expect fibers to write to stdout. |
Also, can this ignoring cause some events to be missed? |
Hola! Thanks! @Habush was the one to do all the initial work, I just took the torch 😄
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! 😅 |
If we add a fallback for 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 don't think unconditionally adding a replacement for |
Looking at my local Guile REPL, we already have 'getaffinity' / 'setaffinity'. |
85e8cb8
to
432719b
Compare
Not on macOS REPL. |
10eccf6
to
ec18802
Compare
On epoll platforms, the epoll library is not loadeded when cross-compiling
However, AFAICT, the libevent and darwin code doesn't have anything equivalent, is this somehow unnecessary when cross-compiling to non-Linux? |
@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). |
4df052c
to
f66c446
Compare
I put the comments on the PR this time, and added a few more. |
e322929
to
57db967
Compare
@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:
which will use
which will enable the |
e084c04
to
30057c7
Compare
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. |
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 |
@emixa-d All comments have been addressed, I just added a question for this one: #53 (comment) |
@emixa-d I'm waiting for the author of that article to mention something about th elicense or give us permission to add a copyright line with his name on it and with LGPL3+. If I don't hear back, what would be the options? Looking at that code I don't see many other ways to implement that. Aside from that, is there anything else missing that would be a blocker? (Other improvements can be done in future PRs). Not sure if you had time to review again. No rush at all! Thank you! 🙌 |
On options: there exist a bunch of exceptions in copyright. For example, https://fsfe.org/freesoftware/legal/faq.en.html mentions
That's a bit of an extreme case, but you could look into whether this 'threshold of originality' or some other limitation applies in our case. If all fails, we could also ignore the |
One thing I forgot: you are making a pipe, but aren't setting O_CLOEXEC, which makes spawning new processes in a clean environment more complicated. Preferably O_CLOEXEC would be set atomically (instead of with fcntl), using the optional argument of 'pipe': From the Guile NEWS (changes in 3.0.9)
Don't know if Darwin has that, though. However, according to the documentation, 'pipe' will throw a 'system-error' with ENOSYS when the second argument is unavailable so you could implement a fcntl fallback with that. |
if setaffinity is an optimization, then building on a platform without support should issue a configure or build time warning and/or a run-time notice, and just use a null implementation that does nothing. It's far better to function slightly slower than to not function. I previously had the impression from how this was handled, despite what I would guess it does, that it was somehow mandatory. |
FWIW:
|
Ah, good catch! Unfortunately, macOS doesn't have |
I decided to get rid of macOS https://developer.apple.com/forums/thread/44002 So, with this we also solve the copyright issues. Please, take a final look and let me know how everything looks. Once you guys give the thumbs up I will merge this PR and cut a 1.2.0 release soon. Again, thank you so much for all the feedback! This was started 1 and a half years ago by @Habush and it's taken a while, mainly due to lack of time, but we almost there! |
1579505
to
63c1388
Compare
It seems it is not possible to actually set affinity on macOS so we make it a no-op and make all CPU cores available https://developer.apple.com/forums/thread/44002
63c1388
to
f18fd72
Compare
ffa19ed
to
c744201
Compare
Unless there are more comments, I'm planning to merge this PR next Thursday Dec 22 and cut Fibers 1.2.0 before Dec 25 as a holidays gift. |
0363d4d
to
14f5dcf
Compare
14f5dcf
to
0be1359
Compare
Hi @aconchillo. I'm glad to see this work develop into this and be finally merged. Thank you for all your work. Happy Holidays! |
Just realized I said I would release Dec 25... oh well 😅 |
No description provided.