Php/resp bench - #23
Open
prateek-kumar-improving wants to merge 9 commits into
Open
prateek-kumar-improving wants to merge 9 commits into
prateek-kumar-improving wants to merge 9 commits into
Conversation
* Fix broken Ruby gem dependency and stale configs, pin build inputs Signed-off-by: James Xin <james.xin@improving.com> * Resolve config paths without rewriting them; finish the gem rename Addresses review findings on the initial commit. Rewriting driver_config/workload_template to absolute paths made matrix expansion depend on where the repo sits on disk: those strings double as identity, since DimensionSpec.matches_driver() fnmatches applies_to globs against the whole string and write_manifest() records it. A checkout under any directory whose name contains "glide" made applies_to ["*glide*"] match every driver, applying GLIDE-only thread env vars to Jedis, Lettuce and Redisson. Validation and the runtime opens now share one resolve_config_path() helper, so they agree by construction while the config strings survive byte-for-byte. The new guard tests chdir off the repo root, without which they passed against the buggy source. Also: reject $binding values for driver_config at parse time, fold a non-string path into the aggregated ValueError, drop a false comment about detect_engine_for_driver() swallowing errors (generate_driver_config() raises outside the guarded region, so a bad path crashes loudly), caveat the requirements.txt pinning claim since transitives float, and finish the gem rename in ruby/README.md and ValkeyGlideClient#driver_version, which still looked up a Bundler spec name that repo never published. Signed-off-by: James Xin <james.xin@improving.com> * address comment: trim over-verbose dependency comments Reviewer noted the AI-authored dep comments were too verbose/narrowly scoped. Trimmed them across the Ruby Gemfile, gemspec, README, the driver_version comment, and scripts/requirements.txt, keeping only the non-obvious bits (the require "valkey" name mismatch and the numpy<2.5 CI constraint). Lockfile follow-up tracked in #20. Comment/doc-only; no logic changes. Signed-off-by: James Xin <james.xin@improving.com> --------- Signed-off-by: James Xin <james.xin@improving.com>
Three failure-visibility problems made unattended sweeps unsafe: an unrelated precondition aborted the whole run, separate runs silently merged into the same files, and partial failure reported as success. - Preflight resolves a server CLI (honouring $RESP_BENCH_CLI and $SERVER_PROJECT) and PINGs the endpoint with bounded retry before any benchmark work, passing auth/TLS flags from the driver config. The per-cell FLUSHALL moved inside the guarded region, so a transient failure costs one cell rather than the sweep. Recording-only matrices skip both the probe and the flush. - Results are written under <output-dir>/<run-id>/, with --run-id defaulting to a UTC timestamp; a populated run directory is refused unless --resume or --overwrite is given. The orchestrator maintains an <output-dir>/latest symlink. - _manifest.json records per-cell outcome (status, error summary, duration, records written) plus a planned/attempted/succeeded/failed summary, and the unconditional success message is gone. Exit codes are now a contract: 0 = every attempted cell succeeded, 1 = the sweep finished with at least one failed cell, 2 = preflight failed and nothing ran. make benchmark-matrix-graphs is updated in the same change so main is never left pointing the graph generator at the base output dir: it now reads $(OUTPUT_DIR)/$(RUN_ID) with RUN_ID?=latest. Signed-off-by: James Xin <james.xin@improving.com>
…#21) Signed-off-by: James Xin <james.xin@improving.com>
) * Add Node.js benchmark engine (valkey-glide-node, ioredis, iovalkey) TypeScript engine under node/, compiled with tsc to node/dist. One event loop, one client per connection, one worker per connection; `pipeline_depth > 1` gives each connection that many independent in-flight slots. Drivers: `valkey-glide-node`, `ioredis`, `iovalkey` (+ `recording` for server-free tests). The GLIDE id is deliberately not bare `valkey-glide` — that is already Java's in the global DRIVER_ENGINE_MAP, and reusing it would reroute Java's glide runs here. Cross-engine parity verified against the Java reference, not assumed: - Key sequences byte-identical: 79,000 keys diffed against Java's real KeyGenerator across both algorithms, 1-16 workers, prime keys_count, and tight prefix padding. javaRandom.ts uses BigInt because the 48-bit LCG multiply reaches ~2^83, past what a JS number holds exactly. - HDR payloads decode in Java (org.HdrHistogram.Histogram) with matching count and percentiles. payload_b64 is encodeIntoCompressedBase64() used directly — it is already base64. - summary.min/max use getValueAtPercentile(0/100), not minNonZeroValue/maxValue. Java returns bucket-equivalent bounds; the JS properties return the raw sample, and they diverge above ~1000us (50000us reads back as 50015 in Java). - Request budget is shared across workers and claimed per request, matching Java's per-phase AtomicLong rather than pre-splitting it. - PING does not consume a key, matching Java's PingCommand. Node-specific fairness controls: ioredis/iovalkey auto-pipelining forced off (it would batch same-tick commands and inflate throughput), reconnects disabled (the default retries forever, hanging a run on a wrong host), uniform string decoding across drivers, SET payloads allocated once, and sub-millisecond rate limits yielding via setImmediate since setTimeout clamps to ~1ms. Harness: Makefile targets, DRIVER_ENGINE_MAP, both graph scripts, driver configs (default/high-throughput/example), schema examples, a benchmark-node CI job, a Node.js block in infra/provision.sh, and config-editor driver list. Docs: node/README.md and docs/BENCHMARKS_NODE.md, which records the measured single-core ceiling (throughput plateaus ~42k rps at 200 connections; at pipeline_depth 16 the process pins 99% of one core at ~72k rps, so past that the engine and not the driver is the limit). Java solves this with parallel issuer threads; the worker_threads equivalent is left as a follow-up to be justified by measurement. Also fixes the HDR range in docs/ADDING_LANGUAGE.md (3600000000 -> 600000000, the value every engine actually uses) and extends its validation checklist with the parity traps found here. 124 tests: unit (no server), full-engine tests via the recording driver, and live-server tests per driver. Signed-off-by: James Xin <james.xin@improving.com> * Wire the Node engine into the sweep automation Two gaps that made the engine unreachable from, or slow under, the matrix orchestrator and the AWS runner. Add configs/matrices/node-driver-comparison.json. Every shipped matrix was Java-only, so although DRIVER_ENGINE_MAP resolves the Node drivers correctly, no matrix actually exercised them — a default `bench-aws.sh` run would never touch the Node engine. Mirrors driver-comparison-defaults.json in shape: three drivers x five connection counts x five iterations. Make `node-build` idempotent via a stamp file. The orchestrator calls `make node-run` once per matrix cell, and `npm ci` deletes and reinstalls node_modules every time it runs: measured ~40s of pure reinstall per cell, so ~15 minutes wasted on a 75-cell sweep, plus 75 opportunities for a network blip to fail a cell mid-sweep on the AWS runner. `npm ci` now runs only when package.json/package-lock.json change; `npm run build` still runs every time because tsc is incremental and no-ops in under a second. Invocation cost drops from ~40s to ~1s. CI is unaffected — the workflow calls `npm ci && npm run build` directly, which is the right thing on a clean machine. Signed-off-by: James Xin <james.xin@improving.com> * address comment: warmup allSettled, rate-limiter scope, exact dep pins Three review items from #27. Warmup used Promise.all, which rejects on the first failure while the remaining warmup loops keep running unawaited — so executePhase's finally ran closeClients() underneath them and they rejected against closed clients with nobody awaiting. Switched to Promise.allSettled and rethrow the first rejection once every loop has settled, preserving fail-fast with nothing left in flight. RateLimiter was constructed before warmup. Its constructor sets nextAllowedNanos to "now", so the whole warmup duration was banked as credit and the workload issued (warmup_duration / interval) requests back-to-back before pacing engaged — defeating the evenly-spaced, no-burst property the limiter exists for. Now constructed after warmup. Added a regression test sized so the burst would swallow the entire workload (~500ms warmup at a 20ms interval banks ~25 free requests; the phase issues 25), verified to fail at 676ms with the bug present and pass at ~980ms without it. Pinned all six dependencies exactly instead of using caret ranges, matching Java/C#/Ruby and PR #5's "pin build inputs". Pinned to the versions already resolved in the committed lockfile, so package-lock.json is unchanged and npm ci still succeeds. This matters more for a benchmark than for ordinary code: a caret range lets `npm install` quietly measure a different client build. Reviewers: jeremyprime, Aryex. Signed-off-by: James Xin <james.xin@improving.com> * address comment: graph the Node results, and allow per-engine CI runs Two gaps in the CI wiring, found while trying to exercise benchmark-node. generate-graphs listed benchmark-node in `needs` and downloaded its artifacts, but had no Node graph step — only Java and Ruby. Node results were collected and then silently dropped. Added a Node step; DRIVER_LANGUAGE_MAP already maps the three Node driver ids to "node", so nothing else was needed. One step rather than three: the reference workload runs a single connection, and the Java/Ruby "10/100 Clients" steps all re-read that same 1-connection glob, so the extra copies would be duplicates of the same data. The workflow also took no inputs, so validating one engine meant running all of them — Java's 9 drivers plus Ruby's 2, each at 1M requests. Added an `engines` choice input (all/java/ruby/node, default all) gating each engine job and each graph step. generate-graphs needs `if: !cancelled()` because a skipped `needs` job would otherwise skip it too, which would produce no graphs at all on an engine-scoped run. Default is `all`, so the existing behaviour is unchanged. Signed-off-by: James Xin <james.xin@improving.com> * address comment: allow --language node in generate_graphs.py The Node graph step added in c780f89 failed in CI with: generate_graphs.py: error: argument --language: invalid choice: 'node' (choose from 'java', 'ruby', 'csharp', 'python') My omission: the earlier commit added the three Node driver ids to DRIVER_LANGUAGE_MAP but not to the argparse choices list, so --language node was rejected before the map was ever consulted. Added "node" to the choices and a comment noting the two lists must stay in sync. Verified by re-running the exact failing command against the artifacts from run 34914789751: 3 result records found, 9 graphs generated. Signed-off-by: James Xin <james.xin@improving.com> --------- Signed-off-by: James Xin <james.xin@improving.com>
prateek-kumar-improving
force-pushed
the
php/resp-bench
branch
from
September 15, 2026 17:18
fae89db to
df25b03
Compare
Signed-off-by: Kumar <kupratec@amazon.com>
Signed-off-by: Kumar <kupratec@amazon.com>
Signed-off-by: Kumar <kupratec@amazon.com>
Resolve conflicts from the merged Node.js engine (#27), which touched the same shared files as the PHP engine PR. All resolutions are additive — both engines coexist: - scripts/{run_benchmark_matrix,generate_graphs}.py: keep php + node driver maps - Makefile: keep both PHP and Node engine target sections and .PHONY entries - README.md: keep PHP and Node rows in languages/structure/target tables - .github/workflows/benchmark.yml: keep test-php + benchmark-php and benchmark-node jobs; add 'php' to the engines input; generate-graphs needs all four engines
prateek-kumar-improving
force-pushed
the
php/resp-bench
branch
from
September 15, 2026 19:28
df25b03 to
36df634
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.