Copilot commented on code in PR #14000:
URL: https://github.com/apache/trafficserver/pull/14000#discussion_r4474004226
##########
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:
This changes counter overflow from defined atomic wraparound to undefined
signed arithmetic. `Counter::increment()` accepts a `uint64_t`, so deferring
increments of `INT64_MAX` and then `1` for the same counter overflows `s.delta`
before it is flushed, whereas the direct `fetch_add` path handles that sequence
without undefined behavior. Please accumulate using unsigned arithmetic,
preserving the direct path's wrapping semantics.
##########
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:
`sleep_time` is only an upper bound: `PollCont::do_poll()` forces a zero
timeout when network ready/enable lists are nonempty, so this can flush every
busy loop turn and defeat batching. Conversely, successive waits shorter than
the interval can cross the flush deadline without publishing pending counters.
Keep the elapsed-time check here, but move idle flushing to the actual
blocking-wait boundaries, after pre-poll counter updates, so busy threads
retain batching and sleeping threads publish their pending increments.
##########
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:
The flush deadline is checked only after `fire_ready_events()` finishes
draining its queues. A chain of continuations using `schedule_imm_local()` can
keep `process_queue()` draining beyond the configured interval, leaving their
counter increments unpublished until the chain ends. Also check the deadline
during event dispatch, and add a regression case that keeps the local queue
busy longer than the interval.
--
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]
