Skip to content

Commit 9004e34

Browse files
excelle08meta-codesync[bot]
authored andcommitted
Channel keepalive eliminates cold-channel latency cliff
Summary: Adds a per-MockServicesClient keepalive timer that fires a fire-and-forget `getStatus()` probe RPC every N milliseconds, keeping Rocket channels warm between sparse session bursts. After D105903218 distributed outbound RPCs across one MockServicesClient per SREventBase (123 channels on BGM), the t25 QPS-latency sweep surfaced a severe cold-channel anti-pattern: p95 latency at low offered load was up to 14× *worse* than at peak load on BGM (9,488 ms at q=5/inst vs 684 ms at q=35/inst), with throughput collapsing to 31% of requested (1.55 of 5 QPS). Grace and CPL showed milder 2.5-2.8× cliffs. Root cause (confirmed via perf-record diff at q=5 vs q=35 on BGM): at low offered load, ≈1.5 sessions in-flight × K=16 fanout = 24 RPCs spread across 123 channels — most channels idle for 100-300 ms between bursts. Each cold RPC then pays three compounding penalties: Rocket channel re-arm (+2.4pp Folly, +1.9pp RPC-AsyncIO at q=5), deep C-state wake on idle SREventBase cores (Zen4c C6 wake ≈100 µs; `poll_idle` + `acpi_processor_ffh_cstate_enter` in top 14 hot functions), and cold allocator/JIT caches (+4.1pp MemAlloc, +2.0pp JIT-Unresolved). All three penalties share the same proximate cause: channel idle time > 100 ms. Touching every channel every 150 ms via a cheap getStatus() probe eliminates all three penalties simultaneously. The probe runs on the channel's own EventBase (thread-affine) and is fire-and-forget (drops the returned SemiFuture); transport errors are swallowed silently to avoid keepalive failures propagating to the application path. Bandwidth cost: ~13K pings/sec/instance × ~100 bytes round-trip = ~1.3 MB/s — negligible. mock_services CPU cost ≈0.07 cores per instance. Knobs: - New CLI flag `--mock_keepalive_interval_ms` on LeafNodeRank (default 0 = disabled so the anti-pattern stays observable for regression testing). - New `MOCK_KEEPALIVE_INTERVAL_MS` env override in run.sh so sweep scripts can A/B without rebuilding. - Recommended starting value: 150 ms (validated). 300-500 ms likely sufficient; tunable. Reviewed By: YifanYuan3 Differential Revision: D106558423
1 parent b3038be commit 9004e34

5 files changed

Lines changed: 127 additions & 11 deletions

File tree

packages/feedsim/run.sh

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -719,6 +719,16 @@ main() {
719719
mock_services_opts="$mock_services_opts --use_legacy_sleep"
720720
echo "RPC fanout: forced OFF via LEAFNODE_USE_LEGACY_SLEEP=1 (legacy sleep path)"
721721
fi
722+
# t25 mitigation knob: per-MockServicesClient keepalive ping. When
723+
# MOCK_KEEPALIVE_INTERVAL_MS is set and > 0, each channel issues a
724+
# 1-byte getStatus() probe every N ms to defeat the cold-channel
725+
# anti-pattern observed at low QPS (BGM saw 14x p95 cliff at q=5).
726+
# Recommended starting value: 150-500 ms. 0 / unset = disabled
727+
# (anti-pattern stays observable).
728+
if [ -n "${MOCK_KEEPALIVE_INTERVAL_MS:-}" ] && [ "${MOCK_KEEPALIVE_INTERVAL_MS}" != "0" ]; then
729+
mock_services_opts="$mock_services_opts --mock_keepalive_interval_ms=${MOCK_KEEPALIVE_INTERVAL_MS}"
730+
echo "MockServicesClient keepalive: ENABLED (interval=${MOCK_KEEPALIVE_INTERVAL_MS} ms)"
731+
fi
722732

723733
# OMP_NUM_THREADS=1: cap PyTorch's OpenMP parallel backend pool to
724734
# 1 thread. at::set_num_threads(1) only affects libtorch's native

packages/feedsim/third_party/src/workloads/ranking/LeafNodeRank.cc

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,9 @@ void ThreadStartup(
321321
std::make_unique<ranking::MockServicesClient>(
322322
evbs[i].get(),
323323
args.mock_services_host_arg,
324-
static_cast<uint16_t>(args.mock_services_port_arg)));
324+
static_cast<uint16_t>(args.mock_services_port_arg),
325+
std::chrono::milliseconds(
326+
args.mock_keepalive_interval_ms_arg)));
325327
} catch (const std::exception& e) {
326328
std::cerr << "Failed to connect to mock_services on "
327329
<< args.mock_services_host_arg << ":"
@@ -468,7 +470,9 @@ void ThreadStartup(
468470
std::make_unique<ranking::MockServicesClient>(
469471
evbs[i].get(),
470472
args.mock_services_host_arg,
471-
static_cast<uint16_t>(args.mock_services_port_arg)));
473+
static_cast<uint16_t>(args.mock_services_port_arg),
474+
std::chrono::milliseconds(
475+
args.mock_keepalive_interval_ms_arg)));
472476
} catch (const std::exception& e) {
473477
std::cerr << "Failed to connect to mock_services on "
474478
<< args.mock_services_host_arg << ":"
@@ -2357,7 +2361,9 @@ int main(int argc, char** argv) {
23572361
std::cout << "RPC fanout: enabled (target "
23582362
<< args.mock_services_host_arg << ":"
23592363
<< args.mock_services_port_arg
2360-
<< ", scale=" << args.rpc_fanout_scale_arg << ")"
2364+
<< ", scale=" << args.rpc_fanout_scale_arg
2365+
<< ", keepalive_ms=" << args.mock_keepalive_interval_ms_arg
2366+
<< ")"
23612367
<< std::endl;
23622368
} else {
23632369
std::cout << "RPC fanout: disabled (no --rpc_dist_path); using legacy"

packages/feedsim/third_party/src/workloads/ranking/LeafNodeRankCmdline.ggo

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,3 +96,13 @@ option "rpc_fanout_scale" - "Scale factor applied to per-session fanout counts.
9696
# mock_services attempted). Lets later diffs in the stack run end-to-end
9797
# integration tests without depending on the mock_services side process.
9898
option "use_legacy_sleep" - "Force legacy folly::futures::sleep path; skip MockServicesClient construction and outbound RPC fanout." flag off
99+
100+
# Mitigation knob for the t25 cold-channel anti-pattern: when offered load is
101+
# low, MockServicesClient channels go idle between sparse session bursts.
102+
# Per-channel keepalive issues a 1-byte getStatus() ping every N ms to keep
103+
# the Rocket channel + EventBase warm, defeating the cold-wake + deep-C-state
104+
# penalty observed at low QPS on BGM (p95 cliff: 14x at q=5). 0 = disabled
105+
# (default = keepalive off so the anti-pattern stays observable for regression
106+
# tests). Recommended: 150-500ms. See ~/.claude/plans/ancient-launching-pizza.md
107+
# section "2026-05-27 -- t25" for the root-cause analysis.
108+
option "mock_keepalive_interval_ms" - "Interval (ms) between per-MockServicesClient keepalive pings. 0 = disabled." int default="0"

packages/feedsim/third_party/src/workloads/ranking/MockServicesClient.cc

Lines changed: 86 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,68 @@
2929

3030
namespace ranking {
3131

32+
// KeepaliveTimer fires a fire-and-forget getStatus() RPC every
33+
// keepalive_interval_ms to keep the underlying Rocket channel warm.
34+
//
35+
// Why this exists: t25 (2026-05-27) measured a 14x latency cliff on BGM at
36+
// low QPS, traced to cold MockServicesClient channels. After D105903218
37+
// each leaf thread owns one MockServicesClient per SREventBase (~123 on
38+
// BGM); at qps=5 most channels see no traffic for 100-300ms between
39+
// session bursts, then pay re-arm + deep C-state wake costs on the next
40+
// RPC. Per-channel keepalive defeats this by ensuring every channel sees
41+
// traffic at least every keepalive_interval_ms.
42+
//
43+
// Thread-affinity: scheduled on the same EventBase as the parent
44+
// MockServicesClient, so the callback runs on the right thread to call
45+
// dispatchByEnum() (which is thread-affine). Drops the resulting
46+
// SemiFuture immediately (fire-and-forget); transport errors are
47+
// swallowed silently — keepalive failures should not propagate to the
48+
// application path.
49+
class MockServicesClient::KeepaliveTimer : public folly::AsyncTimeout {
50+
public:
51+
KeepaliveTimer(
52+
folly::EventBase* evb,
53+
MockServicesClient* parent,
54+
std::chrono::milliseconds interval)
55+
: folly::AsyncTimeout(evb), parent_(parent), interval_(interval) {}
56+
57+
// Caller must invoke from the EventBase thread (or via
58+
// runInEventBaseThread). scheduleTimeout itself is thread-affine.
59+
void start() { scheduleTimeout(interval_); }
60+
61+
void timeoutExpired() noexcept override {
62+
// Reschedule first so we don't drop the next tick if dispatch throws.
63+
scheduleTimeout(interval_);
64+
65+
// Fire-and-forget getStatus() with latency_us=0 (lightest possible
66+
// server-side handler). The 4-byte response header is the wire-
67+
// contract minimum. We don't await — the returned SemiFuture goes
68+
// out of scope but the underlying RPC remains in flight on the
69+
// channel until completion, achieving the warming effect.
70+
try {
71+
std::string probe(4, '\0');
72+
writeBigEndianResponseSize(probe.data(), 4);
73+
parent_->client_->semifuture_getStatus(probe, /*latency_us=*/0)
74+
.via(parent_->evb_)
75+
.thenValue([](std::string&&) {})
76+
.thenError(folly::tag_t<std::exception>{}, [](const std::exception&) {
77+
// Swallow transport errors silently. The next iter will retry.
78+
});
79+
} catch (...) {
80+
// Defensive — never let the timer callback propagate.
81+
}
82+
}
83+
84+
private:
85+
MockServicesClient* parent_;
86+
std::chrono::milliseconds interval_;
87+
};
88+
3289
MockServicesClient::MockServicesClient(
33-
folly::EventBase* evb, const std::string& host, uint16_t port)
90+
folly::EventBase* evb,
91+
const std::string& host,
92+
uint16_t port,
93+
std::chrono::milliseconds keepalive_interval)
3494
: evb_(evb) {
3595
if (evb_ == nullptr) {
3696
throw std::invalid_argument(
@@ -88,15 +148,34 @@ MockServicesClient::MockServicesClient(
88148
evb_->runInEventBaseThreadAndWait([this]() { client_.reset(); });
89149
std::rethrow_exception(result.exception().to_exception_ptr());
90150
}
151+
152+
// Start keepalive timer if requested. Must be constructed and started
153+
// on the EventBase thread because AsyncTimeout is thread-affine.
154+
if (keepalive_interval.count() > 0) {
155+
evb_->runInEventBaseThreadAndWait([this, keepalive_interval]() {
156+
keepalive_ =
157+
std::make_unique<KeepaliveTimer>(evb_, this, keepalive_interval);
158+
keepalive_->start();
159+
});
160+
}
91161
}
92162

93163
MockServicesClient::~MockServicesClient() {
94-
// The AsyncClient and its channel must be destroyed on the EventBase
95-
// thread. Use runInEventBaseThreadAndWait to guarantee that even when
96-
// the destructor runs from a different thread (e.g., during process
97-
// shutdown).
98-
if (evb_ != nullptr && client_) {
99-
evb_->runInEventBaseThreadAndWait([this]() { client_.reset(); });
164+
// The AsyncClient, its channel, and the keepalive timer are all
165+
// thread-affine to evb_. Destroy them on that thread to guarantee
166+
// correct cleanup even when the destructor runs from a different
167+
// thread (e.g., during process shutdown). cancelTimeout must precede
168+
// destruction or AsyncTimeout will assert.
169+
if (evb_ != nullptr) {
170+
evb_->runInEventBaseThreadAndWait([this]() {
171+
if (keepalive_) {
172+
keepalive_->cancelTimeout();
173+
keepalive_.reset();
174+
}
175+
if (client_) {
176+
client_.reset();
177+
}
178+
});
100179
}
101180
}
102181

packages/feedsim/third_party/src/workloads/ranking/MockServicesClient.h

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,14 @@
1414

1515
#pragma once
1616

17+
#include <chrono>
1718
#include <cstdint>
1819
#include <memory>
1920
#include <string>
2021

2122
#include <folly/SocketAddress.h>
2223
#include <folly/futures/Future.h>
24+
#include <folly/io/async/AsyncTimeout.h>
2325
#include <folly/io/async/EventBase.h>
2426

2527
#include "RpcDistRegistry.h"
@@ -66,7 +68,9 @@ class MockServicesClient {
6668
MockServicesClient(
6769
folly::EventBase* evb,
6870
const std::string& host,
69-
uint16_t port);
71+
uint16_t port,
72+
std::chrono::milliseconds keepalive_interval =
73+
std::chrono::milliseconds(0));
7074

7175
~MockServicesClient();
7276

@@ -97,8 +101,15 @@ class MockServicesClient {
97101
folly::EventBase* getEventBase() const { return evb_; }
98102

99103
private:
104+
// KeepaliveTimer (defined in .cc) fires a fire-and-forget getStatus()
105+
// probe RPC every keepalive_interval_ms to keep the underlying Rocket
106+
// channel + SREventBase warm. Forward declared here so the unique_ptr
107+
// member doesn't pull AsyncTimeout into the header's public surface.
108+
class KeepaliveTimer;
109+
100110
folly::EventBase* evb_;
101111
std::unique_ptr<mock_services::MockServiceAsyncClient> client_;
112+
std::unique_ptr<KeepaliveTimer> keepalive_;
102113
};
103114

104115
/**

0 commit comments

Comments
 (0)