cmcfarlen commented on PR #13506: URL: https://github.com/apache/trafficserver/pull/13506#issuecomment-5402548493
@serrislew @bryancall — on the `add_source` linear scan, since you both landed on it independently. I measured rather than guessed. `DerivedMetric` is 40 bytes (`IdType` is `int32_t`, plus a 24-byte source vector and the op), so `std::find_if` is a contiguous sweep comparing one `int32_t` at stride 40: | distinct group identities | derived entries (default `metric_aggregate: 0`, 3/group) | per group creation (3 scans) | |---|---|---| | ~33 | ~100 | ~4 KB swept, ~0.5 µs | | 500 | ~1500 | ~180 KB swept, ~10–20 µs | For context on what shares that call path: the `Group` constructor already does 3–7 `std::string` concatenations (a heap allocation each) plus 3–7 `_create` calls, each taking the metrics-store mutex and hashing a ~50 character name — on the order of 1–3 µs. So at the cardinality this actually runs at, the scan is **cheaper than the string work sitting next to it**, and an id-indexed map would not be measurable. It starts to matter in the high hundreds of distinct upstreams, and even then it is ~10 µs on a path that also does a DNS lookup and a TCP connect. The contention concern is the sharper version of this, and the map does not address it either: the serialization is `_outbound_table._mutex` held across the whole constructor, plus `metrics_lock`, which `update()` also takes every sync tick while walking every entry. Making the lookup O(1) shortens the hold slightly without changing which locks are held or for how long in aggregate. What actually makes the linear scan the right structure is bounding N, which is the registry lifetime issue Bryan raised separately: `~Group` never unregisters, so entries accumulate per distinct `(fqdn, addr, port)` identity ever seen, and `update()` walks dead groups forever. Worth being precise about the growth, though — `add_source` dedups by source pointer and the hidden slots are name-keyed, so a reaped group that comes back reuses its existing entry. Churn is free; growth is bounded by distinct upstream identity cardinality, which plateaus for a fixed origin set. Not a per-connection leak. We are deferring that cleanup to a follow-up rather than growing this PR, because the design needs a discussion first: 1. Whether real deployments churn the identity space enough for the growth to matter at all. 2. If so, `remove_source` needs a final refresh before erasing an entry (otherwise a published gauge freezes at its last synced value instead of dropping to 0, since the hot path only writes the hidden copy) and refcounted registrations (otherwise an overlapping reap and re-create can strip a live group's publication). 3. Retiring metrics from `Storage` is the harder half. Since pointers handed out are raw and never invalidated, and a slot is only 8 bytes plus a name, reclamation buys almost nothing — the costs that bite are enumeration and `update()`. A tombstone that hides a metric from both, without ever invalidating a pointer, looks like the right first step. Filing that as a separate issue so it does not get lost. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
