ITADN

[API change] Visitor-style report collection: Report::visit

#165Opensandersaares 创建于 2026-05-24
enhancement
S
sandersaarescommented
## 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 条评论