Complete 9.6.1 Codec Performance Benchmarks, Tests, and Docs#476
Merged
Complete 9.6.1 Codec Performance Benchmarks, Tests, and Docs#476
Conversation
Contributor
Reviewer's GuideIntroduces a Criterion-based benchmark harness, helper utilities, and BDD/unit tests for 9.6.1 codec performance (throughput, latency, fragmentation overhead, allocations), wires them into Cargo/Makefile and documentation/roadmap, and fixes some existing test result-handling issues. Sequence diagram for codec benchmark execution via make bench-codecsequenceDiagram
actor Developer
participant Makefile
participant Cargo
participant BenchCodecPerformance as codec_performance
participant BenchCodecAlloc as codec_performance_alloc
participant CriterionLib as Criterion
participant WireframeCrate as wireframe_codec
Developer->>Makefile: make bench-codec
Makefile->>Cargo: cargo bench --bench codec_performance --bench codec_performance_alloc --features test-support
Cargo->>BenchCodecPerformance: run bench binary
BenchCodecPerformance->>CriterionLib: Criterion::default().configure_from_args()
BenchCodecPerformance->>CriterionLib: benchmark_encode
CriterionLib->>WireframeCrate: measure_encode (LengthDelimitedFrameCodec, HotlineFrameCodec)
CriterionLib-->>BenchCodecPerformance: encode measurements
BenchCodecPerformance->>CriterionLib: benchmark_decode
CriterionLib->>WireframeCrate: measure_decode (LengthDelimitedFrameCodec, HotlineFrameCodec)
CriterionLib-->>BenchCodecPerformance: decode measurements
BenchCodecPerformance->>CriterionLib: benchmark_fragmentation_overhead
CriterionLib->>WireframeCrate: measure_fragmented_wrap / measure_unfragmented_wrap
CriterionLib-->>BenchCodecPerformance: fragmentation metrics
BenchCodecPerformance->>CriterionLib: final_summary
Cargo->>BenchCodecAlloc: run bench binary
BenchCodecAlloc->>CriterionLib: Criterion::default().configure_from_args()
BenchCodecAlloc->>BenchCodecAlloc: count_allocations(measure_encode)
BenchCodecAlloc->>WireframeCrate: measure_encode with counting allocator
BenchCodecAlloc->>BenchCodecAlloc: count_allocations(measure_decode)
BenchCodecAlloc->>WireframeCrate: measure_decode with counting allocator
BenchCodecAlloc->>CriterionLib: benchmark_allocations
CriterionLib-->>BenchCodecAlloc: allocation benchmarks
BenchCodecAlloc->>CriterionLib: final_summary
CriterionLib-->>Cargo: benchmark results
Cargo-->>Developer: console output with throughput, latency, fragmentation, allocations
Class diagram for codec benchmark helper types and allocation benchclassDiagram
class CodecUnderTest {
<<enum>>
+LengthDelimited
+Hotline
+label() &str
}
class PayloadClass {
<<enum>>
+Small
+Large
+label() &str
+len() usize
}
class BenchmarkWorkload {
+codec CodecUnderTest
+payload_class PayloadClass
+label() String
}
class MeasurementAllocBench {
<<struct>>
+payload_bytes u64
+elapsed Duration
}
class AllocationBaseline {
<<struct>>
+wrap_allocations usize
+decode_allocations usize
}
class FragmentationBaseline {
<<struct>>
+unfragmented MeasurementSupport
+fragmented MeasurementSupport
+nanos_ratio() f64
}
class MeasurementSupport {
<<struct>>
+payload_bytes u64
+elapsed Duration
+nanos_per_op() u64
}
class CountingAllocator {
<<struct>>
+alloc(layout Layout) u8*
+dealloc(ptr u8*, layout Layout) void
+alloc_zeroed(layout Layout) u8*
+realloc(ptr u8*, layout Layout, new_size usize) u8*
}
class LengthDelimitedOps {
<<struct>>
+codec_name() &str
+create_codec() LengthDelimitedFrameCodec
+extract_payload(frame Frame) u8[]
}
class HotlineOps {
<<struct>>
+codec_name() &str
+create_codec() HotlineFrameCodec
+extract_payload(frame Frame) u8[]
}
class CodecBenchmarkSupportModule {
<<module>>
+SMALL_PAYLOAD_BYTES usize
+LARGE_PAYLOAD_BYTES usize
+FRAGMENT_PAYLOAD_CAP_BYTES usize
+VALIDATION_ITERATIONS u64
+benchmark_workloads() BenchmarkWorkload[]
+payload_for_class(class PayloadClass) Bytes
+measure_encode(workload BenchmarkWorkload, iterations u64) MeasurementSupport~Result~
+measure_decode(workload BenchmarkWorkload, iterations u64) MeasurementSupport~Result~
+measure_unfragmented_wrap(class PayloadClass, iterations u64) MeasurementSupport
+measure_fragmented_wrap(class PayloadClass, iterations u64, cap_bytes usize) MeasurementSupport~Result~
+measure_fragmentation_overhead(class PayloadClass, iterations u64, cap_bytes usize) FragmentationBaseline~Result~
+allocation_label(workload BenchmarkWorkload, baseline AllocationBaseline) String
}
class CodecPerformanceBenchBinary {
<<binary>>
+benchmark_encode(c Criterion)
+benchmark_decode(c Criterion)
+benchmark_fragmentation_overhead(c Criterion)
+main()
}
class CodecPerformanceAllocBenchBinary {
<<binary>>
+benchmark_allocations(c Criterion)
+benchmark_workloads() BenchmarkWorkload[]
+measure_encode(workload BenchmarkWorkload, iterations u64) MeasurementAllocBench~Result~
+measure_decode(workload BenchmarkWorkload, iterations u64) MeasurementAllocBench~Result~
+prepare_decode(payload Bytes) (Bytes, Decoder)~Result~
+run_decode_iterations(decoder Decoder, encoded Bytes, iterations u64) Result
+count_allocations(operation FnOnce) usize
+allocation_label(workload BenchmarkWorkload, wrap_allocations usize, decode_allocations usize) String
+main()
}
CodecBenchmarkSupportModule --> CodecUnderTest
CodecBenchmarkSupportModule --> PayloadClass
CodecBenchmarkSupportModule --> BenchmarkWorkload
CodecBenchmarkSupportModule --> MeasurementSupport
CodecBenchmarkSupportModule --> FragmentationBaseline
CodecBenchmarkSupportModule --> AllocationBaseline
CodecPerformanceBenchBinary --> CodecBenchmarkSupportModule
CodecPerformanceAllocBenchBinary --> CodecUnderTest
CodecPerformanceAllocBenchBinary --> PayloadClass
CodecPerformanceAllocBenchBinary --> BenchmarkWorkload
CodecPerformanceAllocBenchBinary --> MeasurementAllocBench
CodecPerformanceAllocBenchBinary --> LengthDelimitedOps
CodecPerformanceAllocBenchBinary --> HotlineOps
CodecPerformanceAllocBenchBinary --> CountingAllocator
LengthDelimitedOps ..|> CodecDecodeOps
HotlineOps ..|> CodecDecodeOps
class CodecDecodeOps {
<<trait>>
+Codec FrameCodec
+Decoder Decoder
+codec_name() &str
+create_codec() Codec
+extract_payload(frame Item) u8[]
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
WalkthroughAdd Criterion-based codec benchmarks and an allocation-counting harness; introduce shared benchmark/test utilities, BDD fixtures and scenarios, a Makefile bench target, documentation and roadmap updates; change test push helpers to propagate errors instead of ignoring them. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/common/codec_benchmark_support.rs Comment on file pub fn measure_decode(workload: BenchmarkWorkload, iterations: u64) -> Result<Measurement, String> {
if iterations == 0 {
return Ok(Measurement {
operations: 0,
payload_bytes: 0,
elapsed: Duration::ZERO,
});
}
let payload = payload_for_class(workload.payload_class);
let payload_len = payload.len() as u64;
match workload.codec {
CodecUnderTest::LengthDelimited => {
let codec = LengthDelimitedFrameCodec::new(LARGE_PAYLOAD_BYTES + 4096);
let mut seed_encoder = codec.encoder();
let mut encoded = BytesMut::new();
seed_encoder
.encode(codec.wrap_payload(payload), &mut encoded)
.map_err(|err| format!("length-delimited seed encode failed: {err}"))?;
let encoded = encoded.freeze();
let mut decoder = codec.decoder();
let started = Instant::now();
for _ in 0..iterations {
let mut wire = BytesMut::from(encoded.as_ref());
let frame = decoder
.decode(&mut wire)
.map_err(|err| format!("length-delimited decode failed: {err}"))?
.ok_or("length-delimited decode produced no frame")?;
if LengthDelimitedFrameCodec::frame_payload(&frame).is_empty() {
return Err("length-delimited decode produced empty payload".to_string());
}
}
Ok(Measurement {
operations: iterations,
payload_bytes: iterations * payload_len,
elapsed: started.elapsed(),
})
}
CodecUnderTest::Hotline => {
let codec = HotlineFrameCodec::new(LARGE_PAYLOAD_BYTES + 4096);
let mut seed_encoder = codec.encoder();
let mut encoded = BytesMut::new();
seed_encoder
.encode(codec.wrap_payload(payload), &mut encoded)
.map_err(|err| format!("hotline seed encode failed: {err}"))?;
let encoded = encoded.freeze();
let mut decoder = codec.decoder();
let started = Instant::now();
for _ in 0..iterations {
let mut wire = BytesMut::from(encoded.as_ref());
let frame = decoder
.decode(&mut wire)
.map_err(|err| format!("hotline decode failed: {err}"))?
.ok_or("hotline decode produced no frame")?;
if HotlineFrameCodec::frame_payload(&frame).is_empty() {
return Err("hotline decode produced empty payload".to_string());
}
}
Ok(Measurement {
operations: iterations,
payload_bytes: iterations * payload_len,
elapsed: started.elapsed(),
})
}
}
}❌ New issue: Bumpy Road Ahead |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: benches/codec_performance_alloc.rs Comment on file fn measure_decode(workload: BenchmarkWorkload, iterations: u64) -> Result<Measurement, String> {
let payload = payload_for_class(workload.payload_class);
let payload_len = payload.len() as u64;
match workload.codec {
CodecUnderTest::LengthDelimited => {
let codec = LengthDelimitedFrameCodec::new(LARGE_PAYLOAD_BYTES + 4096);
let mut seed_encoder = codec.encoder();
let mut encoded = BytesMut::new();
seed_encoder
.encode(codec.wrap_payload(payload), &mut encoded)
.map_err(|err| format!("length-delimited seed encode failed: {err}"))?;
let encoded = encoded.freeze();
let mut decoder = codec.decoder();
let started = Instant::now();
for _ in 0..iterations {
let mut wire = BytesMut::from(encoded.as_ref());
let frame = decoder
.decode(&mut wire)
.map_err(|err| format!("length-delimited decode failed: {err}"))?
.ok_or("length-delimited decode produced no frame")?;
if LengthDelimitedFrameCodec::frame_payload(&frame).is_empty() {
return Err("length-delimited decode produced empty payload".to_string());
}
}
Ok(Measurement {
payload_bytes: iterations.saturating_mul(payload_len),
elapsed: started.elapsed(),
})
}
CodecUnderTest::Hotline => {
let codec = HotlineFrameCodec::new(LARGE_PAYLOAD_BYTES + 4096);
let mut seed_encoder = codec.encoder();
let mut encoded = BytesMut::new();
seed_encoder
.encode(codec.wrap_payload(payload), &mut encoded)
.map_err(|err| format!("hotline seed encode failed: {err}"))?;
let encoded = encoded.freeze();
let mut decoder = codec.decoder();
let started = Instant::now();
for _ in 0..iterations {
let mut wire = BytesMut::from(encoded.as_ref());
let frame = decoder
.decode(&mut wire)
.map_err(|err| format!("hotline decode failed: {err}"))?
.ok_or("hotline decode produced no frame")?;
if HotlineFrameCodec::frame_payload(&frame).is_empty() {
return Err("hotline decode produced empty payload".to_string());
}
}
Ok(Measurement {
payload_bytes: iterations.saturating_mul(payload_len),
elapsed: started.elapsed(),
})
}
}
}❌ New issue: Bumpy Road Ahead |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Add a comprehensive living ExecPlan for roadmap item 9.6.1 that defines benchmark contracts, progress stages, constraints, risks, decision logs, tolerances, and planned work for codec performance benchmarking. This new document provides structure for implementing and validating throughput, latency, fragmentation overhead, and allocation baselines across codecs, along with testing guidance and documentation updates. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…and allocation benchmarks - Introduced Criterion benchmark suites for codec encoding/decoding throughput and latency. - Benchmarked default LengthDelimitedFrameCodec and custom HotlineFrameCodec with small and large payloads. - Measured fragmentation overhead comparing fragmented vs unfragmented wrapping. - Added allocation baseline benchmarks with a counting global allocator reporting wrap and decode allocations. - Integrated benchmarks into Makefile with a new bench-codec target. - Added extensive unit tests and behavior-driven tests for benchmark validation. - Updated ADR, roadmap, and execution plan docs to mark item 9.6.1 as complete. - Fixed related lint and test issues, including properly handling push_expect! results in interleaved push queue tests. No public runtime API changes introduced; benchmarks remain internal tooling. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
devboxerhub
Bot
force-pushed
the
benchmark-codec-performance-qgw4sp
branch
from
44081af to
726f129
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
…ogic Refactored the codec benchmark code by extracting the common preparation and iteration loops for length-delimited and hotline decode benchmarks into separate helper functions. This reduces duplication and improves readability in benchmark measurement functions across benches and tests/common modules. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: benches/codec_performance_alloc.rs Comment on file fn prepare_length_delimited_decode(
payload: Bytes,
) -> Result<(Bytes, LengthDelimitedFrameDecoder), String> {
let codec = LengthDelimitedFrameCodec::new(LARGE_PAYLOAD_BYTES + 4096);
let mut seed_encoder = codec.encoder();
let mut encoded = BytesMut::new();
seed_encoder
.encode(codec.wrap_payload(payload), &mut encoded)
.map_err(|err| format!("length-delimited seed encode failed: {err}"))?;
Ok((encoded.freeze(), codec.decoder()))
}❌ New issue: Code Duplication |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/common/codec_benchmark_support.rs Comment on file fn measure_decode_length_delimited(
payload: Bytes,
payload_len: u64,
iterations: u64,
) -> Result<Measurement, String> {
let codec = LengthDelimitedFrameCodec::new(LARGE_PAYLOAD_BYTES + 4096);
let mut seed_encoder = codec.encoder();
let mut encoded = BytesMut::new();
seed_encoder
.encode(codec.wrap_payload(payload), &mut encoded)
.map_err(|err| format!("length-delimited seed encode failed: {err}"))?;
let encoded = encoded.freeze();
let mut decoder = codec.decoder();
let started = Instant::now();
for _ in 0..iterations {
let mut wire = BytesMut::from(encoded.as_ref());
let frame = decoder
.decode(&mut wire)
.map_err(|err| format!("length-delimited decode failed: {err}"))?
.ok_or("length-delimited decode produced no frame")?;
if LengthDelimitedFrameCodec::frame_payload(&frame).is_empty() {
return Err("length-delimited decode produced empty payload".to_string());
}
}
Ok(Measurement {
operations: iterations,
payload_bytes: iterations * payload_len,
elapsed: started.elapsed(),
})
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…aits and macros - Introduced a CodecDecodeOps trait to generalize codec decoding operations. - Replaced codec-specific decode preparation and iteration functions with generic ones using CodecDecodeOps. - Added a macro to generate decode measurement functions to reduce code duplication. - Updated benchmarks to use the new generic abstractions for LengthDelimited and Hotline codecs. This improves code maintainability by reducing duplication and enhancing extensibility for codec benchmarks. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The allocation benchmarks in
benches/codec_performance_alloc.rsreintroduce their ownCodecUnderTest/PayloadClass/workload definitions and encode/decode helpers instead of reusingtests/common/codec_benchmark_support.rs; consider factoring these into a single shared module to avoid drift and reduce duplication. - Constants like
SMALL_PAYLOAD_BYTES,LARGE_PAYLOAD_BYTES, andVALIDATION_ITERATIONSare now defined both incodec_benchmark_supportandcodec_performance_alloc; centralising them in the shared benchmark support module will make it easier to keep benchmark parameters consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The allocation benchmarks in `benches/codec_performance_alloc.rs` reintroduce their own `CodecUnderTest`/`PayloadClass`/workload definitions and encode/decode helpers instead of reusing `tests/common/codec_benchmark_support.rs`; consider factoring these into a single shared module to avoid drift and reduce duplication.
- Constants like `SMALL_PAYLOAD_BYTES`, `LARGE_PAYLOAD_BYTES`, and `VALIDATION_ITERATIONS` are now defined both in `codec_benchmark_support` and `codec_performance_alloc`; centralising them in the shared benchmark support module will make it easier to keep benchmark parameters consistent.
## Individual Comments
### Comment 1
<location path="benches/codec_performance_alloc.rs" line_range="85-94" />
<code_context>
+static ALLOCATION_COUNT: AtomicUsize = AtomicUsize::new(0);
</code_context>
<issue_to_address>
**issue (testing):** Allocation counting may be polluted by unrelated allocations and relaxed ordering, making baselines noisy or misleading.
Using a global `ALLOCATION_COUNT` with `Ordering::Relaxed` in Criterion benchmarks risks counting unrelated background allocations (e.g., stats collection, Vec growth, logging) between the `before` and `after` reads. That noise, plus the lack of ordering guarantees, means increments may be observed outside the intended measurement window.
To get cleaner codec-only counts, you could (a) narrow what’s executed inside `count_allocations`, (b) use stronger atomics (`SeqCst` or at least Acquire/Release), or (c) switch to a scoped allocator/counter rather than a single global shared across the whole process.
</issue_to_address>
### Comment 2
<location path="benches/codec_performance_alloc.rs" line_range="254-257" />
<code_context>
+ }
+}
+
+fn prepare_decode<Ops: CodecDecodeOps>(payload: Bytes) -> Result<(Bytes, Ops::Decoder), String>
+where
+ <Ops::Codec as FrameCodec>::Frame: Clone,
+{
+ let codec = Ops::create_codec();
</code_context>
<issue_to_address>
**suggestion:** The extra `Frame: Clone` bound in `prepare_decode` appears unnecessary.
In `prepare_decode`, the `where <Ops::Codec as FrameCodec>::Frame: Clone` bound isn’t used—the code only encodes into `BytesMut` and never clones the frame. If nothing else in the call chain depends on this, consider removing the bound to relax the generic constraints and support non-`Clone` frame types.
```suggestion
fn prepare_decode<Ops: CodecDecodeOps>(payload: Bytes) -> Result<(Bytes, Ops::Decoder), String> {
```
</issue_to_address>
### Comment 3
<location path="benches/codec_performance.rs" line_range="94" />
<code_context>
+ Ok(result) => result,
+ Err(err) => panic!("fragmentation baseline setup failed: {err}"),
+ };
+ let ratio = baseline.nanos_ratio().unwrap_or(0.0);
+ black_box(baseline.unfragmented.nanos_per_op());
+ black_box(baseline.fragmented.nanos_per_op());
+
+ group.bench_function(
+ BenchmarkId::new("unfragmented", payload_class.label()),
+ |b| {
+ b.iter_custom(|iters| {
+ let measurement = measure_unfragmented_wrap(payload_class, iters);
+ black_box(measurement.payload_bytes);
+ measurement.elapsed
+ });
+ },
+ );
+
+ group.bench_function(
+ BenchmarkId::new(
+ "fragmented",
+ format!("{}_ratio_{ratio:.3}", payload_class.label()),
+ ),
+ |b| {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Silently treating a missing fragmentation ratio as 0.0 may hide unexpected baseline issues.
In `benchmark_fragmentation_overhead`, `baseline.nanos_ratio().unwrap_or(0.0)` folds all failure/degenerate cases into `0.0`, which then goes into the benchmark ID. If `nanos_ratio()` returns `None` (e.g., empty measurements, division guard), the benchmark still runs but the label falsely shows `0.0` overhead.
Instead, consider either reflecting `None` in the label (e.g., `ratio_na`) or treating it as an error and panicking with context, matching the other setup code. This will make unexpected fragmentation conditions visible rather than silently masked.
```suggestion
let ratio = baseline.nanos_ratio().unwrap_or_else(|| {
panic!(
"fragmentation baseline has no valid nanos_ratio for payload_class={}",
payload_class.label()
)
});
```
</issue_to_address>
### Comment 4
<location path="benches/codec_performance_alloc.rs" line_range="149" />
<code_context>
+ Bytes::from(payload)
+}
+
+trait CodecDecodeOps {
+ type Codec: FrameCodec<Decoder = Self::Decoder>;
+ type Decoder: Decoder;
</code_context>
<issue_to_address>
**issue (complexity):** Consider unifying the codec-specific encode/decode logic behind a single trait, centralizing shared configuration, and simplifying error handling to reduce duplication and make the benchmarks easier to follow.
You can reduce complexity and duplication without changing behavior by unifying the encode/decode abstractions and centralizing config.
### 1. Unify encode/decode paths via a single `CodecOps` trait
Right now `CodecDecodeOps` is only used for decode, while encode does a manual `match` over `CodecUnderTest`. You can extend the ops trait so both encode and decode use the same abstraction, and then use it in both `measure_encode` and `measure_decode`.
```rust
trait CodecOps {
type Codec: FrameCodec<Decoder = Self::Decoder>;
type Decoder: Decoder;
fn codec_name() -> &'static str;
fn create_codec() -> Self::Codec;
fn extract_payload(frame: &<Self::Decoder as Decoder>::Item) -> &[u8];
}
struct LengthDelimitedOps;
struct HotlineOps;
impl CodecOps for LengthDelimitedOps {
type Codec = LengthDelimitedFrameCodec;
type Decoder = LengthDelimitedFrameDecoder;
fn codec_name() -> &'static str { "length-delimited" }
fn create_codec() -> Self::Codec {
LengthDelimitedFrameCodec::new(MAX_FRAME_BYTES)
}
fn extract_payload(frame: &<Self::Decoder as Decoder>::Item) -> &[u8] {
LengthDelimitedFrameCodec::frame_payload(frame)
}
}
// same pattern for HotlineOps
```
Then add a small helper to map `CodecUnderTest` to the ops type:
```rust
fn with_codec_ops<T>(
codec: CodecUnderTest,
f: impl FnOnce::<LengthDelimitedOps>() -> T,
g: impl FnOnce::<HotlineOps>() -> T,
) -> T {
match codec {
CodecUnderTest::LengthDelimited => f(),
CodecUnderTest::Hotline => g(),
}
}
```
Or more directly, use a plain `match` but only to select the type:
```rust
fn measure_encode(workload: BenchmarkWorkload, iterations: u64) -> Result<Measurement, String> {
match workload.codec {
CodecUnderTest::LengthDelimited => measure_encode_with::<LengthDelimitedOps>(workload, iterations),
CodecUnderTest::Hotline => measure_encode_with::<HotlineOps>(workload, iterations),
}
}
fn measure_encode_with<Ops: CodecOps>(
workload: BenchmarkWorkload,
iterations: u64,
) -> Result<Measurement, String>
where
<Ops::Codec as FrameCodec>::Frame: Clone,
{
let payload = payload_for_class(workload.payload_class);
let payload_len = payload.len() as u64;
let codec = Ops::create_codec();
let mut encoder = codec.encoder();
let mut wire = BytesMut::new();
let started = Instant::now();
for _ in 0..iterations {
wire.clear();
encoder
.encode(codec.wrap_payload(payload.clone()), &mut wire)
.map_err(|err| format!("{} encode failed: {err}", Ops::codec_name()))?;
}
Ok(Measurement {
payload_bytes: iterations.saturating_mul(payload_len),
elapsed: started.elapsed(),
})
}
```
You can likewise make `measure_decode` delegate to a generic `measure_decode_with::<Ops>` using the same `CodecOps` trait, so the encode and decode paths share the same mental model.
### 2. Centralize the max frame size
The expression `LARGE_PAYLOAD_BYTES + 4096` appears in multiple places. Pull it into a single constant and use that everywhere:
```rust
const SMALL_PAYLOAD_BYTES: usize = 32;
const LARGE_PAYLOAD_BYTES: usize = 64 * 1024;
const MAX_FRAME_BYTES: usize = LARGE_PAYLOAD_BYTES + 4096;
```
Then replace each:
```rust
LengthDelimitedFrameCodec::new(LARGE_PAYLOAD_BYTES + 4096);
HotlineFrameCodec::new(LARGE_PAYLOAD_BYTES + 4096);
```
with:
```rust
LengthDelimitedFrameCodec::new(MAX_FRAME_BYTES);
HotlineFrameCodec::new(MAX_FRAME_BYTES);
```
This keeps the benchmark configuration in one place and avoids drift between encode/decode or between codecs.
### 3. Simplify error handling in benchmark-only measurement functions
Since this is benchmark code and failures already panic, you can inline the panics into the measurement helpers, which removes `Result` plumbing and closure wrapping in `count_allocations`.
For example:
```rust
fn measure_encode(workload: BenchmarkWorkload, iterations: u64) -> Measurement {
// same body, but use `expect` / `unwrap_or_else`:
encoder
.encode(codec.wrap_payload(payload.clone()), &mut wire)
.unwrap_or_else(|err| panic!("{} encode failed: {err}", Ops::codec_name()));
// ...
}
```
Then `count_allocations` can take a simpler closure:
```rust
fn count_allocations<F>(operation: F) -> usize
where
F: FnOnce(),
{
let before = ALLOCATION_COUNT.load(Ordering::Relaxed);
operation();
let after = ALLOCATION_COUNT.load(Ordering::Relaxed);
after.saturating_sub(before)
}
```
And the call sites become:
```rust
let wrap_allocations = count_allocations(|| {
for _ in 0..VALIDATION_ITERATIONS {
let _ = measure_encode(workload, 1);
}
});
let decode_allocations = count_allocations(|| {
for _ in 0..VALIDATION_ITERATIONS {
let _ = measure_decode(workload, 1);
}
});
```
And inside `bench_function`:
```rust
b.iter_custom(|iters| {
let encode = measure_encode(workload, iters);
let decode = measure_decode(workload, iters);
black_box(encode.payload_bytes.saturating_add(decode.payload_bytes));
encode.elapsed + decode.elapsed
});
```
This keeps all functionality (still panics on failure) but trims branching and reduces the visual noise around the benchmark core.
</issue_to_address>
### Comment 5
<location path="docs/execplans/9-6-1-codec-performance-benchmarks.md" line_range="3" />
<code_context>
+# 9.6.1 Codec performance benchmarks for throughput, latency, and allocations
+
+This ExecPlan is a living document. The sections `Constraints`, `Tolerances`,
+`Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and
+`Outcomes & Retrospective` must be kept up to date as work proceeds.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** ExecPlan is introduced as an acronym without defining it on first use, which violates the requirement to define uncommon acronyms.
Consider expanding this to something like "This execution plan (ExecPlan) is a living document" so the acronym is defined on first use.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dc5b22f80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/codec_performance_alloc.rs`:
- Around line 26-75: The enums and structs CodecUnderTest, PayloadClass,
BenchmarkWorkload and the helper functions benchmark_workloads() and
payload_for_class() are duplicated; extract these domain types and helpers into
a shared module (e.g., a new mod or crate feature) and replace the local
definitions in benches/codec_performance_alloc.rs and
tests/common/codec_benchmark_support.rs with imports from that module, keeping
Measurement local if it must differ; update usages of CodecUnderTest::label,
PayloadClass::label/len, BenchmarkWorkload::label and any calls to
benchmark_workloads() and payload_for_class() to reference the shared items and
remove the duplicated definitions from this file.
- Around line 291-301: Update the count_allocations function's documentation to
state that it measures a global allocation delta via ALLOCATION_COUNT (loaded
with Ordering::Relaxed) and therefore includes allocations from Criterion,
closures, iterators, and other test harness activity, not only the codec
operation passed as operation: FnOnce() -> Result<(), String>; mark the counts
as noisy/approximate and intended as a relative baseline rather than precise
codec-only allocation numbers so future maintainers won't misinterpret the
results.
- Around line 188-226: measure_encode duplicates the encode loop for
LengthDelimited and Hotline; introduce a CodecEncodeOps trait (or extend
CodecDecodeOps) exposing encoder(), wrap_payload(), and a constructor-compatible
new(size) so you can factor the loop into a generic run_encode_iterations
function; implement CodecEncodeOps for LengthDelimitedFrameCodec and
HotlineFrameCodec, update measure_encode to call
run_encode_iterations(codec_new, payload, iterations) and ensure
run_encode_iterations maps codec-specific errors into the existing
"length-delimited encode failed" / "hotline encode failed" messages or accepts
an error-label parameter to preserve diagnostic text.
In `@benches/codec_performance.rs`:
- Around line 37-43: Remove the stray benchmarking call: delete the
black_box(allocation_label(...)) invocation (and remove the now-unused
AllocationBaseline import) from the encode benchmark; if you actually need the
workload, avoid panicking indexing by replacing benchmark_workloads()[0] with a
safe accessor such as benchmark_workloads().first() and handle the Option,
otherwise just remove the entire allocation_label call and any related unused
symbols (allocation_label, AllocationBaseline).
In `@docs/adr-004-pluggable-protocol-codecs.md`:
- Around line 328-332: Rewrite the lead-in sentence so it does not use the
preposition-plus-colon construction "across:" before the list; for example
change the first bullet to read something like "`benches/codec_performance.rs`
measures encode and decode throughput/latency for `LengthDelimitedFrameCodec`
and `HotlineFrameCodec`, and for small (32-byte) and large (64 KiB) payload
classes." Keep the second bullet about fragmentation overhead unchanged except
adjust wording to match en-GB spelling/grammar if needed; update text that
references `benches/codec_performance.rs` and
`LengthDelimitedFrameCodec`/`HotlineFrameCodec` to ensure the new lead-in flows
naturally into the list.
In `@docs/execplans/9-6-1-codec-performance-benchmarks.md`:
- Around line 117-171: Change the three top-level headings to sentence case and
expand ampersands: replace "Surprises & Discoveries" with "Surprises and
discoveries", replace "Decision Log" with "Decision log", and replace "Outcomes
& Retrospective" with "Outcomes and retrospective" so headings follow the
repository's "sentence case" and no-ampersand style; update those exact heading
strings in the markdown file.
In `@tests/codec_performance_benchmark_helpers.rs`:
- Line 3: Add a short comment above the crate-level attribute explaining why the
tests must not run under loom (for example: they use real-thread timing,
non-deterministic behavior, external resources, or unsafe code incompatible with
loom). Specifically, update the top of the file to place a one-line explanatory
comment immediately above the existing attribute `#![cfg(not(loom))]` that
references the reason (e.g., "tests rely on real threads/timing and are not
deterministic under loom") so future readers understand why `#![cfg(not(loom))]`
is required.
- Around line 84-92: The test uses a verbose match + panic to handle the Result
from measure_encode; replace that pattern with a single .expect(...) call to
make failures idiomatic and clearer — e.g., in
encode_measurement_reports_requested_iterations (and the other test functions
that call measure_encode with VALIDATION_ITERATIONS), call
measure_encode(workload, VALIDATION_ITERATIONS).expect("encode measurement
failed") and then assert on measurement; apply the same replacement for the
other two occurrences that currently use match/panic.
In `@tests/common/codec_benchmark_support.rs`:
- Around line 223-225: The Measurement construction uses unchecked
multiplication for payload_bytes (payload_bytes: iterations * payload_len) which
can overflow; update these instances to use saturating_mul on iterations (e.g.,
iterations.saturating_mul(payload_len)) for defensive consistency with the alloc
benchmark; change all similar occurrences (including the other block around
lines 240-242) so Measurement { operations: iterations, payload_bytes:
iterations.saturating_mul(payload_len), ... } uses saturating_mul and keep the
rest of the fields unchanged.
- Around line 198-247: The two almost-identical match arms in measure_encode
(handling CodecUnderTest::LengthDelimited vs CodecUnderTest::Hotline) should be
deduplicated by extracting the common logic into a small macro or a trait helper
(mirror the existing codec_measure_decode_fn! pattern): create a macro (e.g.,
codec_measure_encode_fn!) or a trait method that takes the codec constructor and
error label, instantiates LengthDelimitedFrameCodec or HotlineFrameCodec, calls
.encoder(), loop-encodes codec.wrap_payload(payload.clone()) into a BytesMut,
and returns the Measurement; then replace both arms in measure_encode with calls
to that macro/helper using LengthDelimitedFrameCodec::new(...) and
HotlineFrameCodec::new(...) and appropriate error messages to eliminate
duplication while keeping measure_encode simple.
In `@tests/fixtures/codec_performance_benchmarks.rs`:
- Around line 117-135: The two discarded calls to nanos_per_op() inside
assert_fragmentation_overhead_available are unclear; either assert on their
returned values (e.g., ensure they are finite/greater than zero) or, if the
intent is only to ensure the methods don't panic, add a concise inline comment
above the calls stating "calling nanos_per_op() to verify it does not panic" so
future readers understand why the results are ignored; update the code in
assert_fragmentation_overhead_available around
overhead.unfragmented.nanos_per_op() and overhead.fragmented.nanos_per_op()
accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.tomlMakefilebenches/codec_performance.rsbenches/codec_performance_alloc.rsdocs/adr-004-pluggable-protocol-codecs.mddocs/execplans/9-6-1-codec-performance-benchmarks.mddocs/roadmap.mdtests/codec_performance_benchmark_helpers.rstests/common/codec_benchmark_support.rstests/features/codec_performance_benchmarks.featuretests/fixtures/codec_performance_benchmarks.rstests/fixtures/interleaved_push_queues.rstests/fixtures/mod.rstests/interleaved_push_queues.rstests/scenarios/codec_performance_benchmarks_scenarios.rstests/scenarios/mod.rstests/steps/codec_performance_benchmarks_steps.rstests/steps/mod.rs
…e benchmarks - Move common codec benchmark support types and functions to a shared module - Refactor encode/decode measurement functions using macros to reduce duplication - Introduce PreparedDecodeInput enum to unify decode input preparations - Improve allocation counting with scoped enable/disable and memory fences - Add helper function to keep shared symbols reachable for bench linking - Clean up benchmark code for better error handling and clarity - Minor docs updates and code style improvements related to benchmarks This refactor improves code maintainability, clarity, and benchmarking correctness without changing benchmark behavior. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/10-3-2-exercise-interleaved-queues.md`:
- Around line 392-411: Replace the indented command sequence (the repeated "set
-o pipefail" + "make fmt/check-fmt/lint/test-bdd/test/markdownlint/nixie ..."
lines) with a fenced code block using the shell language identifier (start with
```shell and end with ```), so the Stage D command sequence is a proper fenced
shell block that linters can validate; ensure you include all the original lines
inside the fenced block exactly as they appear.
In `@docs/execplans/9-6-1-codec-performance-benchmarks.md`:
- Around line 366-374: Replace the indented command list with a fenced code
block that includes a language identifier (use "shell") so linters can validate
it; locate the block containing the sequence of make commands (e.g., the lines
starting with "set -o pipefail; make fmt ..." through "set -o pipefail; make
bench-codec ...") and wrap those lines in a triple-backtick fenced block with
"shell" on the opening fence and a closing triple-backtick, preserving each
command exactly as-is.
In `@tests/fixtures/codec_performance_benchmarks.rs`:
- Around line 30-36: The public fixture function
codec_performance_benchmarks_world is missing a Rustdoc and the doc comment must
be placed above the attribute; add a short /// Rustdoc describing the fixture
(what it returns and that it sets iterations to VALIDATION_ITERATIONS) and move
that doc comment so it precedes the #[fixture] attribute on the
codec_performance_benchmarks_world function; leave the function body intact
(returning CodecPerformanceBenchmarksWorld with iterations:
VALIDATION_ITERATIONS and ..CodecPerformanceBenchmarksWorld::default()).
- Around line 22-28: Add a Rustdoc comment for the public struct
CodecPerformanceBenchmarksWorld that briefly describes its purpose (holds
benchmark configuration and state for codec performance tests) and documents key
fields like iterations, matrix_samples, fragmentation_overhead, and
allocation_labels so cargo doc will produce complete API docs; place the
triple-slash comments (///) immediately above the
CodecPerformanceBenchmarksWorld declaration and include short descriptions for
the struct and each field.
- Around line 38-153: Add Rustdoc comments (///) to every public method in impl
CodecPerformanceBenchmarksWorld: configure_iterations, run_codec_matrix_samples,
run_fragmentation_sample, build_allocation_labels,
assert_matrix_samples_cover_workloads, assert_fragmentation_overhead_available,
and assert_allocation_labels_include_counters; for each method include a
one-line summary of behavior, `# Arguments` (or `# Parameters`) describing
inputs like `iterations`, any preconditions (e.g., iterations must be > 0 or
must be configured first), and `# Returns` describing the TestResult success
case and possible Err conditions so docs generated by cargo doc clearly show
usage and failure modes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
benches/codec_performance.rsbenches/codec_performance_alloc.rsdocs/adr-004-pluggable-protocol-codecs.mddocs/execplans/10-3-2-exercise-interleaved-queues.mddocs/execplans/9-6-1-codec-performance-benchmarks.mdsrc/app_data_store.rstests/codec_performance_benchmark_helpers.rstests/common/codec_benchmark_support.rstests/fixtures/codec_performance_benchmarks.rs
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: benches/codec_performance_alloc.rs Comment on file fn prepare_length_delimited_decode_input(
payload: Bytes,
) -> Result<(Bytes, LengthDelimitedFrameDecoder), String> {
let codec = LengthDelimitedFrameCodec::new(LARGE_PAYLOAD_BYTES + 4096);
let mut seed_encoder = codec.encoder();
let mut encoded = BytesMut::new();
seed_encoder
.encode(codec.wrap_payload(payload), &mut encoded)
.map_err(|err| format!("length-delimited seed encode failed: {err}"))?;
Ok((encoded.freeze(), codec.decoder()))
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
… tests - Introduce `CodecPerformanceBenchmarksWorld` struct to hold benchmark state. - Provide configuration, sampling, and assertion methods for encoding/decoding benchmarks. - Replace repetitive codec input preparation functions with a macro to reduce duplication. - Add Markdown lint disables around shell script blocks in execplan docs to fix lint warnings. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/codec_performance_alloc.rs`:
- Around line 253-267: The encode allocation measurement is asymmetric:
count_allocations currently calls measure_encode in VALIDATION_ITERATIONS
separate invocations (measure_encode, VALIDATION_ITERATIONS) while decode runs
many iterations inside a single closure via run_prepared_decode_iterations after
prepare_decode_input; to fix, make the encode path mirror decode by either (A)
preparing an encode input and adding a run_prepared_encode_iterations(&mut
prepared_encode_input, VALIDATION_ITERATIONS) invoked once inside
count_allocations, or (B) move the for _ in 0..VALIDATION_ITERATIONS loop into
the closure passed to count_allocations so count_allocations measures the whole
batch (use measure_encode inside that single closure), and keep references to
measure_encode, prepare_decode_input, run_prepared_decode_iterations,
count_allocations, and VALIDATION_ITERATIONS when implementing the change.
- Around line 226-246: Remove the dead helper function
keep_shared_support_symbols_reachable and its invocation and instead add a
scoped lint allow with a FIXME reason on the module declaration (e.g., replace
the top-level mod declaration with something like #[allow(dead_code)] // FIXME:
reason) so the unused symbols are explicitly documented; delete the function and
its call, and remove the unused imports FRAGMENT_PAYLOAD_CAP_BYTES,
FragmentationOverhead, Measurement, PayloadClass, measure_unfragmented_wrap,
measure_fragmented_wrap, and measure_fragmentation_overhead from the use block
so no dead-code suppression is hidden inside a dummy function.
In `@docs/execplans/9-6-1-codec-performance-benchmarks.md`:
- Around line 164-168: Update the sentence in the Decision block that currently
reads "macros in order to satisfy strict `missing_docs` linting..." by replacing
"in order to" with "to" so it reads "macros to satisfy strict `missing_docs`
linting..."; locate this text in the Decision paragraph (the line starting
"Decision: implement criterion entrypoints...") and apply the simple phrasing
change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
benches/codec_performance_alloc.rsdocs/execplans/10-3-2-exercise-interleaved-queues.mddocs/execplans/9-6-1-codec-performance-benchmarks.mdtests/fixtures/codec_performance_benchmarks.rs
…lify allocation benchmark Removed the keep_shared_support_symbols_reachable function and its usage, which kept various unused measurement helpers alive unnecessarily. Cleaned up imports by removing unused items related to fragmentation measurement. Simplified the allocation benchmark by removing redundant loops and calls, improving code clarity and reducing dead code. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/codec_performance_alloc.rs`:
- Around line 22-28: Update the FIXME reason on the #[expect(dead_code, reason =
"...")] attribute that precedes the mod codec_benchmark_support declaration to
include a tracking issue URL (e.g., "FIXME: https://github.com/.../issues/NN -
split shared benchmark support to avoid importing unused helpers in this
bench"); if no issue exists, create one to track splitting the shared benchmark
support module and then reference its URL in the FIXME reason string.
In `@docs/execplans/9-6-1-codec-performance-benchmarks.md`:
- Line 209: Replace the phrase "touch points" with the single word "touchpoints"
in the sentence "Primary implementation files and expected touch points:" within
the docs/execplans/9-6-1-codec-performance-benchmarks.md file so it reads
"Primary implementation files and expected touchpoints:"; search for the exact
string "expected touch points" to locate and update the occurrence.
- Around line 366-376: Remove the leading four-space indentation from each shell
command inside the fenced code block so the commands (e.g., the lines starting
with "set -o pipefail; make fmt", "set -o pipefail; make check-fmt", ..., "set
-o pipefail; make bench-codec") begin at column 1; update the fenced block in
the document so each command line has no extra leading whitespace.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
benches/codec_performance_alloc.rsdocs/execplans/9-6-1-codec-performance-benchmarks.md
- Removed extra indentation in shell commands block - Corrected 'touch points' to 'touchpoints' - Updated FIXME comment with issue URL in benchmarks code Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
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
1 participant
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.
Summary
Changes
Rationale / Why
Testing plan
Follow-ups (post-merge)
Notes for reviewers
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/97b59cec-f42a-424a-95d3-d7ebea2f8bd6