Build each engine once per sweep instead of once per matrix cell by jamesx-improving · Pull Request #7 · Bit-Quill/resp-bench

jamesx-improving · 2026-08-21T20:15:21Z

Closes #3

Summary

Separate building from running. The orchestrator now resolves which engines the
chosen matrix actually needs, builds each of them exactly once before the sweep
starts, aborts immediately if any build fails, and dispatches each cell to a new
*-run-nobuild target that does not rebuild. The dead run_benchmark() helper
is removed. Docs are updated to describe the two-target contract.

Build-once is per engine, not global: a mixed matrix builds Java, Ruby and C#;
a Java-only matrix does not pay for the .NET or Ruby build.

Motivation — cost

Cell counts from --dry-run:

Matrix	Cells
`driver-comparison-high-tps`	720
`valkey-glide-thread-sweep`	450
`driver-comparison-defaults`	225
`lettuce-pool-sweep`	210
`valkey-glide-basic-multiclients`	25

Measured on a 6-cell run: 165.9s → 98.5s (-41%), 6 builds down to 1. On the
720-cell matrix the same change removes 719 build invocations.

Output is unchanged: NDJSON record counts are identical, and _manifest.json is
byte-identical before and after.

Why new `*-run-nobuild` targets and not `SKIP_BUILD=1`

Both interfaces were considered. SKIP_BUILD=1 is the smaller diff, but the
orchestrator passes each cell os.environ.copy() — so a SKIP_BUILD left in a
shell or a CI job environment would silently disable the build for make java-run everywhere, for every caller. "Silently benchmarked a stale jar" is
the worst possible failure mode for a benchmarking tool.

The additive targets avoid that entirely: *-run behaves exactly as it does
today for every existing caller, and skipping the build is something you have to
ask for by name. Two details worth noting in review:

Each *-run / *-run-nobuild pair shares its recipe through a variable, so
the two cannot drift apart. (For C# only the arguments are shared, because
dotnet run builds by default and the nobuild variant must pass --no-build.)
java-run: java-build java-run-nobuild was deliberately not used:
prerequisite ordering is not guaranteed under make -j, and a duplicated
prerequisite is cheaper than a race that runs a stale artifact.

What changed

Makefile — add java-run-nobuild, ruby-run-nobuild, csharp-run-nobuild;
factor each run command into a shared variable; drop the java-build
prerequisite from benchmark-matrix (the orchestrator now builds exactly the
engines the matrix needs); extend .PHONY and make help.
scripts/run_benchmark_matrix.py — add engines_for_combos() (resolve engine
per distinct driver config, reading each config once) and build_engines()
(build each engine once, sys.exit on the first failure); resolve engines up
front and dispatch cells to <engine>-run-nobuild; remove the dead
run_benchmark().
scripts/tests/test_engine_build.py — new, 9 tests.
docs/BENCHMARK_MATRIX.md — new "Engine Builds — Once Per Sweep" section and
a target-behavior table.
README.md — document the *-run-nobuild targets and add the missing
ruby-build row.
docs/ADDING_LANGUAGE.md — the worked Python example now shows the shared-
variable pattern and both run targets, so the in-flight Python engine adopts
this interface instead of reintroducing python-run: python-build.

Verification

Dispatch, via a PATH-shimmed fake make that logs argv:

Mixed matrix (Java + Ruby + C#, 12 cells): one build per engine, then 4
*-run-nobuild calls each.
Java-only matrix: exactly 1 java-build, 4 java-run-nobuild, and zero
csharp / ruby / dotnet invocations.
Injected build failure: aborts after the first failure with exit 1 — no cells
run, and the output directory is never created.

Negative test: with the jar moved aside, java-run-nobuild fails loudly
(Unable to access jarfile) instead of rebuilding — confirming there is no
hidden build left in the path.

Guard test with teeth: a parametrized test asserts that every engine in
DRIVER_ENGINE_MAP has both a -build and a -run-nobuild rule in the
Makefile. It passes today and will fail the moment a new engine is registered
without its target — python-run-nobuild does not exist yet, so this genuinely
bites rather than being decorative.

Tests: the 9 new tests pass. Full unit suite: 101 passed, 8 failed; e2e:
8 passed, 1 failed. Every one of those failures is the pre-existing
macOS /proc limitation in test_system_monitor.py — unrelated to this change
and present on the base commit.

Backward compatibility

An external consumer, valkey-io/spring-data-valkey, clones this repo at
unpinned HEAD and drives it through this Makefile, so five entry points had to
stay byte-identical in behavior:

make java-build
the work/valkey/bin/valkey-server file target
server-standalone-start / server-cluster-init
server-standalone-stop / server-cluster-stop
clean

Verified by diffing make's own database (make -qp, which parses the
Makefile without running any recipe) between base and this branch. The only
rule-level differences are: the three new phony targets, the three *-run
recipes now referencing shared variables, and benchmark-matrix losing a
prerequisite. make -n java-run produces byte-identical argv to base.

That consumer does not call make java-run at all — it runs java -jar
directly after make java-build — so it is unaffected either way.

Not verified / limitations

Stated explicitly rather than implied:

csharp-run-nobuild was never executed. The local .NET SDKs are 9.0.301
and 8.0.411 while the project targets net10.0, so dotnet build fails with
NETSDK1045. It was verified only via make -n expansion and shim dispatch.
ruby-run-nobuild was likewise never executed.
The five frozen targets above were verified by inspection and make -qp, not
by execution.
Timing figures are with a warm ~/.m2; cold-cache was not measured.

Notes for the reviewer

Sequencing: this branch will need a rebase. Another in-flight change lands
first and also touches scripts/run_benchmark_matrix.py and the Makefile's
matrix block. Flagging for ordering only — no attempt made to accommodate it
here.
Pre-existing defect preserved, not introduced: ruby-run prefixes
../$(DRIVER), so the orchestrator's absolute temp paths become
..//var/folders/... and Ruby cells cannot work in a sweep. The split
preserves that bug faithfully; it is tracked separately.

Aryex · 2026-08-25T21:07:28Z

Are we adding a test for this? Where is this run?

To follow up on this:

Do we have existing GitHub workflows and commands for these tests? If not, I don't think there is much value in adding them?

If we are going to test logic, I think that logic should be extracted to class(es). We can write unit tests for those class(es), and then the script can just be a thin wrapper over that logic.

currantw

Reviewed to f8ca40d.

currantw · 2026-08-25T22:02:39Z

+# Same as java-run but assumes the engine is already built. Used by the matrix
+# orchestrator, which builds each engine it needs once per sweep instead of
+# once per cell.
+java-run-nobuild:
+	$(JAVA_RUN_CMD)


Nit. Similar comments about unnecessary comments. Perhaps just do a sweep of this and other PRs? 🤷

currantw · 2026-08-25T22:11:09Z

 java-run: java-build
-	java -jar $(JAVA_JAR) \
-		--server $(SERVER) \
-		--driver $(DRIVER) \
-		--workload $(WORKLOAD) \
-		--metrics $(METRICS_OUTPUT)
+	$(JAVA_RUN_CMD)


Could this be written as:

java-run: java-build $(MAKE) java-run-nobuild

That would also let us get rid of the *_RUN_CMD constants, right?

currantw · 2026-08-25T22:12:38Z

 csharp-run: csharp-build
-	dotnet run --project $(CSHARP_PROJECT) -c Release -- \
-		--server $(SERVER) \
-		--driver $(DRIVER) \
-		--workload $(WORKLOAD) \
-		--metrics $(METRICS_OUTPUT)
+	dotnet run --project $(CSHARP_PROJECT) -c Release -- $(CSHARP_RUN_ARGS)


If dotnet run builds by default, why do we need the call to csharp-build as well?

currantw · 2026-08-25T22:15:04Z

 | Target | Description |
 |--------|-------------|
-| `make java-run` | Run Java engine (DRIVER, WORKLOAD, SERVER) |
-| `make ruby-run` | Run Ruby engine (DRIVER, WORKLOAD, SERVER) |
-| `make csharp-run` | Run C# engine (DRIVER, WORKLOAD, SERVER) |
+| `make java-run` | Build, then run Java engine (DRIVER, WORKLOAD, SERVER) |
+| `make ruby-run` | Build, then run Ruby engine (DRIVER, WORKLOAD, SERVER) |
+| `make csharp-run` | Build, then run C# engine (DRIVER, WORKLOAD, SERVER) |
+| `make java-run-nobuild` | Run Java engine without rebuilding (used by the matrix orchestrator) |
+| `make ruby-run-nobuild` | Run Ruby engine without re-running `bundle install` |
+| `make csharp-run-nobuild` | Run C# engine without rebuilding |
 | `make java-build` | Build Java JAR |
+| `make ruby-build` | Install Ruby dependencies |
 | `make csharp-build` | Build C# executable |


Isn't this information already specified in the Makefile? Maybe worth just pointing readers there so that this information only needs to be specified/updated in one place?

currantw · 2026-08-25T22:33:48Z


+## Engine Builds — Once Per Sweep
+
+Before the sweep starts, the orchestrator resolves the engine behind every `driver_config` (via the driver's `driver_id`) and runs `make <engine>-build` **once for each engine the matrix actually needs**. A Java-only matrix builds Java only; a mixed matrix builds Java, Ruby and C#. If a build fails the run aborts immediately, rather than failing every cell.


A Java-only matrix builds Java only; a mixed matrix builds Java, Ruby and C#

Is this accurate and/or necessary? Would C# get built even if the matrix only had Ruby and Java?

currantw · 2026-08-25T22:35:09Z

+| Target | Behavior |
+|--------|----------|
+| `make java-run` / `ruby-run` / `csharp-run` | Build, then run. Unchanged — the right target for one-off manual runs. |
+| `make java-run-nobuild` / `ruby-run-nobuild` / `csharp-run-nobuild` | Run only. Assumes the engine is already built; used by the matrix orchestrator. |


Similar to other comments: is there a single source of truth that we can point to for commands syntax – e.g. Makefile?

currantw · 2026-08-25T22:40:12Z

+    Each config file is read once, however many cells reference it.
+    """
+    driver_configs = {combo["driver_config"] for combo in series_combos}
+    return {path: detect_engine_for_driver(path) for path in driver_configs}


It seems like this methods returns a dictionary, but the caller then immediately calls values() – so could we just return a list of engines? Could we even just inline this entire method, since without that I think it might just be simplified to a one-liner?

currantw · 2026-08-25T22:41:19Z

+def build_engines(engines):
+    """Run `make <engine>-build` for each engine, aborting on the first failure.
+
+    Cells run via the `*-run-nobuild` targets, so a sweep builds each engine it
+    needs exactly once instead of once per cell.
+    """


Do we need to mention "cells" and "sweep" here? Seems more likely to confuse, and I think the first line is sufficient to explain what this does? 🤷

Suggested change

def build_engines(engines):

"""Run `make <engine>-build` for each engine, aborting on the first failure.

Cells run via the `*-run-nobuild` targets, so a sweep builds each engine it

needs exactly once instead of once per cell.

"""

def build_engines(engines):

"""Run `make <engine>-build` for each engine, aborting on the first failure."""

currantw · 2026-08-25T22:50:17Z

To follow up on this:

Do we have existing GitHub workflows and commands for these tests? If not, I don't think there is much value in adding them?

If we are going to test logic, I think that logic should be extracted to class(es). We can write unit tests for those class(es), and then the script can just be a thin wrapper over that logic.

Signed-off-by: James Xin <james.xin@improving.com>

jamesx-improving requested review from Aryex and currantw August 24, 2026 20:49

Aryex reviewed Aug 25, 2026

View reviewed changes

currantw reviewed Aug 25, 2026

View reviewed changes

prateek-kumar-improving previously approved these changes Aug 28, 2026

View reviewed changes

Build each engine once per sweep instead of once per matrix cell

382f2ca

Signed-off-by: James Xin <james.xin@improving.com>

jamesx-improving dismissed prateek-kumar-improving’s stale review via 382f2ca August 28, 2026 23:12

jamesx-improving force-pushed the fix/build-engines-once-per-sweep branch from f8ca40d to 382f2ca Compare August 28, 2026 23:12

Navigation Menu

Uh oh!

Build each engine once per sweep instead of once per matrix cell - #7

Build each engine once per sweep instead of once per matrix cell#7
jamesx-improving wants to merge 1 commit into
mainfrom
fix/build-engines-once-per-sweep

jamesx-improving commented Aug 21, 2026

Uh oh!

Aryex Aug 25, 2026

Uh oh!

currantw Aug 25, 2026

Uh oh!

currantw left a comment

Uh oh!

currantw Aug 25, 2026

Uh oh!

currantw Aug 25, 2026

Uh oh!

currantw Aug 25, 2026

Uh oh!

currantw Aug 25, 2026

Uh oh!

currantw Aug 25, 2026

Uh oh!

currantw Aug 25, 2026

Uh oh!

currantw Aug 25, 2026

Uh oh!

currantw Aug 25, 2026

Uh oh!

currantw Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		## Engine Builds — Once Per Sweep

		Before the sweep starts, the orchestrator resolves the engine behind every `driver_config` (via the driver's `driver_id`) and runs `make <engine>-build` once for each engine the matrix actually needs. A Java-only matrix builds Java only; a mixed matrix builds Java, Ruby and C#. If a build fails the run aborts immediately, rather than failing every cell.

Uh oh!

Conversation

jamesx-improving commented Aug 21, 2026

Summary

Motivation — cost

Why new *-run-nobuild targets and not SKIP_BUILD=1

What changed

Verification

Backward compatibility

Not verified / limitations

Notes for the reviewer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

currantw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Why new `*-run-nobuild` targets and not `SKIP_BUILD=1`