Multithreading via C++ thread pool of clients

[kentslaney]

I've implemented the C++ thread workers to be FILO under the assumption that reusing a single client is preferable when possible. The current implementation needs C++17 or above to parallelize some of the client setup (std::execution). Building still fails with the required version, but I figured I'd open a draft PR anyway in case there's feedback. I'd especially appreciate advice about a good testing setup for the concurrent code along with any opinions about language bindings.

[lexdene]

World you please show a benchmark about the performance improvement your modifications have? If there is no obvious improvement, I'd like to do the multithreading stuffs in Python code rather than inside the code of libmc.

[kentslaney]

The benchmark results can be found here. The naive implementation of a python thread pool comes with the problem of an arbitrarily bad worst case since the unlock order is undefined, which you can see in the standard deviation of py thread pool. A thread mapped pool handles large memory request spikes poorly, space which can be used for a local cache. It can also put the caching server under high load since the number of memcached client connections is tied to the number of HTTP response handlers. The python equivalent of the C++ implementation, py FIFO thread pool should solve both problems. Compared to C++, the python version has a higher average response time (~1.5x) and standard deviation (~2x), but shouldn't suffer from the same scaling problems as py thread pool.

The same C++ implementation can be applied to Go as well, but from a maintenance standpoint, C++ is 250+ lines compared to python's 50. Given Go doesn't have a GIL, the response time is probably going to be significantly better than 1.5x C++. Results including gevent are still pending bug fixes on an update to greenify.

@lexdene I'm not sure what portion of server time is spent on cache I/O or if a 30% improvement is worth the maintenance. I'll probably finish up the greenify thing regardless so there might be more accurate benchmarks soon.

[lexdene]

I think any improvement is welcome unless it brings breaking changes. After looking over your code, I see that a new class with thread is introduced and the original class remains the same as before, which lgtm.

[kentslaney]

Alright, I think this is ready to be reviewed/merged. I also rewrote some parts of the README unrelated to this PR for the sake of clarity.

I'll start tracking gevent support for threaded clients in a greenify PR. Ideally that shouldn't affect the implementation here.

Edit: actually, one more thing; I meant to look into golang bindings

[kentslaney]

It looks like gomemcache is already thread safe, so I'm back to thinking this PR is done.

[kentslaney]

I've closed the greenify PR for being unsolvable. Unless you have feedback, this pull request is done. Please squash & merge.

Thanks for your time, I know this is a lot of code to add at one time. The reason that I made the fork is already satisfied by having it under my profile, so the pull request really is up to you.

[lexdene]

I think the changes under .github/workflows directory are not related. Would you please revert all changes under this directory?

I do not agree with some changes of readme. Additionally, I do not want to discuss much about readme here which I think is not related. Would you please revert all changes in readme and submit another pull request which only contains changes of it so that we can discuss specifically and pointedly.

What is misc/aliases used for? I have not found any code referencing this file. If it is possible to delete misc/aliases, misc/git/debug.patch and misc/git/pre-commit which are only referenced by misc/aliases can also be deleted.

Changes about greenify are also not related I think.

Sorry for replying so slowly, but it is really not easy to look over such a big pull request. I will be happier if it is splitted into several small ones.

[kentslaney]

I think I've now removed all the parts of the pull request that aren't necessary for the ThreadedClient interface in Python.

Quick summary of the parts removed:

• .github/workflows/golang.yml and .github/workflows/python.yml: changes for nektos/act
• .github/workflows/manual.yml: shorter, manual tests for faster iteration
• .gitignore: cygdb (cython debugger) uses /cython_debug
• Makefile: make clean
• README.rst: discussed above
• include/Common.h, src/Common.cpp, and src/Connection.cpp: better naming for some of the things in #120
• misc/aliases: executable with shortcuts for commands I regularly used during development
• misc/git/*: created git hooks that automatically un-staged the setup.py changes I needed for debugging Cython
• misc/memcached_server: logs all port traffic for debugging reasons
• misc/runbench.py: raises an exception for tests with incorrect output instead of potentially clogging stdout
• setup.py: actually I don't remember why this one was there, but it removes some process cleanup steps apparently

I'm currently expecting to make separate PRs to add back the changes in:

• README.rst
• include/Common.h, src/Common.cpp, and src/Connection.cpp

Here's some ideas on parts that can further split up the changes, along with what I see as the benefits and reasons not to.

• benchmarking changes needed for threading
  • pros
    • allows the functional parts to be merged with less code to review
  • cons
    • mostly just moves the changes in runbench.py to a different PR
    • removes an integration test for the threaded code
• separate the C threading interface from the python one
  • pros
    • fairly even split, significant reduction in largest PR size
  • cons
    • they're already mostly organized by file type
    • wouldn't clarify the changes in either
    • the python interface, most likely to be user facing, has the same critical path

Please let me know if there's any changes that would make reviewing easier.

[kentslaney]

:tada: :tada: :tada: