Skip to content

feat(bench): EOP by default + --query for query-context measurement#601

Merged
Kamirus merged 2 commits into
mainfrom
feat/bench-query-eop
Jun 30, 2026
Merged

feat(bench): EOP by default + --query for query-context measurement#601
Kamirus merged 2 commits into
mainfrom
feat/bench-query-eop

Conversation

@alexandru-uta

Copy link
Copy Markdown
Contributor

mops bench compiled benchmark canisters with --legacy-persistence and ran every cell as an update call — neither reflects how a real query canister executes on the IC. This makes the instruction counts unrepresentative for read-only/query workloads (they include GC work a query never does, under the wrong persistence mode).

Changes

  • EOP by defaultgetMocArgs no longer forces --legacy-persistence (it was gated only on moc ≥ 0.15). The bench canister is already a persistent actor, so dropping the flag compiles it under enhanced orthogonal persistence — moc's default and the mode real canisters run.
  • --query — measures each cell via the canister's existing runCellQuery (a query method) instead of runCellUpdateAwait. Queries run no GC on the IC, so the counts exclude GC work an update would incur. Synchronous runners only (async/inter-canister runners still need the update path).
  • Verbose pipeline now prints context (query/update) and persistence.

No change to the mops-bench library — the canister template already exposes runCellQuery.

Validation

  • npm run check (tsc) passes.
  • Smoke-tested via tsx against a real project: verbose prints context: query / persistence: enhanced / gc: incremental, and the measured numbers shift as expected — the real-runWith floor drops sharply (no GC counted) while raw allocation rises slightly under 64-bit EOP.

Docs / changelog

  • docs/docs/cli/4-dev/02-mops-bench.md: updated "How it works" + new ### --query section.
  • cli/CHANGELOG.md: entries under ## Next.
  • --help carries a description for --query.
  • The mops-cli skill documents no individual bench flags, so nothing there to update.

🤖 Generated with Claude Code

mops bench compiled benchmark canisters with --legacy-persistence and
ran every cell as an update call — neither reflects how a real query
canister executes. Two changes:

- Compile under enhanced orthogonal persistence (moc's default since
  0.15) instead of forcing --legacy-persistence. The bench canister is
  already a `persistent actor`, so this just drops the flag.
- Add --query: measure each cell via the canister's existing
  runCellQuery (a query method) instead of runCellUpdateAwait. Queries
  run no GC on the IC, so instruction counts exclude GC work an update
  would incur — realistic for query/read-only workloads. Synchronous
  runners only (async runners need the update path).

Verbose pipeline now prints context (query/update) and persistence.
Docs + changelog updated. mops-bench library needs no change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Cursor AI review

👍 APPROVE — looks safe to merge

Category Assessment Details
Summary Switches mops bench to EOP-by-default (was forced --legacy-persistence on moc ≥ 0.15) and adds --query / --legacy-persistence flags so measurements can match real query-canister execution; docs, changelog, and --help updated.
Code Quality Minimal, focused diff reusing existing runCellQuery in the bench canister template; no speculative helpers.
Consistency Commander options wire cleanly into BenchOptions; sibling docs/changelog/--help stay aligned per project rules.
Security Local dev/benchmark tooling only; no auth, registry, or signing surface touched.
Tests No bench Jest coverage pre-existed; change is localized and author smoke-tested; no redundant tests added.
Maintainability Clear comments and verbose pipeline output for the new modes; aligns with existing TODO to drop legacy-persistence default.

Verdict

Decision: APPROVE
Risk: Low
Reason: The behavioral changes are intentional, well-documented, and provide --legacy-persistence as an opt-out; implementation correctly gates the persistence flag on moc version and routes --query through the existing canister query method without touching production CLI/registry paths.


Generated for commit 6c3d9aa

@alexandru-uta alexandru-uta requested a review from Kamirus June 30, 2026 06:36
EOP stays the default, but add a --legacy-persistence flag so a canister
that still uses legacy persistence can be benchmarked. Only emitted for
moc >= 0.15 (legacy is already the default below that). Verbose pipeline
prints the active persistence mode. Docs + changelog updated.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Kamirus Kamirus merged commit 8ace3f4 into main Jun 30, 2026
26 checks passed
@Kamirus Kamirus deleted the feat/bench-query-eop branch June 30, 2026 08:56
Kamirus added a commit that referenced this pull request Jun 30, 2026
…ence for legacy collectors (#606)

`mops bench` (no `--gc` flag) crashed on moc 0.15+ — a regression from
2.16.0 ([#601](#601)), which
made enhanced orthogonal persistence the default bench mode but left the
default `--gc` as `copying`. moc fixes the GC to `incremental` under EOP
and rejects the legacy collectors there, so the default run was an
invalid flag combination.

**Before** — default `mops bench`:

```
moc-wrapper -c --idl canister.mo --force-gc --copying-gc --release ...
moc: Invalid compiler flag combination: --copying-gc is not supported with --enhanced-orthogonal-persistence
Error: benchmarks failed
```

**After** — default `mops bench` compiles under EOP with no collector
flag (incremental is moc's fixed GC there):

```
moc-wrapper -c --idl canister.mo --force-gc --release ...
```

`--gc copying|compacting|generational` still works: those collectors
only exist under legacy persistence, so selecting one now implies
`--legacy-persistence` (instead of crashing with an opaque moc error).
`--gc incremental` or omitting `--gc` keeps measuring under EOP.

## Why the bug evaded detection

#601 switched bench to EOP-by-default and smoke-tested with `--gc
incremental`, but the `--gc` *default* stayed `copying`, so the
unflagged `mops bench` invocation everyone runs was never exercised.
More fundamentally, there was **no `mops bench` end-to-end test at all**
— and this repo's own `mops.toml` pins `moc 0.14.14` (pre-EOP), so even
the local bench never touched the EOP path. The crash only surfaced on a
downstream project (motoko-core) running a newer moc.

This PR adds an e2e test (`cli/tests/bench.test.ts` + `cli/tests/bench/`
fixture) that pins `moc 1.3.0` + `pocket-ic 12.0.0` and runs `mops
bench` for real, asserting exit 0 and no `Invalid compiler flag
combination`. Verified it fails against the pre-fix state with the
original moc error and passes with the fix.

## Migration

Default benchmark numbers shift from `copying` (legacy) to `incremental`
(EOP) — but 2.16.0 already shifted numbers by switching persistence
modes, and `incremental` is the only GC consistent with EOP-by-default.
To reproduce the old legacy-mode measurements: `mops bench --gc copying`
(now auto-enables `--legacy-persistence`).

---------

Co-authored-by: Cursor <cursoragent@cursor.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.

2 participants