Skip to content

Add Node.js benchmark engine (valkey-glide-node, ioredis, iovalkey) - #27

Open
jamesx-improving wants to merge 5 commits into
mainfrom
feat/add-node-engine
Open

jamesx-improving wants to merge 5 commits into
mainfrom
feat/add-node-engine

Conversation

@jamesx-improving

Copy link
Copy Markdown

Summary

Adds a Node.js benchmark engine (node/, TypeScript) with three drivers:
valkey-glide-node, ioredis, and iovalkey — plus recording for
server-free tests. Follows docs/ADDING_LANGUAGE.md, with Java as the
reference implementation.

Fixes #12

What changed

Area Files What
node/ 48 The engine: config parsing, key generation, rate limiter, HDR metrics, NDJSON writer, four drivers, 124 tests
configs/ 11 Driver configs (default / high-throughput / example) + node-driver-comparison matrix
docs/ 4 New BENCHMARKS_NODE.md; ARCHITECTURE, CONFIG_SPECIFICATION, ADDING_LANGUAGE updates
scripts/ 3 DRIVER_ENGINE_MAP + both graph generators' driver maps
Makefile, .github/, infra/, config-editor/, README.md 5 Make targets, benchmark-node CI job, Node block in provision.sh, editor driver list

Concurrency model: one event loop, one client per connection, one worker per
connection. pipeline_depth > 1 gives each connection that many independent
in-flight slots. The per-phase request budget is shared across workers and
claimed one request at a time, matching Java's AtomicLong rather than
pre-splitting it — so a slow connection cannot cap the run.

Driver id note: the GLIDE id is valkey-glide-node, deliberately not bare
valkey-glide. The latter is already Java's in the global DRIVER_ENGINE_MAP,
and reusing it would silently reroute Java's glide runs to the Node engine.

Cross-engine parity — verified against Java, 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
    prefix-width edge cases. javaRandom.ts ports java.util.Random using
    BigInt (the 48-bit LCG multiply reaches ~2^83, past what a JS number holds
    exactly) and is anchored to new Random(0).nextInt() == -1155484576.
  • HDR payloads decode in Java. org.HdrHistogram.Histogram reads our
    payload_b64 with matching count and percentiles. The payload is
    encodeIntoCompressedBase64() used directly — it is already base64, and
    encoding it twice produces something Java cannot read.
  • summary.min/max match Java's quantization. Java returns the bucket's
    equivalent bounds; hdr-histogram-js' minNonZeroValue/maxValue return the
    raw sample, and they diverge above ~1000µs (50000µs reads back as 50015 in
    Java). Uses getValueAtPercentile(0)/(100) instead, which match exactly.
  • PING does not consume a key, matching Java's PingCommand, so mixing PING
    into 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: false on ioredis/iovalkey — it batches commands issued
    in the same event-loop tick, which would inflate throughput while looking like
    a driver win.
  • retryStrategy: () => null — ioredis' default retries forever, so a wrong host
    would hang a run instead of failing it, and a mid-phase reconnect would fold
    connection setup into request latency.
  • Uniform string decoding across all three drivers; SET payloads allocated once
    per command object, not per request.
  • Sub-millisecond rate limits yield via setImmediate, since setTimeout clamps
    to ~1ms (a 100k rps limit is a 10µs interval).

Test plan

Unit + integration (124 tests, all passing):

make node-unit-test                       # 83 tests, no server
make node-integration-test                # recording driver + live-server per driver

Full automation sweep on AWSnode-driver-comparison, 3 drivers × 5
connection counts × 5 iterations:

job:      node-full-bench-20260908-235705-vrzjrp   (commit a8fb0c7)
instance: m5.2xlarge, us-east-1, Amazon Linux 2023 x86_64
result:   75/75 cells, 0 failed, 0 errors, status: succeeded
provision 4m32s · sweep 19m25s · self-terminated

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-gnu native binary loads and reports 2.5.2.

Cross-engine key parity (needs a JDK 21 + the Java classpath):

# Dumps Java's KeyGenerator output and diffs it against the Node generator.
# See docs/BENCHMARKS_NODE.md § Cross-Engine Parity.

Known gaps

Plumbed and reviewed, but not exercised by a test:

  • Cluster modeGlideClusterClient / Redis.Cluster are wired from
    mode: cluster, but no cluster integration test exists yet.
  • Auth and TLS — config is threaded through to every driver
    (credentials/username+password, useTLS/tls), untested end to end.
  • benchmark-node CI job — YAML validates and mirrors benchmark-ruby, but
    the workflow is workflow_dispatch and has not been run.

Happy to close cluster + auth in this PR if you'd prefer them in before merge.

Notes for the reviewer

  • Expect a conflict with Add Python benchmark engine (async valkey-glide, redis-py, valkey-py) #24 (Python engine) on Makefile,
    scripts/run_benchmark_matrix.py, scripts/generate_graphs.py,
    .github/workflows/benchmark.yml, and docs/ADDING_LANGUAGE.md. All changes
    here 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.md HDR-range fix (3600000000600000000) that
    Add 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.
  • The report's delta chart is blank for this matrix — pre-existing, filed as
    Delta charts render blank for any sweep without a spring-data-valkey-glide series #26. Not caused by this change.
  • On the numbers: the sweep shows glide ~2× behind ioredis at 1 connection
    (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_depth sweep is the fair follow-up.
    docs/BENCHMARKS_NODE.md records the measured single-core ceiling: throughput
    plateaus ~43k RPS past 4 connections, and at pipeline_depth=16 the process
    pins 99% of one core at ~72k RPS — past that the engine, not the driver, is the
    limit.

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
Aryex self-requested a review September 9, 2026 20:30
Comment thread node/src/engine/benchmark.ts Outdated
Comment thread node/package.json
Comment thread configs/drivers/default/valkey-glide-node.json
Comment thread node/src/engine/benchmark.ts Outdated
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
jeremyprime previously approved these changes Sep 14, 2026
Aryex
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
jamesx-improving dismissed stale reviews from Aryex and jeremyprime via c780f89 September 15, 2026 00:49
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Node.js engine

3 participants