Multithreaded fan

[JamesOHeaDLS]

Hi @GDYendell

I04 have been using this multithreaded-fan branch for the past week, and it has been running fine, no problems

Hopefully this branch can be merged in to master.

There is a question over where the number of threads is set, and what value it is currently set to?

Thanks,

James

[GDYendell]

Rebased on master.

[ajgdls]

A few magic numbers (8, 100 and 10000) which are used in multiple places so perhaps they should at least be #defined somewhere.

[JamesOHeaDLS]

I've been looking into some of the magic numbers and whether they can be taken from any of the following eigerfan command line options:

  -t [ --threads ] arg (=2)      Set the number of 0MQ threads for the 0MQ context
  -z [ --sockets ] arg (=1)      Set the number of zmq sockets to connect to the Eiger with

The number of sockets option is currently not used in the code

I believe the place where it should be used is when setting up the zmq context as part of EigerFan constructor.

 ctx_(config_.num_zmq_threads)

This context has a second optional argument of number of sockets which is never specified

inline explicit context_t (int io_threads_, int max_sockets_ = ZMQ_MAX_SOCKETS_DFLT)

so must always default to the zmq default of 1023

#define ZMQ_MAX_SOCKETS_DFLT 1023

(https://github.com/zeromq/libzmq/blob/master/include/zmq.h)?

The number of threads option is only used once, in the constructor where config is passed as an argument

EigerFan::EigerFan(EigerFanConfig config_)
: ctx_(config_.num_zmq_threads),
...

I'm wondering whether the number of threads given to the MultiPullBroker (currently hard-coded values) should match the number of zmq threads, or whether it should be treated completely independently (ie given its own argument or #define)?

Finally, there is an additional zmq context created with HandleRxSocket which is spawned by the run function. Should this take the same number of threads as the EigerFan zmq contexts, or should it also be treated independently?

[GDYendell]

I am surprised that the command line sockets not used, but, if it doesn't do anything currently I think we should use it to pass into the broker constructor and leave ZMQ context sockets as the default.

number of threads given to the MultiPullBroker (currently hard-coded values) should match the number of zmq threads

I don't think there is any particular reason that it should be the same, but it is probably more reasonable than hard-coding one while the other is configurable.

[JamesOHeaDLS]

I am surprised that the command line sockets not used, but, if it doesn't do anything currently I think we should use it to pass into the broker constructor and leave ZMQ context sockets as the default.

Do you mean the broker constructor? This only has an argument for number of threads (rather than sockets)?

I don't think there is any particular reason that it should be the same, but it is probably more reasonable than hard-coding one while the other is configurable.

I think that's reasonable if we update the wording to say "the number of threads used for all zmq contexts". Alternatively a new command line option can be provided for this one thing, but it would probably always be the same value anyway

[GDYendell]

This only has an argument for number of threads (rather than sockets)?

Well the two are effectively equivalent, but perhaps it would better to have -t / --threads (passed to MultiPullBroker) and -z / --zmq-context-threads (passed to zmq context).

[MattTaylorDLS]

I'm sure that command line was used at some point! Maybe when I was playing around at the beginning with optimising performance? But can't remember now sorry

[JamesOHeaDLS]

I think the remaining issues are the magic numbers for high water mark and socket linger timeout

I propose #defines for these? Does that sound ok?

[GDYendell]

I think this is good now.

@ndevenish if you have some time to have take a look at this that would be great, but otherwise I think we are just about ready to merge, @JamesOHeaDLS.