ITADN

Drop EventName clones on cache hits in nm_otel registry lookups

#159Opensandersaares 创建于 2026-05-24
enhancement
S
sandersaarescommented
## Proposal Both `nm_otel` lookup helpers clone the `EventName` (a `Cow<'static, str>`) on every export iteration, even when the entry already exists: - `packages/nm_otel/src/state.rs::CollectionState::event_state`: ```rust self.events.entry(name.clone()).or_default() ``` - `packages/nm_otel/src/mapping.rs::InstrumentRegistry::instruments`: ```rust self.events.entry(event_name.clone()).or_insert_with(|| { ... }) ``` Switch to a **lookup-first** pattern that only clones on insertion: ```rust if let Some(entry) = self.events.get_mut(name) { return entry; } self.events.insert(name.clone(), EventState::default()); self.events.get_mut(name).expect("just inserted") ``` …or use `raw_entry_mut` (nightly) or `hashbrown`'s `entry_ref` (already pulled in transitively via `foldhash`?) for a clean one-shot lookup-and-insert that borrows the key. ## Baseline - 2 clones per event per export iteration. - For `Cow::Borrowed(&'static str)`, a clone is a `Copy` of the fat pointer (16 bytes) — cheap. - For `Cow::Owned(String)`, a clone allocates and copies the string — **expensive** (a heap allocation per event per export). `nm_observe_cg::aggregate_group::collect` (8 events) measures 17 387 instructions / 31 327 cycles. Pure-static-string workloads see ~negligible improvement; owned-string workloads see one allocation per event per export saved. ## Expected magnitude - For static-string event names (the documented "recommended" pattern): negligible. - For owned event names (dynamically-registered events, called out as a supported scenario in the crate-level docs): **one allocation per event per export iteration saved**, twice (state + instruments). - Per-event constant-factor cycle reduction even on static strings: 2 fewer fat-pointer copies (~2–4 instructions). ## Implementation strategy 1. Inspect `foldhash::HashMap` (re-exported `hashbrown::HashMap` with foldhash) for `raw_entry_mut` or `entry_ref` availability. If available, use it. 2. Otherwise, the two-step `get_mut`-then-`insert` pattern. The double lookup is essentially free because the hash is recomputed once but the second lookup is identical to a hit-path lookup. 3. Consider centralizing this in a small helper: ```rust fn get_or_insert_with<'a, V>( map: &'a mut HashMap<EventName, V>, key: &EventName, make: impl FnOnce() -> V, ) -> &'a mut V { if !map.contains_key(key) { map.insert(key.clone(), make()); } map.get_mut(key).expect("just inserted") } ``` (Two-lookup version; if `hashbrown::raw_entry_mut` works, use it for a single-lookup variant.) 4. Apply at both call sites. ## Validation - All existing tests cover correctness (state/instrument identity). - If a `nm_otel` Criterion benchmark is added (see #4), confirm allocation drop via `alloc_tracker`. ## Risk Trivial.
1 条评论