Performance regression in zmq_poll

[WallStProg]

Environment

• libzmq version (commit hash if unreleased): 4.2.4
• OS: CentOS 6.9

[WallStProg]

We recently switched the reference implementation of libzmq that we use from 4.2.3 (latest at the time we started develoment) to 4.3.0, partly to take advantage of some bug fixes that were made in the meantime. (And many thanks to the contributors for those, including @bluca and @bjovke)!

We have a simple "ping-pong" microbench that we use to test for performance regressions, and we found a rather large regression when we switched. (The average latency went from ~ 150us to ~250us).

After some head-scratching and a very helpful conversation with @somdoron we were able to narrow it down to a specific scenario -- viz. when a thread-safe socket is included in the list of sockets being polled.

With that clue I've been able to narrow the regression down to the 4.2.4 release,and also to create a repro, which can be found at https://github.com/WallStProg/zmqtests/tree/master/pingpong.

The results of our tests are as follows:

ping params	4.2.3	4.2.4
(none)	54	55
-poll	58	60
-poll -control	83	186

That lines up almost perfectly with the results from our in-house microbench, which is to say that zmq_poll takes an addl. 100us as of 4.2.4 when a thread-safe socket is included in the socket list. (This on CentOS 6.9, 4 x i5-2400 @ 3.10GHz, 16GB RAM).

Apparently if ZMQ_BUILD_DRAFT_API is defined, the new zmq_poller implementation is used by default, and the "old" zmq_poll implementation is completely unavailable. (Which sort of makes sense, since the old zmq_poll apparently doesn't support thread-safe sockets -- although just because thread-safe sockets are enabled doesn't mean that they'll be used).

What next?

The advice we got from @somdoron is to re-implement our main dispatch thread using zmq_poller, rather than zmq_poll. That makes sense, and since the sockets we poll don't change, should be relatively straight-forward. Presumably there's also a bit of a performance win with zmq_poller as well, but the proof is in the pudding. (We could also replace the CLIENT/SERVER sockets used for IPC signalling, but much prefer sticking with them, as discussed at some length in #2759).

Having said that, there are a few things that I'm curious about:

• Did the 4.2.3 code work properly, even with thread-safe sockets? Or was it just a "ticking time bomb"? If that's the case, it seems to make sense to let people know -- others may be innocently using it in a similar way.
• What specifically is the cause of the regression? Did it fix a bug? (see above)
• Is there any mechanism to detect performance regressions in any of the CI tests?
  • I confess that I'm completely ignorant of the CI infrastructure used for libzmq, so I don't know what it would take to duplicate the CI tests in a different environment.
  • If there's value in the CI tests, it might make sense to "re-factor" them so they can run outside the CI environment. Again, I have no idea whether that's even possible without Travis, etc.

[bluca]

Were you using DRAFT builds beforehand too? If so that might be because of: ab1607f
before that, with draft enabled it directly called the poller implementation, but we got reports that performances were much worse, so I changed it to do so only if there are thread safe sockets being watched, otherwise the old implementation is used.

Now, unless you have a massive amount of sockets, looping to check if there's a thread safe one shouldn't really cause a performance drop - all the data there has to be loaded anyway to be used in the same function, so it's unlikely to thrash the cache or something like that... but I haven't profiled so can't really say for sure

[bluca]

No wait, scratch that - I thought the problem arrived in 4.3.0, but I see now it's 4.2.4 which doesn't have that fix. Try to bisect and see if you can pin point to the right commit.

[somdoron]

Did the 4.2.3 code work properly, even with thread-safe sockets? Or was it just a "ticking time bomb"? If that's the case, it seems to make sense to let people know -- others may be innocently using it in a similar way.

It worked properly, in both versions.

Thread safe sockets require creating an FD to be signaled when they are ready to be polled.
Because zmq_poll is stateless this FD has to be created on every zmq_poll iteration.

In the past, an internal FD was created even if the poll didn't include thread safe socket. The change is to only create an FD if thread safe socket is also being polled.

This change is probably what caused the issue.

This is why zmq_poller can be more efficient because the FD is created once when creating the poller.

[somdoron]

@WallStProg it might be the solution, however, for some, it was actually slower with thread-safe sockets.

[WallStProg]

It worked properly, in both versions.

But in that case, what caused the regression between 4.2.3 and 4.2.4?

[WallStProg]

@bluca wrote:

No wait, scratch that - I thought the problem arrived in 4.3.0, but I see now it's 4.2.4 which doesn't have that fix. Try to bisect and see if you can pin point to the right commit.

The regression exists in 4.3.0 -- we backtracked it to 4.2.4, which is where it first shows up.

[somdoron]

I only suspect this one for the moment:

147fe9e#diff-01df48f9d498dba994077a8675169b88

But it doesn't make a lot of sense. I will be able to run the performance test only tomorrow.

[sigiesec]

@somdoron In 147fe9e you mention, only the code that queries the thread-safe sockopt was extracted into a separate method. I cannot imagine how this could lead to a (perceptible) performance regression, if any.

[somdoron]

@sigiesec I agree, as I said, it doesn't make sense. However, it is the only change in the class between the versions.
Anyway ,we can check it out together on Thursday :)

[sigiesec]

Ah great, you will be at the Hackathon as well :) yes, let's give that a look on Thursday!