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

Integrate message reading into twisted reactor loop #66

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@daa

daa commented May 19, 2016

During work with txZMQ I met the case when fast incoming messages stream with not so fast message processing functions prevents twisted reactor's loop from resuming because loop reading messages in doRead() never ends - there are always new messages arrived. This pull request transforms message reading and processing loop to cooperative task scheduled by Twisted.

@daa daa changed the title from integrate message reading into twisted reactor loop to Integrate message reading into twisted reactor loop May 19, 2016

@coveralls

This comment has been minimized.

coveralls commented May 19, 2016

Coverage Status

Coverage increased (+0.6%) to 89.456% when pulling 0aa27ad on daa:master into 2843571 on smira:master.

@coveralls

This comment has been minimized.

coveralls commented May 19, 2016

Coverage Status

Coverage increased (+0.3%) to 89.153% when pulling 661f332 on daa:master into 2843571 on smira:master.

@coveralls

This comment has been minimized.

coveralls commented May 19, 2016

Coverage Status

Coverage increased (+0.3%) to 89.226% when pulling e0ecf93 on daa:master into 2843571 on smira:master.

@smira

This comment has been minimized.

Owner

smira commented May 19, 2016

I wonder what performance impact it might have on throughput overall.

Should probably we have message processing functions out of the event loop?

@daa

This comment has been minimized.

daa commented May 20, 2016

I've made some simple throughput measurements sending 1M short 18 byte messages and counting them at the receiver side and got following numbers for receive rate:

  • before patch: ~83000 messages per second
  • after: ~62500 messages per second

For 180K byte messages rates are almost equal ~25K messages per second.

This impact may be reduced by reading several messages at once in read loop limiting by the time spent in cycle before yielding to reactor. However the whole point of this pull request is to make message receiving and processing functions to properly cooperate with other participants in twisted service and to prevent them from consuming all available resources effectively blocking event loop. Also this patch makes possible synchronization of sending and receiving rates by means of hwm, so that sender will not overload receiver.

As for message processing functions out of the event loop I don't quite understand what do you mean unless you speak of offloading them to other threads or processes because everything in Twisted is driven by the event loop.

@smira

This comment has been minimized.

Owner

smira commented May 20, 2016

What I mean that your message processing function should be itself cooperative task, or simply offloaded as reactor.callLater(0, process).

Good point on HWM though. So I'm kind of 50/50 :)

@daa

This comment has been minimized.

daa commented May 23, 2016

I have 2 suggestion on how to reduce performance impact while keeping possibility to cooperate with reactor:

  1. modify this pull request so that loop is not yielded until some time quantum passed allowing to process multiple messages without interruption.
  2. implement pauseProducing() and resumeProducing() methods, remove cooperative message reading task, and put cooperation responsibility on message consumer.

The first is easier for consumers but the second is more explicit and more in line with how Twisted handles similar problems.

@daa daa closed this Oct 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment