[API change] Visitor-style report collection: Report::visit
enhancement
## Proposal
**[API change]** Introduce a streaming, visitor-style alternative to `Report::collect()`:
```rust
impl Report {
/// Visit each event's metrics without materializing the full report.
pub fn visit(&self, visitor: impl FnMut(&EventMetrics));
}
```
Currently `Report::collect()` materializes an entire `Vec<EventMetrics>` (with allocations per event: bucket-count snapshots, owned strings if any, etc.) and returns it to the caller, who typically iterates once and discards.
`Report::visit` allows the caller to consume each `EventMetrics` as it is produced, reusing scratch storage internally.
## Baseline
| Scenario | Instructions | Cycles |
|-----------------------------------|--------------|--------|
| Aggregate collect (8 events) | 17 387 | 31 327 |
This is dominated by allocation costs and HashMap insertion. With visitor-style streaming:
- No `Vec<EventMetrics>` allocation.
- No per-event bucket-count `Box<[u64]>` allocation (the callback reads from a scratch buffer that is reused across calls).
- No HashMap if sort order is not needed, or the HashMap is reused across `visit()` calls if it is.
## Expected magnitude
- **30–60 % reduction** in `collect`-equivalent instruction count.
- Larger reduction in allocator pressure (no transient allocations per event).
- nm_otel's `export_report` is the primary consumer — switching it to `Report::visit` would propagate the win to the whole export pipeline.
## Implementation strategy
### API
```rust
// in nm/src/reports.rs:
impl Report {
pub fn visit(&self, mut visitor: impl FnMut(&EventMetrics));
// existing collect() can stay (deprecated) or be re-implemented on top of visit().
pub fn collect(&self) -> Vec<EventMetrics> {
let mut out = Vec::new();
self.visit(|m| out.push(m.clone()));
out
}
}
```
### EventMetrics for borrowed scratch
The visitor receives `&EventMetrics`. If today's `EventMetrics` owns its bucket-count storage, that needs to change:
```rust
pub struct EventMetrics<'a> {
pub name: &'a EventName,
pub bucket_magnitudes: &'a [Magnitude],
pub bucket_counts: &'a [u64], // borrows scratch
pub count: u64,
pub sum: i64,
// ...
}
```
The visitor sees borrowed data; if it needs to retain anything, it clones explicitly.
### Sort order
Today's `collect` may sort events for deterministic output. Visitor-style needs to decide:
- Option A: `visit` is unsorted; `collect` sorts after collecting.
- Option B: `visit` is sorted (requires materializing the order first, partially defeating the optimization).
- Option C: `visit_sorted` and `visit` are separate methods.
Recommend Option C — let the caller choose. nm_otel exports do not require stable order across iterations.
### nm_otel consumer migration
Update `nm_otel::Publisher::export_report` to use `Report::visit` directly, eliminating the intermediate `Vec<EventMetrics>`.
## Constraints / risks
- **API impact**: `EventMetrics` changes shape (borrowed vs. owned). All consumers must migrate.
- The crate-level docs guarantee `Report::collect` exists; deprecate slowly.
- Lifetime gymnastics with the borrowed scratch buffer — the visitor closure cannot outlive the borrow.
- If consumers want to store `EventMetrics` for later use, they need an explicit `.to_owned()` step.
## Validation
1. `wsl -e bash -l -c "just package=nm bench-cg"` aggregate collect scenario — confirm reduction.
2. Add a `visit`-based bench-cg scenario for nm_otel export.
3. `just package=nm test` and `just package=nm_otel test`.
## Related
- Subsumes the snapshot-buffer reuse issue (the visitor pattern naturally reuses scratch).
- Pairs well with the streaming-deltas issue in nm_otel — visitor on the producer side, streaming on the consumer side.
0 条评论