Add Python benchmark engine (async valkey-glide, redis-py, valkey-py) - #24
jamesx-improving wants to merge 3 commits into
Conversation
Signed-off-by: James Xin <james.xin@improving.com>
1c02c21 to
b83ef00
Compare
|
Note that an earlier attempt (https://github.com/jduo/resp-bench/tree/python-engine) also added Python sync if we want to see how that was done and consider here. |
Signed-off-by: James Xin <james.xin@improving.com>
Thanks — I went through that branch before opening this PR. Borrowed from it: the Skipped its sync mode deliberately: #11 calls for the async clients, and that path is |
That's fine, just wanted to make sure we referenced the other implementation. We can add sync in the future as needed. |
|
Overall looks good. My main concerns that need to be resolves are that:
|
…arity Signed-off-by: James Xin <james.xin@improving.com>
4096bac
Thanks — this was a genuinely useful review; the measured reproductions made every item easy to confirm. I reproduced all of them locally before changing anything, and all 12 are addressed in On sharing the GLIDE client: I have kept one client per connection for now, because sharing a multiplexing client across workers is what ikolomi#11 explicitly declined — the reasoning there was that one-client-per-transport-connection is the cross-engine comparison baseline, and that transport sharing should be reached via pipelining instead. I do not think I should reverse that inside a language-engine PR. That said your point stands on its own merits: N separate On peer drivers: agreed, and your own findings are the argument for it — the two peers were not configured equivalently. Dropped The four correctness bugs (worker cancellation, loop starvation, warmup, config validation) have tests; the suite is now 59 tests. Two of your findings led somewhere broader than the original report: the starvation hole also existed on the success path (any non-suspending driver let one connection monopolise a duration phase — Given the severity of the starvation and validation issues I would rather you re-reviewed than merged on the earlier approval. |
|
Looks like we are following the conclusion of ikolomi#11 (i.e. client==connection, and use pipelining for shared transport). My only remaining concern I have is that Java/Ruby/C# all support pipelining and can set pipeline_depth>1, so I think we need the same pipeline support in the Python client. |
|
@jamesx-improving +1 on the pipeline_depth support |
|
I suggest to support both sync and async benchmark. |
| if TYPE_CHECKING: # pragma: no cover | ||
| from .benchmark_client import AsyncBenchmarkClient | ||
|
|
||
|
|
||
| def _make_glide() -> "AsyncBenchmarkClient": | ||
| from .impl.glide_client import GlideBenchmarkClient | ||
|
|
||
| return GlideBenchmarkClient() |
There was a problem hiding this comment.
Do we have sync clients? this could be added in a separate PR
There was a problem hiding this comment.
The GLIDE Ruby client is sync and already added so resp-bench. GLIDE Python has sync and async, so we should either add sync support in this PR or create a backlog item to add it later, all depending on how easy it is to add (can also reference the previous branch to add Python to resp-bench here).
Implements the Python engine (#11), at parity with the Java (reference), Ruby, and C# engines.
What
python/(pip install -e .,python -m resp_bench), driving three async drivers, one client per connection on a single asyncio event loop:valkey-glide-python— GLIDE async (import glide)redis-py—redis.asynciovalkey-py—valkey.asyncio(Valkey fork of redis-py)recording— in-memory driver for server-free testsunit:"us", uppercased command keys, HDR compressed base64).Concurrency model
connections = N→ N clients (one per connection — theclient == connectioninvariant) driven by N worker coroutines viaasyncio.gather, each awaiting one command at a time. This is the faithful async analogue of the Java/Ruby "one in-flight request per connection" model, keeping results comparable across engines.pipeline_depth > 1is not yet implemented; a>1request logs a warning and runs at depth 1.Cross-engine parity
JavaRandomLCG port (incl. int32-overflow rejection) — verified byte-identical to Java'sjava.util.Random(seed-0 anchor) and to the Ruby engine (seed 12345).sequential_intuses a per-phase shared counter (matches Java'sforkForThread);uniform_randseeded per workerseed+idx.%0Ndhonoringkey_size_bytes; leaky-bucket rate limiter; HdrHistogram(1, 600_000_000, 3).Harness
python-build/test/run/clean/infotargets.driver_ids inscripts/run_benchmark_matrix.py(DRIVER_ENGINE_MAP) andscripts/generate_graphs.py(DRIVER_LANGUAGE_MAP).configs/drivers/{default,high-throughput}/+example-*-standalone.json.benchmark-pythonjob (matrix over the three drivers) added togenerate-graphsneeds;drivers.jsonpython list.docs/ADDING_LANGUAGE.mdfrom3_600_000_000(1 hour) to600_000_000(600s) — every engine uses 600s.Tests
34 unit + integration tests (
cd python && python -m pytest): parity anchors (JavaRandom, key generator), rate limiter, config loader, HDR encode/decode round-trip, NDJSON schema, and end-to-end runs via the recording driver (no server needed).Notes
valkey-glidedriver_idisvalkey-glide-python(not the barevalkey-glide, which is Java's) — matches thevalkey-glide-ruby/valkey-glide-csharpconvention.