Add Node.js benchmark engine (valkey-glide-node, ioredis, iovalkey) - #27
Open
jamesx-improving wants to merge 5 commits into
Open
jamesx-improving wants to merge 5 commits into
jamesx-improving wants to merge 5 commits into
Conversation
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>
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>
Aryex
self-requested a review
September 9, 2026 20:30
jeremyprime
reviewed
Sep 11, 2026
jeremyprime
reviewed
Sep 11, 2026
Aryex
reviewed
Sep 11, 2026
jeremyprime
reviewed
Sep 11, 2026
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>
jeremyprime
previously approved these changes
Sep 14, 2026
Aryex
previously approved these changes
Sep 14, 2026
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>
jamesx-improving
dismissed stale reviews from Aryex and jeremyprime
via
September 15, 2026 00:49
c780f89
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>
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
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.
Summary
Adds a Node.js benchmark engine (
node/, TypeScript) with three drivers:valkey-glide-node,ioredis, andiovalkey— plusrecordingforserver-free tests. Follows
docs/ADDING_LANGUAGE.md, with Java as thereference implementation.
Fixes #12
What changed
node/configs/node-driver-comparisonmatrixdocs/BENCHMARKS_NODE.md; ARCHITECTURE, CONFIG_SPECIFICATION, ADDING_LANGUAGE updatesscripts/DRIVER_ENGINE_MAP+ both graph generators' driver mapsMakefile,.github/,infra/,config-editor/,README.mdbenchmark-nodeCI job, Node block inprovision.sh, editor driver listConcurrency model: one event loop, one client per connection, one worker per
connection.
pipeline_depth > 1gives each connection that many independentin-flight slots. The per-phase request budget is shared across workers and
claimed one request at a time, matching Java's
AtomicLongrather thanpre-splitting it — so a slow connection cannot cap the run.
Driver id note: the GLIDE id is
valkey-glide-node, deliberately not barevalkey-glide. The latter is already Java's in the globalDRIVER_ENGINE_MAP,and reusing it would silently reroute Java's glide runs to the Node engine.
Cross-engine parity — verified against Java, not assumed
KeyGeneratoracross both algorithms, 1–16 workers, primekeys_count, andprefix-width edge cases.
javaRandom.tsportsjava.util.RandomusingBigInt(the 48-bit LCG multiply reaches ~2^83, past what a JSnumberholdsexactly) and is anchored to
new Random(0).nextInt() == -1155484576.org.HdrHistogram.Histogramreads ourpayload_b64with matching count and percentiles. The payload isencodeIntoCompressedBase64()used directly — it is already base64, andencoding it twice produces something Java cannot read.
summary.min/maxmatch Java's quantization. Java returns the bucket'sequivalent bounds; hdr-histogram-js'
minNonZeroValue/maxValuereturn theraw sample, and they diverge above ~1000µs (50000µs reads back as 50015 in
Java). Uses
getValueAtPercentile(0)/(100)instead, which match exactly.PingCommand, so mixing PINGinto a workload does not shift the key sequence.
Node-specific fairness controls
These have no analogue in the other engines and would otherwise make results
incomparable:
enableAutoPipelining: falseon ioredis/iovalkey — it batches commands issuedin the same event-loop tick, which would inflate throughput while looking like
a driver win.
retryStrategy: () => null— ioredis' default retries forever, so a wrong hostwould hang a run instead of failing it, and a mid-phase reconnect would fold
connection setup into request latency.
per command object, not per request.
setImmediate, sincesetTimeoutclampsto ~1ms (a 100k rps limit is a 10µs interval).
Test plan
Unit + integration (124 tests, all passing):
Full automation sweep on AWS —
node-driver-comparison, 3 drivers × 5connection counts × 5 iterations:
This exercised the whole path: clone →
provision.sh→ sweep → graphs → S3.Coefficient of variation was 0.7–3.3% across all cells. Also confirmed glide's
linux-x64-gnunative binary loads and reports2.5.2.Cross-engine key parity (needs a JDK 21 + the Java classpath):
Known gaps
Plumbed and reviewed, but not exercised by a test:
GlideClusterClient/Redis.Clusterare wired frommode: cluster, but no cluster integration test exists yet.(
credentials/username+password,useTLS/tls), untested end to end.benchmark-nodeCI job — YAML validates and mirrorsbenchmark-ruby, butthe workflow is
workflow_dispatchand has not been run.Happy to close cluster + auth in this PR if you'd prefer them in before merge.
Notes for the reviewer
Makefile,scripts/run_benchmark_matrix.py,scripts/generate_graphs.py,.github/workflows/benchmark.yml, anddocs/ADDING_LANGUAGE.md. All changeshere are additive; I'll rebase once Add Python benchmark engine (async valkey-glide, redis-py, valkey-py) #24 lands. This branch also carries the
same
docs/ADDING_LANGUAGE.mdHDR-range fix (3600000000→600000000) thatAdd Python benchmark engine (async valkey-glide, redis-py, valkey-py) #24 makes, since it is a real correctness bug in the guide.
config-editor's driver list was Java-only. I added Node's three ids;Ruby, C#, and Python are still missing from it. Out of scope here.
Delta charts render blank for any sweep without a
spring-data-valkey-glideseries #26. Not caused by this change.(10.3k vs 21.8k RPS), narrowing to ~18% at 16. I would not read that as a
driver verdict — glide's Node client routes through a Rust core over a socket,
and this workload holds one in-flight request per connection, which is where
that architecture looks worst. A
pipeline_depthsweep is the fair follow-up.docs/BENCHMARKS_NODE.mdrecords the measured single-core ceiling: throughputplateaus ~43k RPS past 4 connections, and at
pipeline_depth=16the processpins 99% of one core at ~72k RPS — past that the engine, not the driver, is the
limit.