Some issues concerning the network module

[Cylix]

Some issues are reported concerning the network part. (#19 #21 #24 #25) One is concerning the unix version, the other are mostly concerning the windows port.

I am currently working on an external library providing networking TCP features in a much-enhanced design.

This external library is intended to replace the networking module of cpp_redis and this should fix these issues.

[MikesAracade]

Please no external dependencies. Keep it simple.

On Nov 23, 2016 2:45 PM, "Simon Ninon" notifications@github.com wrote:

Some issues are reported concerning the network part. (#19 https://github.com/Cylix/cpp_redis/issues/19 #21 https://github.com/Cylix/cpp_redis/issues/21 #24 https://github.com/Cylix/cpp_redis/issues/24 #25 https://github.com/Cylix/cpp_redis/issues/25) One is concerning the unix version, the other are mostly concerning the windows port.

I am currently working on an external library providing networking TCP features in a much-enhanced design.

This external library is intended to replace the networking module of cpp_redis and this should fix these issues.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Cylix/cpp_redis/issues/29, or mute the thread https://github.com/notifications/unsubscribe-auth/ARc77BbmyfWN6GCdvFruG0Fdt9cgaSnrks5rBKXOgaJpZM4K7B_Y .

[Cylix]

Yes, I don't specifically like the idea of dependency. It is just that I am currently working on a C++ HTTP Server and so the TCP networking part should be the same as the one that will be used on C++ redis.

So, I'm going to see how I will integrate that, having in mind your request (with which I totally agree).

[rpopescu]

Does this work together with Boost ASIO's io_service? The client side API design is good, but the ability to integrate this library with a codebase that uses io_service is essential. Also, the IO implementation seems very heavy - there are mutexes locked on every read and write. That's a big price to pay for a single threaded client doing asynchronous IO. Have you considered ditching the entire hand-rolled implementation for network IO and replacing it with something based on Boost ASIO?

[MikesAracade]

This may be true for your needs. Most people DO NOT want more dependencies like Boost. Being small and simple to build it what makes it great.

I thought the Boost dependency was removed some time ago.

What price do you think you are paying? The IO Service is pretty light weight if you look at it. There isn’t much code at all.

From: rpopescu [mailto:notifications@github.com] Sent: Tuesday, November 29, 2016 3:53 AM To: Cylix/cpp_redis Cc: MikesAracade; Comment Subject: Re: [Cylix/cpp_redis] Some issues concerning the network module (#29)

Does this work together with Boost ASIO's io_service? The client side API design is good, but the ability to integrate this library with a codebase that uses io_service is essential. Also, the IO implementation seems very heavy - there are mutexes locked on every read and write. That's a big price to pay for a single threaded client doing asynchronous IO. Have you considered ditching the entire hand-rolled implementation for network IO and replacing it with something based on Boost ASIO?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Cylix/cpp_redis/issues/29#issuecomment-263525547, or mute the thread https://github.com/notifications/unsubscribe-auth/ARc77EqPG8BANRE5nft4gjRcUrNcLpumks5rC_YAgaJpZM4K7B_Y .[image: Image removed by sender.]

[rpopescu]

Mike, first of all, I only wanted to be able to integrate it with something I use, not force this down to everyone else. There's a couple of ways this integration can be done, and one of them is exposing a poll_one() function of your own reactor, which will poll the managed FDs and will run at most one handler for the ready events, and which can be called repeatedly to process everything without actually owning the main run() loop. If this is easily achieved then codebases using other main loops - e.g. from Boost ASIO or libevent - can easily integrate cpp_redis.

My second point stemmed from the fact that the network implementation has seemingly quite a few bugs and is also slow, using locking even when the usage pattern doesn't require it. An implementation that doesn't reinvent the wheel and makes use of an established IO library doesn't sound like a serious price to pay, compared to crashes and poor performance, but maybe "most people" disagree?

[MikesAracade]

Your code suggestions are only for a single platform.

One of the things that makes cpp_redis great is the lack of dependancies.

How is it slow? What poor performance? Back those statement up with facts, not opinions.

The bugs that exist are small and will be fixed.

From: rpopescu [mailto:notifications@github.com] Sent: Tuesday, November 29, 2016 9:03 AM To: Cylix/cpp_redis Cc: MikesAracade; Comment Subject: Re: [Cylix/cpp_redis] Some issues concerning the network module (#29)

Mike, first of all, I only wanted to be able to integrate it with something I use, not force this down to everyone else. There's a couple of ways this integration can be done, and one of them is exposing a poll_one() function of your own reactor, which will poll the managed FDs and will run at most one handler for the ready events, and which can be called repeatedly to process everything without actually owning the main run() loop. If this is easily achieved then codebases using other main loops - e.g. from Boost ASIO or libevent - can easily integrate cpp_redis.

My second point stemmed from the fact that the network implementation has seemingly quite a few bugs and is also slow, using locking even when the usage pattern doesn't require it. An implementation that doesn't reinvent the wheel and makes use of an established IO library doesn't sound like a serious price to pay, compared to crashes and poor performance, but maybe "most people" disagree?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Cylix/cpp_redis/issues/29#issuecomment-263593840, or mute the thread https://github.com/notifications/unsubscribe-auth/ARc77E_eRlWwHSubw0sDH65mcmkHCSIUks5rDD6qgaJpZM4K7B_Y .[image: Image removed by sender.]

[Cylix]

Hi rpopescu,

Thanks for your interest in cpp_redis!

I'm not a big fan of Boost.Asio and especially their io_service. cpp_redis once used it and I personally decided to remove the dependency for something as lightweight as possible.

I'm currently not open to add dependencies to the project.

However, I guess it should be possible to make a workaround which can be optionally compiled and can provide compatibility of the library with Boost.Asio io_service. That way, the library can stay lightweight (no deps) but people willing to use Boost.Asio will be able to do it. I'll have to think about how to it in a convenient way.

Concerning locks, the new network service I'm working on is designed in a totally different way that should reduce the number of locks drastically, thus improving performance. I might also give a look to locks located outside the network part of the library.

Thanks again for your comment, it is really cool to have some feedbacks and real-world needs that can help to improve the library!

Best

[rpopescu]

No, they are not for a single platform. They would work for all platforms, as does any decent IO library. Looking through the code, process_io() runs an IO processing loop while not m_should_stop; my suggestion makes the body of that loop available as a run-once function, that can be integrated with a different IO event loop.

Regarding performance, the recursive mutexes used in read and write operations are an unnecessary penalty. The fact that the library uses at least one separate thread for IO, the scheduling of which is out of control of the user, is again a performance concern. Applications that require optimal use of CPU time must bind their threads to specific cores (setting the CPU affinity mask) in order to ensure their performance is maximised by not having their cache trashed by other threads, and by preventing very expensive and unpredictable context switches caused by CPU migrations. These are design concerns that are well known when implementing high performance IO bound applications.

[rpopescu]

Hi Simon,

Thanks for your welcoming message! I think our messages crossed with one another, I only saw yours after I've posted mine in reply to MikesAracade.

I'd be genuinely interested in learning what you disliked about the Boost ASIO io_service! Would you care to share your concerns?

Much appreciated!

[Cylix]

Hi again rpopescu,

I had the opportunity to work several months on a CCTV C++ Server for a datacenter company. The software was based on cpp-netlib (which is based on boost-asio) and I never had so many issues for so little benefit (I'm still puzzled that cpp-netlib is one of the only available HTTP Server library in C++ in 2016 btw). Getting the same issue on another project also based on cpp-netlib. Even though I guess the issue was mainly coming from cpp-netlib itself, I don't have any proof and thus still had a bad experience.

Also, even though I don't think reinventing the wheel is something useful, making cpp_redis dependent on Boost.Asio would bring a huge dependency that may be completely overkilled for many smaller projects (Boost Asio would just replace our io_service class and a part of the tcp_client class, so only a few part of the cpp_redis code).

In my opinion, a lightweight network module is enough for lots of projects and I don't want to bring unnecessary dependency.

However, I completely do agree that changing the interface of the library to make it possible to use any other network library (the one I am developing, Boost.Asio or any other one) would bring a huge benefit to the library, making it possible to integrate it in any existing project and allowing the user to choose the networking library he wants depending on his needs (and by providing the networking module I'm working on by default).

Moreover, it is perfectly clear that even though lightweight library is good enough for many project, many others need advanced networking features that are not available in a lightweight version. So being flexible on that side is an interesting improvement I haven't thought about before.

Thanks again for your feedback,

Best

[rpopescu]

Simon, that's a superb answer, couldn't really ask for more, thank you kindly.

For what it's worth, I've used Boost ASIO for low latency network IO with no issues. I also really appreciate the way its post() functionality allows it to play nicely with other IO libraries.

If you're interested, I have a couple of suggestions regarding the actual implementation of the improvements discussed.

The polling function that we call from "outside" should take two parameters - the maximum number of events to process in a single run, and the number of nanoseconds for which it should retry to find a ready event if none are available to begin with; in other words, it would be a int poll_one(uint64_t max_wait_ns, unsigned max_events) signature, returning the number of events/callbacks processed. If events are ready to be processes directly, max_wait_ns has no impact; otherwise, the code should query the underlying event readiness mechanism again, until max_wait_ns have elapsed without any events being ready for processing. max_events is obviously the upper limit of callbacks fired / events processed in a single go. These parameters allow sufficient tuning for different latency/throughput scenarios, guiding how much of the top-level run() loop the Redis stuff takes, all other things being equal.

Regarding multi-threading, it is my opinion that at least on systems providing e/poll the library shouldn't need any. However, if you do choose to use threads, I suggest adding a std::function callback to be called once, at startup, from the threads you're spawning internally in the library. This allows the library user to perform some per-thread setup, e.g. set the CPU affinity mask and thus control thread scheduling.

All the best, and looking forward to seeing these changes come to life.

[Cylix]

Hi,

Thanks for your advice, I really do appreciate! I'm going to look into that as soon as I have some time :)

Thanks again!

[MikesAracade]

Now you’ve got me curious…

What is your use case?

Most people don’t set CPU affinity masks on a thread.

Are you trying to run on some IOT device?

I’m not a big fan of premature optimization.

From: rpopescu [mailto:notifications@github.com] Sent: Tuesday, November 29, 2016 10:01 AM To: Cylix/cpp_redis Cc: MikesAracade; Comment Subject: Re: [Cylix/cpp_redis] Some issues concerning the network module (#29)

No, they are not for a single platform. They would work for all platforms, as does any decent IO library. Looking through the code, process_io() runs an IO processing loop while not m_should_stop; my suggestion makes the body of that loop available as a run-once function, that can be integrated with a different IO event loop.

Regarding performance, the recursive mutexes used in read and write operations are an unnecessary penalty. The fact that the library uses at least one separate thread for IO, the scheduling of which is out of control of the user, is again a performance concern. Applications that require optimal use of CPU time must bind their threads to specific cores (setting the CPU affinity mask) in order to ensure their performance is maximised by not having their cache trashed by other threads, and by preventing very expensive and unpredictable context switches caused by CPU migrations. These are design concerns that are well known when implementing high performance IO bound applications.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Cylix/cpp_redis/issues/29#issuecomment-263611845, or mute the thread https://github.com/notifications/unsubscribe-auth/ARc77NYHwC9Hy0wIb-fxN3ZbJoQ_i3_Uks5rDExFgaJpZM4K7B_Y .[image: Image removed by sender.]

[rpopescu]

Mike, this isn't premature optimisation in the least. It is just solid engineering with respect to:

• integration, i.e. not imposing your own event loop on the client
• resource usage (not using plenty of threads for asynchronous IO because there really is no point on some, if not most OSes), and resource management (optimal use of CPU and network resources). Note that Redis itself is single threaded in the network IO part.

My work happens to be in HFT, which tends to be a pioneering industry in some respects, but the concerns and approaches I've touched upon have longed become rather commonplace in other competitive industry sectors that require high throughput, low latency, or both. For example, virtualisation providers have for a couple of years at least started to use ultra low latency Solarflare network cards, use the OS network stack bypass mechanism these cards provide with special drivers, to tune the kernel, the memory allocators etc. Also, most Linux distros have for years included command line utilities to reschedule processes, change their affinity, prevent process migration and so on, e.g. tuna, taskset etc. All in all, nothing esoteric here.

[MikesAracade]

I see, makes sense with HFT, every microsecond counts.

From: rpopescu [mailto:notifications@github.com] Sent: Wednesday, November 30, 2016 11:12 AM To: Cylix/cpp_redis Cc: MikesAracade; Comment Subject: Re: [Cylix/cpp_redis] Some issues concerning the network module (#29)

Mike, this isn't premature optimisation in the least. It is just solid engineering with respect to:

• integration, i.e. not imposing your own event loop on the client
• resource usage (not using plenty of threads for asynchronous IO because there really is no point on some, if not most OSes), and resource management (optimal use of CPU and network resources). Note that Redis itself is single threaded in the network IO part.

My work happens to be in HFT, which tends to be a pioneering industry in some respects, but the concerns and approaches I've touched upon have longed become rather commonplace in other competitive industry sectors that require high throughput, low latency, or both. For example, virtualisation providers have for a couple of years at least started to use ultra low latency Solarflare network cards, use the OS network stack bypass mechanism these cards provide with special drivers, to tune the kernel, the memory allocators etc. Also, most Linux distros have for years included command line utilities to reschedule processes, change their affinity, prevent process migration and so on, e.g. tuna, taskset etc. All in all, nothing esoteric here.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Cylix/cpp_redis/issues/29#issuecomment-263933454, or mute the thread https://github.com/notifications/unsubscribe-auth/ARc77CUFmf8867l-NpaCryQx_4lgWW5nks5rDa57gaJpZM4K7B_Y .[image: Image removed by sender.]

[Cylix]

Good new, the new network module should be delivered soon!

I worked on a network library, tacopie which provides TCP Client/Server features in a very light way and with a better design than the current network module used in cpp_redis.

In a first time, the library will be mandatory for cpp_redis (meaning that cpp_redis will have to use it) but it won't change anything for the users (even concerning the installation). Basically, the library will be present as a submodule of cpp_redis and is fully integrated to the compilation process (using the same installation steps as before). Under unix/mac, the library is compiled first, then linked to cpp_redis. Under windows, to avoid having two VC++ projects, everything will be compiled as a single library (with the option to compile them separately).

In a second time, later on, the library will be optional: by default, it will be provided, but people willing to use their own network module will be able to set it up (for example, for people already have networking in their app and willing to use the same network interface, like boost asio).

I'm currently working on the integration of the new network module on a dedicated branch.

The changes will be released once:

• tacopie will support windows and additional features that were already supported on cpp_redis such that there is no regression in terms of behavior after the change (this might be achieved soon).
• the link between the two libraries will be completely finished and fully tested (for example, right now, CMake customization variables are not propagated to the tacopie cmakelist).

For now, I have done some tests of cpp_redis+tacopie under unix/mac and it works perfectly fine, so this is on the good way!

Sorry a bit for the delays to provide that but not easy to find the time.

[Cylix]

Closing as it is not relevant anymore after the new release v3.0.0 as explained in #39.