Stop the bundled benchmarks comparing daisy against itself - #78
Merged
Conversation
The `gerbera -> daisy` rename (2c1a382) collapsed both arms of every comparison onto the same package. Each script ended up defining `bench_daisy` twice — the second definition shadowing the first, so one arm of every comparison was silently discarded and both columns ran the same code — and building result dicts with a duplicate `"daisy"` / `"daisy_s"` key, where the later value wins. Running them on v2.0 reports a "speedup" of 1.0-1.1x across the board, which is what measuring one implementation against itself looks like. The five committed artifacts encode that. `dep_graph_results.json` and `worker_scaling_results.json` contain literal duplicate JSON object keys, which a Python dict literal cannot produce — they were text-substituted by the rename rather than regenerated — and `block_scaling_results.json` reports a 28.44x speedup on the 100-block row that is pure inter-run noise. There is no second implementation in this repository to compare against, so the vestigial daisy-1.x arm is dropped rather than restored: - bench_dependency_graph.py becomes a single-implementation scaling benchmark: how enumeration cost grows with block count, conflict levels and context. One arm, no ratio, flat result records. - bench_worker_scaling.py keeps the two `process_function` modalities that do still exist — 1-arg block function vs 0-arg worker function, both running in worker subprocesses since aae3e6d — and drops the third phantom series from the plot. Three further corrections were needed for the regenerated numbers to mean anything: - The worker-scaling curve now uses the distributed `Server` at every worker count. It previously switched to the in-process `SerialServer` at `workers=1` for the block-fn arm only, so that point measured a different execution model than the rest of its own line (and than the worker-fn arm, which always used `Server`). - `progress=False`, so tqdm rendering is not inside a timed region that claims to isolate coordination overhead. - Success is asserted as `completed_count == total_block_count`, not `is_done()`. `is_done()` only means the counters balance, which is also true when every block failed — a run that processed nothing passed the old assertion and reported a fast time for it. All five artifacts were regenerated by running the fixed scripts; nothing was hand-edited. The scripts are now clean under the project's ruff configuration, so the `extend-exclude = ["benchmarks"]` escape hatch (whose comment asked for exactly this rewrite) is removed. This changes no conclusion documented elsewhere: the subprocess-vs-thread figures in `_worker_processes.py` come from `bench_workload_mix` (see 7e0ebc8), a script that was never committed, not from these two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
Oh good catch. I haven't revisited these benchmarks in a little while. Thanks for cleaning them up. I'm curious why the block function is slower than the worker function, they should be pretty identical at this point. But that can be something for future investigation. |
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.
Why
The
gerbera -> daisyrename (2c1a382) collapsed both arms of every comparison inbenchmarks/onto the same package. The damage is mechanical and easy to miss:bench_daisytwice (bench_dependency_graph.pyL19/L47,bench_worker_scaling.pyL23/L53). The second shadows the first, so one arm of every comparison is silently discarded and both reported columns run the same code;bench_worker_scaling.pybuilds its result dicts with the key"daisy_s"twice, andbench_dependency_graph.pywith"daisy"twice. The later value wins, so one arm never even reaches the JSON.Running the scripts unchanged on current
v2.0shows what that produces — a "speedup" of 1.0x, i.e. the shape of one implementation measured against itself:The five committed artifacts encode the same thing, and two of them prove they were text-substituted by the rename rather than regenerated — a Python dict literal cannot emit a duplicate key, but the committed JSON has one:
And
block_scaling_results.jsonadvertised a 28.44x speedup on the 100-block row, which is inter-run noise on a ~0.2s measurement of the same code path.What this does — drop the vestigial arm, don't restore it
git show 2c1a382^:benchmarks/bench_dependency_graph.pysettles the question: the firstbench_*in each file was the old pure-Python daisy 1.x package, the second wasgerbera, the Rust rewrite that became this repository. There is no second implementation here to compare against and no way to obtain one from this tree, so option (a):bench_dependency_graph.pyis now a single-implementation scaling benchmark — how enumeration cost grows with block count, conflict levels and context. One arm, no ratio, flat result records (blocks,levels,build_s,iter_s,total_s,blocks_per_s).bench_worker_scaling.pykeeps the twoprocess_functionmodalities that genuinely still exist and drops the phantom third series from the plot: 1-arg block-fn (the worker subprocess drivesClient.acquire_block()) vs 0-arg worker-fn (the subprocess calls the function, which drives its own loop). Both run in worker subprocesses sinceaae3e6d, so this is one execution model, two callback shapes.Three further corrections were required for the regenerated numbers to mean anything. Each is called out because each moves a number:
The worker-scaling curve now uses the distributed
Serverat every worker count. It previously switched to the in-processSerialServeratworkers=1for the block-fn arm only — so that point measured a different execution model than the rest of its own line, and than the worker-fn arm, which always usedServer. This is why block-fn atworkers=1moves from 0.075s to ~2.2s: the old value was in-process serial execution with no TCP and no subprocess, mislabelled as "1 worker".progress=False. tqdm rendering was inside a timed region whose docstring claims to isolate coordination overhead.Success is
completed_count == total_block_count, notis_done().TaskState.is_done()means "the counters balance", which is also true when every block failed. A run that completed nothing passed the old assertion and reported a fast time for it — observed while setting this up, beforefunlib.geometrywas present in the venv:0.33s,
done=True, 0 of 1000 blocks completed, andassert g_donewas satisfied.Also updated because they describe exactly what changed:
ARCHITECTURE.md's one-line description ofbenchmarks/("Throughput comparisons vs daisy 1.x" — no longer true), a CHANGELOG entry, and the[tool.ruff] extend-exclude = ["benchmarks"]escape hatch, whose comment ("they need a rewrite, not a lint pass") asked for precisely this.Ruff, before and after
Before, on
origin/v2.0(annotation bodies elided for length;--select F811,F601was passed explicitly because base excludesbenchmarks/from lint entirely):After, with the exclusion removed so the project's own rule set applies:
ruff check .does not pass on this branch — and does not pass onorigin/v2.0either. A pristineorigin/v2.0worktree with the pinnedruff==0.16.1reports 28I001errors, all intests/, none inbenchmarks/, and this branch reports the identical 28. This PR does not touchtests/and neither introduces nor fixes them; removing thebenchmarksexclusion contributes none of them. Worth a separate PR.Regenerated artifacts
All five were produced by running the fixed scripts — nothing was hand-edited. Verbatim output of the run that produced the committed files:
Both scripts are cheap: 15s and 17s wall respectively. The JSON now round-trips with no duplicate keys:
The numbers are now monotone in worker count and consistent between the two block-scaling views, which the old self-comparing ones were not. Worker-fn beating block-fn by 1.2-2.3x is a real, reproducible difference between the two callback shapes (it held across three runs); I have deliberately not speculated in the docstrings about why, since I did not profile it. Note both arms go through the v1-compat
Blockboundary —v1_compatmonkey-patchesClient.acquire_block— so compat wrapping is not the explanation.Environment: AMD EPYC 9454 (96 CPUs), Linux 6.8, CPython 3.12.3,
maturin develop --release, daisy 2.0.0 at542c3c2. Timings are machine-specific; the docstring now says so.Running the benchmarks needs
matplotlib,cloudpickleand — less obviously —funlib.geometry, which is not a declared dependency of daisy anywhere but is imported byv1_compat.Block.read_roi, so every distributed 1-arg block function fails without it. Reported separately; not fixed here.Does this change any documented conclusion?
No. The report that prompted this suspected these scripts were the evidence base for the subprocess-vs-thread defaults in
daisy-py/python/daisy/_worker_processes.py("threads were 1.7x slower at 10% python glue, 28x at 100%", "~15%"). They are not.git log -S "python glue"traces that text to7e0ebc8, which names its source explicitly:bench_workload_mixhas never existed in this repository (git log --all --diff-filter=A -- '*workload_mix*'is empty), so those figures are unreproducible from the tree either way — a separate gap, not this PR's to close. The28x/28.44xnear-collision withblock_scaling_results.jsonis a coincidence: different workload, different worker count, different quantity.Skeptical pass
ARCHITECTURE.mdline, CHANGELOG entry and ruff-exclusion removal all describe or gate exactly that.daisy-py/,daisy-core/untouched), no tests touched, no dependencies added..gitignorechange on top — but they regenerate in ~30s total, so keeping them is cheap and they are the only committed record of the shapes.