[fbzmq] Add support for OS X/BSD systems #19

cabelitos · 2021-05-01T20:46:32Z

Description:
This PR adds the support for using fbzmq on OS X systems which were not supported before.

I made these changes, since I was studying the openr library and I wanted to test it on my machine.

Note: This was tested on a macbook running on Apple Silicon (Apple M1)

Test Plan:

All tests cases successfully passed.
All samples are working

I uploaded a gif to imgur which shows the project compiling and all tests running, here's the link: https://imgur.com/gallery/YWoI6E3
(The gif is quite big, 5mb)

cabelitos · 2021-05-01T21:00:41Z

fbzmq/async/ZmqEventLoop.cpp

+  int signalFd = signalFd_;
+#else
+  createPipe(signalFds_);
+  createPipe(callbackFds_);


we can use kqueue in here if you guys prefer, but I think it's a overkill.

TBH don't know about kqueue. Glad to learn with this change, it seems superior than epoll. Lol, side effect of being confined to linux.

This may be helpful if one wants to disable some warning that was previously added by the project itself which may cause problem on different envs. For example: Clang might warn about certain code constructions that g++ will now, this causing the code to not be compiled on OS X. In order to avoid such issues allow the caller to supress such warnings.

cabelitos · 2021-05-05T15:41:43Z

any news on this one?

cooperlees · 2021-05-06T14:33:51Z

Going to pull in and see how it does with our internal CI. Will report back and hopefully someone will review soon.

facebook-github-bot · 2021-05-06T14:34:07Z

@cooperlees has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cabelitos · 2021-05-06T15:12:22Z

@cooperlees which kind of linter do you guys for C/C++ code?
thank you for the help

cooperlees · 2021-05-06T15:35:48Z

clang formatter with some customizations, so I plan to format this after we accept it. So don't worry there.

saifhhasan

Thanks @cabelitos for a pull request to support fbzmq on MacOS & BSD. One major note about data-structure and few minor nits inline.

If you only intend to use Open/R, then you might want to reconsider the efforts of supporting fbzmq. Open/R is moving away from fbzmq and it has minimal usage.

Only KvStore module uses it for the BACKWARD compatibility reasons to old version of Open/R. KvStore has newer interface for inter-node communication over thrift.
OpenrEventLoop has support for fbzmq only for KvStore
So given this information an Open/R fork to drop fbzmq support for your specific use-case would suffice as well. In near future we're on verge of removing fbzmq traces from Open/R completely so you can converge to master in 1-2 months timeframe.

saifhhasan · 2021-05-06T17:45:01Z

fbzmq/async/AsyncSignalHandler.h

@@ -92,6 +98,10 @@ class AsyncSignalHandler {

  // registered signals
  sigset_t registeredSignals_;
+
+#ifdef IS_BSD
+  std::unordered_map<int, sig_t> signalHandlers_;


Use an array instead of map. Index of array would indicate signal-type and value would indicate signal-value. Map could get very expensive (i.e. one lookup for every message received).

signal.h has macros for MAX signal that you can leverage & initialize to 0.

I had to tweak a little bit, since SIGRTMAX is linux specific. But BSD has the same concept.

saifhhasan · 2021-05-06T17:47:41Z

fbzmq/async/ZmqEventLoop.cpp

+  int signalFd = signalFd_;
+#else
+  createPipe(signalFds_);
+  createPipe(callbackFds_);


TBH don't know about kqueue. Glad to learn with this change, it seems superior than epoll. Lol, side effect of being confined to linux.

saifhhasan · 2021-05-06T17:49:13Z

fbzmq/async/AsyncSignalHandler.h

@@ -92,6 +98,10 @@ class AsyncSignalHandler {

  // registered signals
  sigset_t registeredSignals_;
+
+#ifdef IS_BSD
+  std::unordered_map<int, sig_t> signalHandlers_;


Use array instead of map. There are limited number of signals & signal.h should have MAX signal value as well for bounded array. Most entries would go unused but performance benefit would be huge.

Map lookup on every message would cost a heavy penalty.

saifhhasan · 2021-05-06T17:49:53Z

fbzmq/async/ZmqEventLoop.cpp

@@ -37,27 +42,35 @@ ZmqEventLoop::ZmqEventLoop(
  if ((callbackFd_ = eventfd(0 /* init-value */, EFD_NONBLOCK)) < 0) {
    LOG(FATAL) << "ZmqEventLoop: Failed to create an eventfd.";
  }
-
+  int callbackFD = callbackFd_;


nit: callbackFd for consistent naming

saifhhasan · 2021-05-06T17:51:22Z

fbzmq/async/ZmqEventLoop.cpp

+
+#ifdef IS_BSD
+void
+ZmqEventLoop::createPipe(int fds[]) {


Rename to createPipeBsd to be more specific that it is for Bsd only

saifhhasan · 2021-05-06T17:51:46Z

fbzmq/async/ZmqEventLoop.cpp

+}
+
+void
+ZmqEventLoop::setNonBlockingFileDescriptor(int fd) {


Use short name Fd

facebook-github-bot · 2021-05-07T00:55:19Z

@cabelitos has updated the pull request. You must reimport the pull request before landing.

cabelitos · 2021-05-07T00:56:20Z

@saifhhasan ty very much for the detailed review and the warning about fbzmq being deprecated on open/r.

do you think that I should close this PR? anyways I sent a new version which contains all fixes that you request.

Btw, yeah kqueue is great. It's our epoll on BSD world and I think that the kqueue API is easier to interact with. :]

Ty very much for the awesome support guys.

facebook-github-bot · 2021-05-07T13:03:18Z

@saifhhasan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

saifhhasan · 2021-05-07T13:13:44Z

@cabelitos I believe you can proceed with this PR as the work has already been done and there are no blockers. The main concern is that in future if someone from within FB changes fbzmq, it may break BSD as we don't have CI for BSD. I guess you might be addressing that on your end.

I've pulled your latest pull request on our internal CI. And once it is green, I will trigger merge with master.

BTW happy that we can help with your use-case. I discussed with a core Open/R Developer, @xiangxu1121, and he expects to have fbzmq removed from Open/R in few months timeframe (if we don't hit any roadblocks). So you can drop fbzmq dependency on your end when the changes on Open/R side lands.

This commit adds the support for running fbzmq on OSX/bsd envs

facebook-github-bot · 2021-05-07T15:03:32Z

@cabelitos has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-05-07T15:05:13Z

@cooperlees has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cabelitos · 2021-05-07T15:06:02Z

@saifhhasan great, I'll keep an eye on the repo in order to support it. Btw, I updated the cmake files since I got trolled by cmake with the LINUX constant not being available. I tested the new version on a linux machine and it was able to compile. (sorry for that)

Thank you very much for your support and all the open/r team for the support. You guys are great!

facebook-github-bot · 2021-05-20T20:27:14Z

@cooperlees merged this pull request in f81ac82.

facebook-github-bot added the CLA Signed label May 1, 2021

cabelitos mentioned this pull request May 1, 2021

Compiling for aarch64 architecture facebook/openr#89

Closed

cabelitos commented May 1, 2021

View reviewed changes

cabelitos force-pushed the osx branch 2 times, most recently from 303f721 to 4fd7e20 Compare May 2, 2021 17:22

cabelitos changed the title ~~[fbzmq] Add support for OS X systems~~ [fbzmq] Add support for OS X/BSD systems May 2, 2021

cooperlees requested review from yi-xian, saifhhasan and xiangxu1121 May 5, 2021 15:56

cooperlees added the enhancement label May 5, 2021

saifhhasan suggested changes May 6, 2021

View reviewed changes

cabelitos force-pushed the osx branch from 4fd7e20 to b560bec Compare May 7, 2021 00:55

feat: add osx/bsd support port

bcc759e

This commit adds the support for running fbzmq on OSX/bsd envs

cabelitos force-pushed the osx branch from b560bec to bcc759e Compare May 7, 2021 15:03

facebook-github-bot closed this in f81ac82 May 20, 2021

facebook-github-bot added the Merged label May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fbzmq] Add support for OS X/BSD systems #19

[fbzmq] Add support for OS X/BSD systems #19

cabelitos commented May 1, 2021 •

edited

cabelitos May 1, 2021

saifhhasan May 6, 2021

cabelitos commented May 5, 2021

cooperlees commented May 6, 2021

facebook-github-bot commented May 6, 2021

cabelitos commented May 6, 2021 •

edited

cooperlees commented May 6, 2021

saifhhasan left a comment

saifhhasan May 6, 2021

cabelitos May 7, 2021

saifhhasan May 6, 2021

saifhhasan May 6, 2021

saifhhasan May 6, 2021

saifhhasan May 6, 2021

saifhhasan May 6, 2021

facebook-github-bot commented May 7, 2021

cabelitos commented May 7, 2021 •

edited

facebook-github-bot commented May 7, 2021

saifhhasan commented May 7, 2021

facebook-github-bot commented May 7, 2021

facebook-github-bot commented May 7, 2021

cabelitos commented May 7, 2021 •

edited

facebook-github-bot commented May 20, 2021

[fbzmq] Add support for OS X/BSD systems #19

[fbzmq] Add support for OS X/BSD systems #19

Conversation

cabelitos commented May 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cabelitos commented May 5, 2021

cooperlees commented May 6, 2021

facebook-github-bot commented May 6, 2021

cabelitos commented May 6, 2021 • edited

cooperlees commented May 6, 2021

saifhhasan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented May 7, 2021

cabelitos commented May 7, 2021 • edited

facebook-github-bot commented May 7, 2021

saifhhasan commented May 7, 2021

facebook-github-bot commented May 7, 2021

facebook-github-bot commented May 7, 2021

cabelitos commented May 7, 2021 • edited

facebook-github-bot commented May 20, 2021

cabelitos commented May 1, 2021 •

edited

cabelitos commented May 6, 2021 •

edited

cabelitos commented May 7, 2021 •

edited

cabelitos commented May 7, 2021 •

edited