Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

73 changes: 73 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ SHELL := /bin/bash

# Default docs port; override with: make docs PORT=5180
PORT ?= 5173
BENCH_PORT ?= 8000

.PHONY: hooks docs docs-build docs-ci echo-total
hooks:
Expand Down Expand Up @@ -43,3 +44,75 @@ docs-ci:
echo-total:
@chmod +x scripts/gen-echo-total.sh
@./scripts/gen-echo-total.sh
# Benchmarks and reports
.PHONY: bench-report vendor-d3 bench-serve bench-open

vendor-d3:
@mkdir -p docs/benchmarks/vendor
@if [ ! -f docs/benchmarks/vendor/d3.v7.min.js ]; then \
echo "Downloading D3 v7 to docs/benchmarks/vendor..."; \
curl -fsSL https://unpkg.com/d3@7/dist/d3.min.js -o docs/benchmarks/vendor/d3.v7.min.js; \
echo "D3 saved to docs/benchmarks/vendor/d3.v7.min.js"; \
else \
echo "D3 already present (docs/benchmarks/vendor/d3.v7.min.js)"; \
fi

bench-serve:
@echo "Serving repo at http://localhost:$(BENCH_PORT) (Ctrl+C to stop)"
@python3 -m http.server $(BENCH_PORT)

bench-open:
@open "http://localhost:$(BENCH_PORT)/docs/benchmarks/"

bench-report: vendor-d3
@echo "Running benches (rmg-benches)..."
cargo bench -p rmg-benches
@echo "Starting local server on :$(BENCH_PORT) and opening dashboard..."
@mkdir -p target
@if [ -f target/bench_http.pid ] && ps -p $$(cat target/bench_http.pid) >/dev/null 2>&1; then \
echo "[bench] Stopping previous server (pid $$(cat target/bench_http.pid))"; \
kill $$(cat target/bench_http.pid) >/dev/null 2>&1 || true; \
rm -f target/bench_http.pid; \
fi
@/bin/sh -c 'nohup python3 -m http.server $(BENCH_PORT) >/dev/null 2>&1 & echo $$! > target/bench_http.pid'
@echo "[bench] Waiting for server to become ready..."
@for i in {1..80}; do \
if curl -sSf "http://localhost:$(BENCH_PORT)/" >/dev/null ; then \
echo "[bench] Server is up at http://localhost:$(BENCH_PORT)/" ; \
break ; \
fi ; \
sleep 0.25 ; \
done
@open "http://localhost:$(BENCH_PORT)/docs/benchmarks/"
Comment on lines +60 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Portability concern: open command is macOS-specific.

Lines 65 and 86 use the open command, which is macOS-specific. On Linux, the equivalent is xdg-open, and on Windows, it's start.

Consider a portable opener:

+# Detect OS and set browser opener
+UNAME := $(shell uname -s)
+ifeq ($(UNAME),Darwin)
+  OPEN := open
+else ifeq ($(UNAME),Linux)
+  OPEN := xdg-open
+else
+  OPEN := echo "Please open manually:"
+endif
+
 bench-open:
-	@open "http://localhost:$(BENCH_PORT)/docs/benchmarks/"
+	@$(OPEN) "http://localhost:$(BENCH_PORT)/docs/benchmarks/"

Or document the macOS requirement in the target's comment.

Regarding the server lifecycle: the PID management and polling logic look solid. The nohup redirection correctly discards output, and the for i in {1..80} loop with curl -sSf provides robust readiness checking.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In Makefile around lines 60 to 86, the targets bench-open and bench-report use
the macOS-only open command (lines ~65 and ~86), which breaks on Linux/Windows;
change both invocations to use a small portable opener that detects available
tools (check command -v open && open, elif command -v xdg-open && xdg-open, elif
on Windows use cmd /c start) or call a project-level script that performs this
detection, and/or document in the target comment that macOS is required if you
choose not to implement detection.


.PHONY: bench-status bench-stop

bench-status:
@if [ -f target/bench_http.pid ] && ps -p $$(cat target/bench_http.pid) >/dev/null 2>&1; then \
echo "[bench] Server running (pid $$(cat target/bench_http.pid)) at http://localhost:$(BENCH_PORT)"; \
else \
echo "[bench] Server not running"; \
fi

bench-stop:
@if [ -f target/bench_http.pid ]; then \
kill $$(cat target/bench_http.pid) >/dev/null 2>&1 || true; \
rm -f target/bench_http.pid; \
echo "[bench] Server stopped"; \
else \
echo "[bench] No PID file at target/bench_http.pid"; \
fi

.PHONY: bench-bake bench-open-inline

# Bake a standalone HTML with inline data that works over file://
bench-bake: vendor-d3
@echo "Running benches (rmg-benches)..."
cargo bench -p rmg-benches
@echo "Baking inline report..."
@python3 scripts/bench_bake.py --out docs/benchmarks/report-inline.html
@echo "Opening inline report..."
@open docs/benchmarks/report-inline.html

bench-open-inline:
@open docs/benchmarks/report-inline.html
6 changes: 4 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,10 @@ It’s the core of the Echo engine: runtime, assets, networking, and tools all o

## Developer: Running Benchmarks

- Command: `cargo bench -p rmg-benches`
- Purpose: Runs Criterion micro-benchmarks for the benches crate (`crates/rmg-benches`).
- Command (live dashboard): `make bench-report`
- Runs `cargo bench -p rmg-benches`, starts a local server, and opens the dashboard at `http://localhost:8000/docs/benchmarks/`.
- Command (offline static file): `make bench-bake`
- Runs benches and bakes `docs/benchmarks/report-inline.html` with results injected so it works over `file://` (no server required).
- Docs: see `crates/rmg-benches/benches/README.md` for details, tips, and report paths.

### Core Principles
Expand Down
4 changes: 2 additions & 2 deletions crates/rmg-benches/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ description = "Microbenchmarks for Echo (rmg-core): snapshot hashing and schedul
criterion = { version = "0.5", default-features = false, features = ["html_reports"] }
# Pin version alongside path to satisfy cargo-deny wildcard bans
rmg-core = { version = "0.1.0", path = "../rmg-core" }
# Minor-pin for semver compatibility; benches do not rely on a specific patch.
blake3 = "1.8"
# Exact pin and trimmed features to avoid rayon/parallelism in benches.
blake3 = { version = "=1.8.2", default-features = false, features = ["std"] }
Comment on lines +13 to +14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Exact version pin creates maintenance burden; document rationale or use tilde requirement.

The change from blake3 = "1.8" to blake3 = { version = "=1.8.2", ... } uses an exact pin (=1.8.2), which:

  1. Prevents security patches: If 1.8.3 fixes a vulnerability, it won't be pulled automatically
  2. Requires manual updates: Every blake3 release needs a deliberate Cargo.toml edit
  3. Conflicts with other deps: If another crate requires blake3 >= 1.8.3, resolution fails

The comment says "exact pin", but doesn't explain why exact rather than tilde (~1.8.2) or caret with minor (^1.8.2).

Recommendations by priority:

  1. If only avoiding rayon is the goal, use tilde for patch flexibility:

    blake3 = { version = "~1.8.2", default-features = false, features = ["std"] }

    This allows 1.8.x patches while staying on minor 1.8.

  2. If patch-level stability is critical (e.g., for benchmark reproducibility), document why:

    # Exact pin ensures bit-identical hashing across benchmark runs for reproducibility.
    # Update deliberately when validating new blake3 releases don't change bench characteristics.
    blake3 = { version = "=1.8.2", default-features = false, features = ["std"] }
  3. Verify the pin is current:

    What is the latest version of the blake3 Rust crate?
    

What's the specific rationale for the exact pin here?


[[bench]]
name = "motion_throughput"
Expand Down
11 changes: 9 additions & 2 deletions crates/rmg-benches/benches/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ cargo bench -p rmg-benches --bench scheduler_drain

Criterion HTML reports are written under `target/criterion/<group>/report/index.html`.

### Charts & Reports

- Live server + dashboard: `make bench-report` opens `http://localhost:8000/docs/benchmarks/`.
- Offline static report (no server): `make bench-bake` writes `docs/benchmarks/report-inline.html` with results injected.
- Open the file directly (Finder or `open docs/benchmarks/report-inline.html`).

## Interpreting Results

- Use the throughput value to sanity‑check the scale of work per iteration.
Expand All @@ -48,7 +54,9 @@ Criterion HTML reports are written under `target/criterion/<group>/report/index.
## Environment Notes

- Toolchain: `stable` Rust (see `rust-toolchain.toml`).
- Dependency policy: avoid wildcards; benches use a minor pin for `blake3`.
- Dependency policy: avoid wildcards; benches use an exact patch pin for `blake3`
with trimmed features to avoid incidental parallelism:
`blake3 = { version = "=1.8.2", default-features = false, features = ["std"] }`.
- Repro: keep your machine under minimal background load; prefer `--quiet` and
close other apps.

Expand All @@ -62,4 +70,3 @@ cargo flamegraph -p rmg-benches --bench snapshot_hash -- --sample-size 50
```

These tools are not required for CI and are optional for local analysis.

53 changes: 48 additions & 5 deletions crates/rmg-benches/benches/scheduler_drain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use rmg_core::{
make_node_id, make_type_id, ApplyResult, ConflictPolicy, Engine, Footprint, Hash, NodeId,
NodeRecord, PatternGraph, RewriteRule,
};
use std::time::Duration;

// Bench constants to avoid magic strings.
const BENCH_NOOP_RULE_NAME: &str = "bench/noop";
Expand Down Expand Up @@ -70,29 +71,71 @@ fn build_engine_with_entities(n: usize) -> (Engine, Vec<NodeId>) {

fn bench_scheduler_drain(c: &mut Criterion) {
let mut group = c.benchmark_group("scheduler_drain");
for &n in &[10usize, 100, 1_000] {
// Stabilize CI runs: explicit warmup/measurement and sample size.
group
.warm_up_time(Duration::from_secs(3))
.measurement_time(Duration::from_secs(10))
.sample_size(60);
for &n in &[10usize, 100, 1_000, 3_000, 10_000, 30_000] {
// Throughput: number of rule applications in this run (n entities).
group.throughput(Throughput::Elements(n as u64));

// Full apply+commit cycle (original benchmark)
group.bench_with_input(BenchmarkId::from_parameter(n), &n, |b, &n| {
b.iter_batched(
|| build_engine_with_entities(n),
|(mut engine, ids)| {
// Apply the no-op rule to all entities, then commit.
let tx = engine.begin();
for id in &ids {
let res = engine
.apply(tx, BENCH_NOOP_RULE_NAME, id)
.expect("Failed to apply noop bench rule");
let res = engine.apply(tx, BENCH_NOOP_RULE_NAME, id).unwrap();
// Avoid affecting timing; check only in debug builds.
debug_assert!(matches!(res, ApplyResult::Applied));
}
let snap = engine.commit(tx).expect("Failed to commit benchmark tx");
let snap = engine.commit(tx).unwrap();
// Ensure the commit work is not optimized away.
criterion::black_box(snap);
},
BatchSize::PerIteration,
)
});

// Enqueue phase only (apply without commit)
group.bench_function(BenchmarkId::new("enqueue", n), |b| {
b.iter_batched(
|| build_engine_with_entities(n),
|(mut engine, ids)| {
let tx = engine.begin();
for id in &ids {
let res = engine.apply(tx, BENCH_NOOP_RULE_NAME, id).unwrap();
debug_assert!(matches!(res, ApplyResult::Applied));
}
criterion::black_box(tx);
},
BatchSize::PerIteration,
)
});

// Drain phase only (commit with pre-enqueued rewrites)
group.bench_function(BenchmarkId::new("drain", n), |b| {
b.iter_batched(
|| {
let (mut engine, ids) = build_engine_with_entities(n);
let tx = engine.begin();
// Pre-enqueue all rewrites (not timed)
for id in &ids {
let _ = engine.apply(tx, BENCH_NOOP_RULE_NAME, id).unwrap();
}
(engine, tx)
},
|(mut engine, tx)| {
// Only measure the commit (drain + execute)
let snap = engine.commit(tx).unwrap();
criterion::black_box(snap);
},
BatchSize::PerIteration,
)
});
}
group.finish();
}
Expand Down
10 changes: 8 additions & 2 deletions crates/rmg-benches/benches/snapshot_hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use criterion::{criterion_group, criterion_main, BatchSize, BenchmarkId, Criteri
use rmg_core::{
make_edge_id, make_node_id, make_type_id, EdgeRecord, Engine, GraphStore, NodeRecord,
};
use std::time::Duration;

// String constants to avoid magic literals drifting silently.
const ROOT_ID_STR: &str = "root";
Expand Down Expand Up @@ -71,7 +72,12 @@ fn build_chain_engine(n: usize) -> Engine {

fn bench_snapshot_hash(c: &mut Criterion) {
let mut group = c.benchmark_group("snapshot_hash");
for &n in &[10usize, 100, 1_000] {
// Stabilize CI runs across environments.
group
.warm_up_time(Duration::from_secs(3))
.measurement_time(Duration::from_secs(10))
.sample_size(80);
for &n in &[10usize, 100, 1_000, 3_000, 10_000, 30_000] {
// Throughput: total nodes in reachable set (n entities + 1 root).
group.throughput(Throughput::Elements(n as u64 + 1));
group.bench_with_input(BenchmarkId::from_parameter(n), &n, |b, &n| {
Expand All @@ -82,7 +88,7 @@ fn bench_snapshot_hash(c: &mut Criterion) {
let snap = engine.snapshot();
criterion::black_box(snap.hash);
},
BatchSize::SmallInput,
BatchSize::PerIteration,
)
});
}
Expand Down
1 change: 1 addition & 0 deletions crates/rmg-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ hex = { version = "0.4", optional = true }
serde = { version = "1.0", features = ["derive"], optional = true }
serde_json = { version = "1.0", optional = true }
once_cell = "1.19"
rustc-hash = "2.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

Verify rustc-hash version is current and optimal for deterministic use.

The dependency specifies rustc-hash = "2.0" using semver caret syntax, allowing automatic minor/patch updates. Given the PR's emphasis on deterministic scheduling, confirm that:

  1. Version 2.0 is the latest stable release
  2. Default features provide the required determinism guarantees for FxHashMap/FxHashSet

🌐 Web query:

What is the latest stable version of the rustc-hash crate and what are its default features?

💡 Result:

Latest stable on crates.io: rustc-hash 2.1.1 (published 2025-02-05). Default features: a single default feature "std" (i.e., the "std" feature is enabled by default). [1][2]

Sources:
[1] docs.rs rustc-hash 2.1.1 (version + publish date).
[2] docs.rs rustc-hash feature flags (shows "default" -> "std").


Upgrade rustc-hash to latest stable (2.1.1) for optimal dependency currency.

The dependency rustc-hash = "2.0" specifies version 2.0, but the latest stable is 2.1.1 (published 2025-02-05). While the caret syntax allows automatic minor/patch updates to 2.1.1, explicitly pinning an outdated version is sloppy. Upgrade to rustc-hash = "2.1.1" to:

  • Eliminate ambiguity about which version was verified
  • Capture the latest bug fixes and performance improvements in 2.1.1
  • Confirm determinism: the "std" default feature is properly enabled and provides the required FxHashMap/FxHashSet guarantees
🤖 Prompt for AI Agents
In crates/rmg-core/Cargo.toml around line 22 the dependency is pinned as
rustc-hash = "2.0"; update this to rustc-hash = "2.1.1" to explicitly depend on
the latest stable release, ensuring you pick up bug fixes and performance
improvements; if your code relies on the standard feature set, keep the default
features (no change needed) or explicitly add features = ["std"] only if you
previously disabled defaults.


[dev-dependencies]
serde = { version = "1.0", features = ["derive"] }
Expand Down
4 changes: 2 additions & 2 deletions crates/rmg-core/src/engine_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,8 @@ impl Engine {
"missing compact rule id for a registered rule",
));
};
self.scheduler.pending.entry(tx).or_default().insert(
(scope_fp, rule.id),
self.scheduler.enqueue(
tx,
PendingRewrite {
rule_id: rule.id,
compact_rule,
Expand Down
4 changes: 4 additions & 0 deletions crates/rmg-core/src/footprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ impl IdSet {
pub fn insert_edge(&mut self, id: &EdgeId) {
self.0.insert(id.0);
}
/// Returns an iterator over the identifiers in the set.
pub fn iter(&self) -> impl Iterator<Item = &Hash> {
self.0.iter()
}
Comment on lines +40 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Nice: deterministic iteration exposure for IdSet.

Consider also implementing IntoIterator for &IdSet so for h in &id_set works idiomatically.

 #[derive(Debug, Clone, Default)]
-pub struct IdSet(BTreeSet<Hash>);
+pub struct IdSet(BTreeSet<Hash>);
+
+impl<'a> IntoIterator for &'a IdSet {
+    type Item = &'a Hash;
+    type IntoIter = std::collections::btree_set::Iter<'a, Hash>;
+    fn into_iter(self) -> Self::IntoIter { self.0.iter() }
+}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In crates/rmg-core/src/footprint.rs around lines 40–43, add an implementation of
IntoIterator for &IdSet so users can write `for h in &id_set`; implement
`impl<'a> IntoIterator for &'a IdSet` with Item = &Hash and IntoIter = the
iterator type returned by the internal set (use the concrete iter type, e.g. the
BTreeSet::Iter or equivalent) and have into_iter simply call the existing iter()
method, ensuring lifetimes and types match the file's imports.

/// Returns true if any element is shared with `other`.
pub fn intersects(&self, other: &Self) -> bool {
// Early‑exit by zipping ordered sets.
Expand Down
Loading