-
Notifications
You must be signed in to change notification settings - Fork 1
Build each engine once per sweep instead of once per matrix cell #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,10 +30,10 @@ WORK_DIR=$(shell pwd)/work | |
| server-cluster-start server-cluster-stop server-cluster-init \ | ||
| server-sentinel-start server-sentinel-stop \ | ||
| server-start server-stop \ | ||
| java-build java-test java-run java-clean \ | ||
| java-build java-test java-run java-run-nobuild java-clean \ | ||
| python-build python-test python-run python-clean \ | ||
| ruby-build ruby-test ruby-run ruby-clean ruby-info \ | ||
| csharp-build csharp-test csharp-run csharp-clean csharp-info \ | ||
| ruby-build ruby-test ruby-run ruby-run-nobuild ruby-clean ruby-info \ | ||
| csharp-build csharp-test csharp-run csharp-run-nobuild csharp-clean csharp-info \ | ||
| config-editor-build config-editor-dev | ||
|
|
||
| # ============================================================================ | ||
|
|
@@ -54,6 +54,7 @@ help: | |
| @echo " make java-build Build Java benchmark engine" | ||
| @echo " make java-test Run Java tests" | ||
| @echo " make java-run Run Java benchmark (requires DRIVER and WORKLOAD)" | ||
| @echo " make java-run-nobuild Run Java benchmark without rebuilding first" | ||
| @echo " make java-clean Clean Java build artifacts" | ||
| @echo "" | ||
| @echo "Python Engine (placeholder):" | ||
|
|
@@ -65,12 +66,14 @@ help: | |
| @echo " make ruby-build Install Ruby dependencies" | ||
| @echo " make ruby-test Run Ruby tests" | ||
| @echo " make ruby-run Run Ruby benchmark (requires DRIVER and WORKLOAD)" | ||
| @echo " make ruby-run-nobuild Run Ruby benchmark without re-running bundle install" | ||
| @echo " make ruby-info Show supported Ruby drivers and commands" | ||
| @echo "" | ||
| @echo "C# Engine:" | ||
| @echo " make csharp-build Build C# benchmark engine" | ||
| @echo " make csharp-test Run C# tests" | ||
| @echo " make csharp-run Run C# benchmark (requires DRIVER and WORKLOAD)" | ||
| @echo " make csharp-run-nobuild Run C# benchmark without rebuilding first" | ||
| @echo " make csharp-clean Clean C# build artifacts" | ||
| @echo " make csharp-info Show supported C# drivers and commands" | ||
| @echo "" | ||
|
|
@@ -284,12 +287,24 @@ java-integration-test: server-standalone-start | |
| cd java && VALKEY_HOST=localhost VALKEY_PORT=6379 mvn test -DincludeIntegrationTests | ||
| $(MAKE) server-standalone-stop | ||
|
|
||
| # The run command is held in a variable so `java-run` and `java-run-nobuild` | ||
| # cannot drift apart. Deliberately not `java-run: java-build java-run-nobuild`: | ||
| # prerequisite ordering is not guaranteed under `make -j`, and a benchmark that | ||
| # silently runs a stale jar is worse than a duplicated prerequisite. | ||
| JAVA_RUN_CMD=java -jar $(JAVA_JAR) \ | ||
| --server $(SERVER) \ | ||
| --driver $(DRIVER) \ | ||
| --workload $(WORKLOAD) \ | ||
| --metrics $(METRICS_OUTPUT) | ||
|
|
||
| java-run: java-build | ||
| java -jar $(JAVA_JAR) \ | ||
| --server $(SERVER) \ | ||
| --driver $(DRIVER) \ | ||
| --workload $(WORKLOAD) \ | ||
| --metrics $(METRICS_OUTPUT) | ||
| $(JAVA_RUN_CMD) | ||
|
|
||
| # 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) | ||
|
Comment on lines
+303
to
+307
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit. Similar comments about unnecessary comments. Perhaps just do a sweep of this and other PRs? 🤷 |
||
|
|
||
| java-run-cluster: java-build | ||
| java -jar $(JAVA_JAR) \ | ||
|
|
@@ -342,12 +357,18 @@ ruby-integration-test: server-standalone-start | |
| cd ruby && VALKEY_HOST=localhost VALKEY_PORT=6379 bundle exec rake integration | ||
| $(MAKE) server-standalone-stop | ||
|
|
||
| RUBY_RUN_CMD=cd ruby && bundle exec ruby bin/resp-bench \ | ||
| --server $(SERVER) \ | ||
| --driver ../$(DRIVER) \ | ||
| --workload ../$(WORKLOAD) \ | ||
| --metrics ../$(METRICS_OUTPUT) | ||
|
|
||
| ruby-run: ruby-build | ||
| cd ruby && bundle exec ruby bin/resp-bench \ | ||
| --server $(SERVER) \ | ||
| --driver ../$(DRIVER) \ | ||
| --workload ../$(WORKLOAD) \ | ||
| --metrics ../$(METRICS_OUTPUT) | ||
| $(RUBY_RUN_CMD) | ||
|
|
||
| # Same as ruby-run but skips `bundle install` — see java-run-nobuild. | ||
| ruby-run-nobuild: | ||
| $(RUBY_RUN_CMD) | ||
|
|
||
| ruby-clean: | ||
| cd ruby && rm -rf vendor .bundle Gemfile.lock | ||
|
|
@@ -372,12 +393,21 @@ csharp-integration-test: server-standalone-start | |
| cd csharp && VALKEY_HOST=localhost VALKEY_PORT=6379 dotnet test --filter "Category=Integration" | ||
| $(MAKE) server-standalone-stop | ||
|
|
||
| # Only the arguments are shared here, not the whole command: unlike java and | ||
| # ruby, the two C# recipes genuinely differ — `dotnet run` builds by default, so | ||
| # the nobuild variant has to pass --no-build. | ||
| CSHARP_RUN_ARGS=--server $(SERVER) \ | ||
| --driver $(DRIVER) \ | ||
| --workload $(WORKLOAD) \ | ||
| --metrics $(METRICS_OUTPUT) | ||
|
|
||
| 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) | ||
|
Comment on lines
404
to
+405
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
|
|
||
| # Same as csharp-run but assumes the engine is already built, so `dotnet run` | ||
| # skips its own incremental build too — see java-run-nobuild. | ||
| csharp-run-nobuild: | ||
| dotnet run --project $(CSHARP_PROJECT) -c Release --no-build -- $(CSHARP_RUN_ARGS) | ||
|
|
||
| csharp-clean: | ||
| cd csharp && dotnet clean | ||
|
|
@@ -408,7 +438,9 @@ GRAPHS_DIR?=graphs/interactive/ | |
| RUN_ID?=latest | ||
| MATRIX_RESULTS_DIR=$(OUTPUT_DIR)/$(RUN_ID) | ||
|
|
||
| benchmark-matrix: java-build | ||
| # No build prerequisite: the orchestrator builds every engine the matrix needs | ||
| # (and only those) once, before the sweep starts. | ||
| benchmark-matrix: | ||
| python scripts/run_benchmark_matrix.py \ | ||
| --matrix $(MATRIX) \ | ||
| --output-dir $(OUTPUT_DIR) \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,12 +198,18 @@ See [docs/CONFIG_SPECIFICATION.md](docs/CONFIG_SPECIFICATION.md) for full detail | |
|
|
||
| | 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 | | ||
|
Comment on lines
199
to
209
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this information already specified in the |
||
|
|
||
| The matrix orchestrator builds each engine the matrix needs once per sweep and then uses the `*-run-nobuild` targets per cell — see [docs/BENCHMARK_MATRIX.md](docs/BENCHMARK_MATRIX.md#engine-builds--once-per-sweep). | ||
|
|
||
| ### Server Management | ||
|
|
||
| | Target | Description | | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,19 @@ make benchmark-matrix-graphs OUTPUT_DIR=results/glide-sweep | |
| make benchmark-matrix-graphs OUTPUT_DIR=results/glide-sweep RUN_ID=20260321T140322Z | ||
| ``` | ||
|
|
||
| ## 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. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this accurate and/or necessary? Would C# get built even if the matrix only had Ruby and Java? |
||
|
|
||
| Individual cells then execute `make <engine>-run-nobuild`, which runs the already-built engine without rebuilding it. This matters because sweeps are large — `driver-comparison-high-tps` is 720 cells — and `*-build` is not incremental (`mvn clean package`, `bundle install`, `dotnet build -c Release`). | ||
|
|
||
| | 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. | | ||
|
Comment on lines
+47
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to other comments: is there a single source of truth that we can point to for commands syntax – e.g. |
||
|
|
||
| `make benchmark-matrix` no longer pre-builds Java itself, since the orchestrator builds exactly the engines the chosen matrix requires. | ||
|
|
||
| ## Matrix Config Format | ||
|
|
||
| Matrix configs live in `configs/matrices/` and define **dimensions** to sweep: | ||
|
|
@@ -234,4 +247,4 @@ python scripts/run_benchmark_matrix.py --help | |
|
|
||
| `GLIDE_TOKIO_WORKER_THREADS` and `GLIDE_CALLBACK_WORKER_THREADS` are **process-level environment variables** consumed by the native Rust/Tokio runtime inside the valkey-glide JAR. They are read once when `GlideClient.createClient()` first initializes the process-wide Tokio runtime, and cannot be changed afterward. | ||
|
|
||
| Because the matrix runner launches each benchmark as a separate JVM process (via `make java-run`), different env var values can be set per run. These are specified in the matrix config's `env` dimension, NOT in the driver config JSON. | ||
| Because the matrix runner launches each benchmark as a separate JVM process (via `make java-run-nobuild`), different env var values can be set per run. These are specified in the matrix config's `env` dimension, NOT in the driver config JSON. | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -722,23 +722,30 @@ def count_ndjson_lines(path, offset=0): | |||||||||||||||||
| return sum(1 for line in f if line.strip()) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def run_benchmark(server, driver_file, workload_file, metrics_output, env_overrides=None): | ||||||||||||||||||
| """Run a single benchmark via `make java-run`.""" | ||||||||||||||||||
| env = os.environ.copy() | ||||||||||||||||||
| if env_overrides: | ||||||||||||||||||
| env.update({k: str(v) for k, v in env_overrides.items()}) | ||||||||||||||||||
| def engines_for_combos(series_combos): | ||||||||||||||||||
| """Map each distinct driver config path to the engine that runs it. | ||||||||||||||||||
|
|
||||||||||||||||||
| 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} | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like this methods returns a dictionary, but the caller then immediately calls |
||||||||||||||||||
|
|
||||||||||||||||||
| subprocess.run( | ||||||||||||||||||
| [ | ||||||||||||||||||
| "make", "java-run", | ||||||||||||||||||
| f"SERVER={server}", | ||||||||||||||||||
| f"DRIVER={driver_file}", | ||||||||||||||||||
| f"WORKLOAD={workload_file}", | ||||||||||||||||||
| f"METRICS_OUTPUT={metrics_output}", | ||||||||||||||||||
| ], | ||||||||||||||||||
| check=True, | ||||||||||||||||||
| env=env, | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| 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. | ||||||||||||||||||
| """ | ||||||||||||||||||
|
Comment on lines
+734
to
+739
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
|
||||||||||||||||||
| for engine in engines: | ||||||||||||||||||
| target = f"{engine}-build" | ||||||||||||||||||
| print(f"\n=== building engine: make {target} ===", flush=True) | ||||||||||||||||||
| result = subprocess.run(["make", target]) | ||||||||||||||||||
| if result.returncode != 0: | ||||||||||||||||||
| sys.exit( | ||||||||||||||||||
| f"ERROR: 'make {target}' failed with exit code " | ||||||||||||||||||
| f"{result.returncode}; aborting matrix run" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| # ═══════════════════════════════════════════════════════════════════════════════ | ||||||||||||||||||
|
|
@@ -921,6 +928,11 @@ def run_matrix(config, output_dir, server_host, port, run_id=None, resume=False, | |||||||||||||||||
|
|
||||||||||||||||||
| run_id = run_id or default_run_id() | ||||||||||||||||||
|
|
||||||||||||||||||
| # Resolve the engine per driver config up front, so the build phase and the | ||||||||||||||||||
| # per-cell run phase can never disagree about which engine a series uses. | ||||||||||||||||||
| engine_by_driver = engines_for_combos(series_combos) | ||||||||||||||||||
| engines = sorted(set(engine_by_driver.values())) | ||||||||||||||||||
|
|
||||||||||||||||||
| print("=" * 70) | ||||||||||||||||||
| print(f"Matrix Benchmark Run") | ||||||||||||||||||
| print(f" Description: {config['description']}") | ||||||||||||||||||
|
|
@@ -936,6 +948,7 @@ def run_matrix(config, output_dir, server_host, port, run_id=None, resume=False, | |||||||||||||||||
| print(f" bindings: {combo['bindings']}") | ||||||||||||||||||
| print(f" Iterations: {iterations}") | ||||||||||||||||||
| print(f" Total runs: {total_cells}") | ||||||||||||||||||
| print(f" Engines: {', '.join(engines)}") | ||||||||||||||||||
| print("=" * 70) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Preflight — refuse to start rather than dying part-way through the sweep | ||||||||||||||||||
|
|
@@ -953,6 +966,10 @@ def run_matrix(config, output_dir, server_host, port, run_id=None, resume=False, | |||||||||||||||||
| if latest_link: | ||||||||||||||||||
| print(f"Preflight: {latest_link} -> {run_id}") | ||||||||||||||||||
|
|
||||||||||||||||||
| # Build each needed engine exactly once, now that preflight has confirmed the | ||||||||||||||||||
| # server is reachable — no point compiling if the sweep can't run. | ||||||||||||||||||
| build_engines(engines) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Write manifest | ||||||||||||||||||
| write_manifest(results_dir, config, series_combos, run_id=run_id, resumed=resume) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -1016,10 +1033,10 @@ def run_matrix(config, output_dir, server_host, port, run_id=None, resume=False, | |||||||||||||||||
| if env_overrides: | ||||||||||||||||||
| bench_env.update({k: str(v) for k, v in env_overrides.items()}) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Auto-detect engine from driver config | ||||||||||||||||||
| engine = detect_engine_for_driver(str(driver_path)) | ||||||||||||||||||
| # Engine was resolved and built before the sweep started | ||||||||||||||||||
| engine = engine_by_driver[driver_cfg_path] | ||||||||||||||||||
| bench_cmd = [ | ||||||||||||||||||
| "make", f"{engine}-run", | ||||||||||||||||||
| "make", f"{engine}-run-nobuild", | ||||||||||||||||||
| f"SERVER={server}", | ||||||||||||||||||
| f"DRIVER={str(driver_path)}", | ||||||||||||||||||
| f"WORKLOAD={str(workload_path)}", | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be written as:
java-run: java-build $(MAKE) java-run-nobuildThat would also let us get rid of the
*_RUN_CMDconstants, right?