cmcfarlen commented on code in PR #14000:
URL: https://github.com/apache/trafficserver/pull/14000#discussion_r4474241461
##########
include/tsutil/Metrics.h:
##########
@@ -431,6 +431,78 @@ class Metrics
std::shared_ptr<Storage> _storage;
public:
+ /** Per-thread accumulation of counter increments.
+
+ Counters are summed by every thread that serves transactions, so
incrementing the shared atomic
+ directly makes every thread write the same cache lines. A thread that
owns a table adds into it
+ instead and periodically applies the sums with one atomic add per
counter it touched. Readers
+ still read only the shared atomics, which lag by at most the owning
thread's flush interval.
+
+ Gauges are not deferred: their current value is used for decisions, such
as connection limits.
+ */
+ class ThreadDeferredCounters
+ {
+ public:
+ /// Adds @a val to @a metric's pending sum. Returns false if the table has
no room for @a metric,
+ /// in which case the caller must apply the increment itself.
+ bool
+ add(AtomicType *metric, int64_t val)
+ {
+ size_t slot = (reinterpret_cast<uintptr_t>(metric) >> 3) *
0x9E3779B97F4A7C15ULL >> (64 - SLOT_BITS);
+
+ for (size_t probe = 0; probe < MAX_PROBES; ++probe, slot = (slot + 1) &
(SLOTS - 1)) {
+ Slot &s = _slots[slot];
+ if (s.metric == metric) {
+ s.delta += val;
Review Comment:
Fixed in 3c38edbac3: pending sums are now uint64_t and are applied with a
cast, so they wrap the same way the direct fetch_add path does.
##########
src/iocore/eventsystem/UnixEThread.cc:
##########
@@ -264,6 +272,13 @@ EThread::execute_regular()
EventStatistics statistics = fire_ready_events(loop_start_time);
ink_hrtime sleep_time = get_sleep_time(statistics.event_time,
current_slice);
+ // Flush before a sleep that would outlast the interval too, so an idle
thread's counters are current.
+ if (counter_flush_interval > 0 &&
+ (statistics.event_time - last_counter_flush >= counter_flush_interval
|| sleep_time >= counter_flush_interval)) {
Review Comment:
Right: `sleep_time` is only an upper bound, and `do_poll` returns at once
while the ready/enable lists are non-empty, so busy threads were flushing every
loop turn. Fixed in 3c38edbac3. The loop now keeps only the elapsed-time check,
and the idle flush moved to the real blocking points: `PollCont::do_poll` when
its timeout is non-zero, and `DefaultTailHandler::waitForActivity` with a
positive timeout.
##########
src/iocore/eventsystem/UnixEThread.cc:
##########
@@ -264,6 +272,13 @@ EThread::execute_regular()
EventStatistics statistics = fire_ready_events(loop_start_time);
ink_hrtime sleep_time = get_sleep_time(statistics.event_time,
current_slice);
+ // Flush before a sleep that would outlast the interval too, so an idle
thread's counters are current.
+ if (counter_flush_interval > 0 &&
+ (statistics.event_time - last_counter_flush >= counter_flush_interval
|| sleep_time >= counter_flush_interval)) {
+ ts::Metrics::flush_thread_counters();
Review Comment:
Leaving this as is. A continuation chain that keeps the local queue draining
past the interval already starves network polling, which is the bigger problem
in that case. Checking the deadline per dispatched event would add a cost to
the hot path to cover it. The counters are still published as soon as the chain
ends.
--
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]
