-
Notifications
You must be signed in to change notification settings - Fork 1
rmg-core/scheduler: fail-fast drain; u32 radix histogram; keep PendingTx private #121
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 9 commits
17ccf77
8b2fa28
607155d
debc4c5
ea1676e
7fc3420
9223f27
b29f168
61499dd
0f704a6
a15d2d7
0ba4a1a
c40f254
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
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. 🧹 Nitpick | 🔵 Trivial Exact version pin creates maintenance burden; document rationale or use tilde requirement. The change from
The comment says "exact pin", but doesn't explain why exact rather than tilde ( Recommendations by priority:
What's the specific rationale for the exact pin here? |
||
|
|
||
| [[bench]] | ||
| name = "motion_throughput" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| #![allow(missing_docs)] | ||
| //! Benchmark: reserve() scaling with footprint size and number of reserved rewrites | ||
| //! | ||
| //! Measures how reserve() performance scales with: | ||
| //! 1. Number of previously reserved rewrites (k) | ||
| //! 2. Size of footprint being reserved (m) | ||
| //! | ||
| //! The current GenSet-based implementation should scale as O(m), independent of k. | ||
| //! A naive Vec<Footprint> implementation would scale as O(k × m). | ||
| use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; | ||
| use rmg_core::Hash; | ||
| use std::time::Duration; | ||
|
|
||
| // Import the scheduler - it's crate-private so we can't access it directly | ||
| // Instead we'll use it through the Engine API | ||
| // Actually, we need direct access for this micro-benchmark, so we'll create | ||
| // a test module inside rmg-core and expose it via a feature flag or just | ||
| // write an integration test instead. | ||
|
|
||
| // For now, let's write a simpler benchmark that measures reserve through the Engine API | ||
|
|
||
| fn make_hash(val: u8) -> Hash { | ||
| let mut h = [0u8; 32]; | ||
| h[0] = val; | ||
| h | ||
| } | ||
|
Comment on lines
+23
to
+27
Contributor
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. 🛠️ Refactor suggestion | 🟠 Major Eliminate code duplication: reuse existing hash helper. The Solution: Extract the hash helper to a shared test utility module, or expose it through a If the use rmg_core::test_utils::make_test_hash;
// Remove local make_hash, use shared versionAlternatively, define once in a // benches/common/mod.rs
pub fn make_hash(val: u8) -> rmg_core::Hash {
let mut h = [0u8; 32];
h[0] = val;
h
}Then import: 🤖 Prompt for AI Agents |
||
|
|
||
| // Note: This benchmark requires access to DeterministicScheduler which is crate-private. | ||
| // Moving this to rmg-core/src/scheduler.rs tests module or using a pub(crate) test harness. | ||
|
|
||
| fn bench_reserve_scaling(_c: &mut Criterion) { | ||
| // This is a placeholder - the actual benchmark needs to be in rmg-core | ||
| // where we can access the scheduler directly. | ||
|
|
||
| // TODO: Implement this properly by either: | ||
| // 1. Adding a test-only public API to DeterministicScheduler | ||
| // 2. Moving this benchmark into rmg-core as a test module | ||
| // 3. Using Engine API indirectly (less precise) | ||
|
|
||
| let _ = ( | ||
| BenchmarkId::new("placeholder", "reserve_scaling"), | ||
| Throughput::Elements(1), | ||
| make_hash(0), | ||
| ); | ||
| } | ||
|
Comment on lines
+29
to
+46
Contributor
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. Incomplete benchmark provides no value and pollutes the bench suite. This placeholder benchmark:
While scaffolding has value during development, merged code should be functional. The extensive comments indicate this work is blocked on API design decisions (exposing Options:
#[ignore = "Waiting for scheduler test API - see issue #122"]
fn bench_reserve_scaling(_c: &mut Criterion) {
// ...
}
Which approach aligns with your team's practices? If you'd like, I can draft a minimal implementation using the Engine API approach, or generate an issue template for tracking this work. 🤖 Prompt for AI Agents |
||
|
|
||
| criterion_group! { | ||
| name = benches; | ||
| config = Criterion::default() | ||
| .warm_up_time(Duration::from_secs(1)) | ||
| .measurement_time(Duration::from_secs(5)) | ||
| .sample_size(50); | ||
| targets = bench_reserve_scaling | ||
| } | ||
| criterion_main!(benches); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
Contributor
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. 🧹 Nitpick | 🔵 Trivial 🧩 Analysis chainVerify rustc-hash version is current and optimal for deterministic use. The dependency specifies
🌐 Web query: 💡 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: Upgrade rustc-hash to latest stable (2.1.1) for optimal dependency currency. The dependency
🤖 Prompt for AI Agents |
||
|
|
||
| [dev-dependencies] | ||
| serde = { version = "1.0", features = ["derive"] } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
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. 🧹 Nitpick | 🔵 Trivial Nice: deterministic iteration exposure for IdSet. Consider also implementing IntoIterator for &IdSet so #[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() }
+}
🤖 Prompt for AI Agents |
||
| /// Returns true if any element is shared with `other`. | ||
| pub fn intersects(&self, other: &Self) -> bool { | ||
| // Early‑exit by zipping ordered sets. | ||
|
|
||
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.
Portability concern:
opencommand is macOS-specific.Lines 65 and 86 use the
opencommand, which is macOS-specific. On Linux, the equivalent isxdg-open, and on Windows, it'sstart.Consider a portable opener:
Or document the macOS requirement in the target's comment.
Regarding the server lifecycle: the PID management and polling logic look solid. The
nohupredirection correctly discards output, and thefor i in {1..80}loop withcurl -sSfprovides robust readiness checking.🤖 Prompt for AI Agents