Skip to content
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

Improve the throughput of Actors using ZeroMQ and the Request/Reply pattern. #679

Closed

Conversation

alexandreagular
Copy link

Following this discussion https://groups.google.com/forum/#!topic/akka-user/lGBb8GTouV0

I propose a new implementation of the ConcurrentSocketActor in akka-zeromq.

This new version does not rely on polling at all and has faster throughput.

I also made a sample application (akka-sample-zeromq-reqrep) for testing purpose.

Alexandre Agular added 2 commits September 6, 2012 19:16
A first attempt that is not very elegant but significantly improves the throughput of the actor.
@viktorklang
Copy link
Member

Hey! Have all contributors to the code signed the CLA?

typesafe.com/contribute/cla

@alexandreagular
Copy link
Author

I haven't done that yet. I need to check it with my manager, which won't be until monday.

@viktorklang
Copy link
Member

Great, let me know when you've signed or if you cannot sign. Without CLA signature I cannot do anything with this PR but to close it.

Added an sample example for ZeroMQ's publish/subscribe
Merge sample applications into one
@alexandreagular
Copy link
Author

Hi,

Just committed a new version and added some more sample code. the CLA signature should come soon.

@jboner
Copy link
Member

jboner commented Sep 14, 2012

Great. Let us know when you have it signed.

@jacobkolind
Copy link

I am Alexandre's manager (@jacobkolind), and I have just signed the CLA. Are we good to go if he also signs the CLA?

@viktorklang
Copy link
Member

Hi Jacob,

everyone who has contributed to the PR has to sign the CLA. However, we have the possibility of issuing a corporate CLA for the company so that all employees' contributions are covered by that, is that something that would be interesting?

@jacobkolind
Copy link

We discussed this with our CEO, but he was of the opinion that he would rather that we signed the CLA individually. The company has waived all rights to any IP that we contribute to Akka.

@johanlaidlaw
Copy link

I have also signed the CLA so now everyone contributing to this pull request have signed the CLA. Let's hope it can be used :)

@viktorklang
Copy link
Member

Excellent. First of all, could you split it up into logical chunks? (1 PR for the performance fix(es)) and one for the docs and samples?

(msg: PollMsg) ⇒
poller.poll(pollLength)
self ! msg
socket.setReceiveTimeOut(duration.toMillis)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to only work on ZMQ >= 3.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexandreagular
Copy link
Author

Hi,

I haven't checked that, thanks. What is the minimal version of ZeroMQ that we should support ?

I can make a fix for older versions, but it's not going to be as neat. I would rather keep that one for the current and newest version of ZeroMQ and make the fix specifically for oldest versions. Would that be acceptable ?

@viktorklang
Copy link
Member

Latest stable release is 2.2,so that has to be supported.

@alexandreagular
Copy link
Author

So should I leave it as it is ?

@viktorklang
Copy link
Member

Please apply this change to ConcurrentSocketActorSpec so you can run the tests:

def checkZeroMQInstallation =
try {
zmq.version match {
case ZeroMQVersion(x, y, _) if x >= 3 || (x >= 2 && y >= 2) ⇒ Unit
case version ⇒ invalidZeroMQVersion(version)
}
} catch {
case e: LinkageError ⇒ zeroMQNotInstalled
}

@viktorklang
Copy link
Member

@alexandreagular
Copy link
Author

Ok, I am going to check that and do the fix on checkZeroMQInstallation now.

@alexandreagular
Copy link
Author

Hi,

So I have found the problem, it turns out that the setReceiveTimeout method is not working. The reason is that the scala-zeromq binding does not allow this setting with zeromq < 3.0.0.

As this does not match the zeromq 2.2 spec, I was thinking that the proper way to fix it would be to change the scala-zeromq binding, this file in particular: https://github.com/valotrading/zeromq-scala-binding/blob/master/src/main/java/org/zeromq/ZMQ.java
(see the setReceiveTimeout and setSendTimeout methods)

And upgrade the required version for the scala-zeromq binding in akka

Does that sound acceptable to you ?

The other solution we have is a duplicate implementation for zeromq < 3.0.0 and >= 3.0.0 which won't be as neat.

Alexandre

@viktorklang
Copy link
Member

What happens if one uses the java-zmq-binding?

@alexandreagular
Copy link
Author

I checked the implementation of java-zmq-binding, it seems correct, we could use it:
https://github.com/zeromq/jzmq/blob/master/src/org/zeromq/ZMQ.java
line 586 you can see the requirement is 2.2.0

@viktorklang
Copy link
Member

Alright, so I think your proposition is OK: Fix it in the scala-zeromq-binding
When that's done we can then let users either use the scala or the java binding to their liking, they will, however, have to upgrade to atleast 0mq 2.2.

How does that sound?

@alexandreagular
Copy link
Author

Ok that sounds good to me.

I will work on that and come back to you when I am done.

@viktorklang
Copy link
Member

Sounds great, thanks

Also require scala-zeromq 0.0.8-SNAPSHOT (temporary)
@alexandreagular
Copy link
Author

Hi,

So I fixed the problem on the scala binding. I made a pull request for that:
valotrading/zeromq-scala-binding#15

I will probably fix other things I have noticed on it but that is not relevant for this pull request.

In my last commit, I changed the required version for the scala-zeromq binding to 0.0.8-SNAPSHOT, which you will have to download from my branch, compile and publish locally to make it work.

With all that (plus a small fix I had to do) the unit tests pass.

@viktorklang
Copy link
Member

Alright. We won't be able to do anything until the artifact is published with a stable identifier (no SNAPSHOT deps allowed)
Great work improving the binding btw!

import compat.Platform

class RequestActor extends Actor {
println("Connecting...")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ActorLogging

@davojan
Copy link

davojan commented Sep 10, 2013

Hi. This pull-request is closed and I didn't find fixes in master. Does it mean that the problem with request/reply performance is resolved any other way? Can I rely on akka-zeromq and have reasonable performance?

@patriknw
Copy link
Member

I don't know the status, but there is another open pull request by @alexandreagular: #1285

@alexandreagular
Copy link
Author

Yes, I closed this pull request because it was getting very old and I thought it would be simpler to get a fresh master branch and put the changes on them.

There is another one indeed on which some comments have been made that I need to work on. I will try to do that soon in my spare time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants