Skip to content

Commit 44a336a

Browse files
committed
fix(imc): make frame timestamp cache thread-local and fix has_pending race
Convert g_FrameTimestampNs from a global variable to thread_local storage to prevent timestamp leakage across threads in multi-threaded publish scenarios. Use atomic exchange for has_pending flag in Pump() to eliminate the race between checking and clearing. Set has_pending after successful enqueue in both backpressure paths and re-check queue after drain to handle concurrent publishes correctly. Add test to verify frame timestamp cache isolation between threads.
1 parent 810f0e4 commit 44a336a

3 files changed

Lines changed: 58 additions & 11 deletions

File tree

src/Core/ImcBusPublish.cpp

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,7 @@ namespace BML::Core {
212212
case BML_BACKPRESSURE_DROP_OLDEST:
213213
ApplyBackpressure(sub, message);
214214
if (sub->queue && sub->queue->Enqueue(message, message->priority)) {
215+
sub->has_pending.store(true, std::memory_order_release);
215216
sub->RecordReceived(message->Size());
216217
m_GlobalStats.total_messages_delivered.fetch_add(
217218
1, std::memory_order_relaxed);
@@ -230,6 +231,7 @@ namespace BML::Core {
230231
for (int spin = 0; spin < 100; ++spin) {
231232
std::this_thread::yield();
232233
if (sub->queue && sub->queue->Enqueue(message, message->priority)) {
234+
sub->has_pending.store(true, std::memory_order_release);
233235
sub->RecordReceived(message->Size());
234236
m_GlobalStats.total_messages_delivered.fetch_add(
235237
1, std::memory_order_relaxed);
@@ -915,30 +917,33 @@ namespace BML::Core {
915917

916918
void ImcBusImpl::Pump(size_t max_per_sub) {
917919
// Cache timestamp for the entire frame to avoid repeated QPC syscalls.
918-
g_FrameTimestampNs = GetTimestampNsRaw();
920+
SetFrameTimestampCache(GetTimestampNsRaw());
919921

920922
m_GlobalStats.pump_cycles.fetch_add(1, std::memory_order_relaxed);
921-
m_GlobalStats.last_pump_time.store(g_FrameTimestampNs, std::memory_order_relaxed);
923+
m_GlobalStats.last_pump_time.store(GetFrameTimestampCache(),
924+
std::memory_order_relaxed);
922925

923926
DrainRpcQueue(max_per_sub);
924927

925928
{
926929
SnapshotGuard guard(m_PublishState.snapshot.load(std::memory_order_acquire));
927930
if (!guard) {
928-
g_FrameTimestampNs = 0;
931+
SetFrameTimestampCache(0);
929932
return;
930933
}
931934

932935
for (auto *sub : guard.get()->all_subs) {
933936
if (sub->closed.load(std::memory_order_acquire))
934937
continue;
935938
// Fast skip: no pending messages means empty queue, avoid drain overhead.
936-
if (!sub->has_pending.load(std::memory_order_acquire))
939+
if (!sub->has_pending.exchange(false, std::memory_order_acq_rel))
937940
continue;
938-
sub->has_pending.store(false, std::memory_order_relaxed);
939941
sub->ref_count.fetch_add(1, std::memory_order_relaxed);
940942
DrainSubscription(sub, max_per_sub);
941943
sub->ref_count.fetch_sub(1, std::memory_order_acq_rel);
944+
if (sub->queue && !sub->queue->IsEmpty()) {
945+
sub->has_pending.store(true, std::memory_order_release);
946+
}
942947
}
943948
}
944949

@@ -952,6 +957,6 @@ namespace BML::Core {
952957
}
953958
}
954959

955-
g_FrameTimestampNs = 0;
960+
SetFrameTimestampCache(0);
956961
}
957962
} // namespace BML::Core

src/Core/ImcBusSharedInternal.h

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,13 +62,20 @@ namespace BML::Core {
6262
duration_cast<nanoseconds>(steady_clock::now().time_since_epoch()).count());
6363
}
6464

65-
// Cached per-frame timestamp. Set once at Pump() start, used for all
66-
// message timestamps and stats within the same frame. Eliminates
67-
// repeated QPC syscalls (~20-50ns each, ~12+/frame).
68-
inline uint64_t g_FrameTimestampNs = 0;
65+
// Cached per-frame timestamp for the current thread. Set once at Pump() start,
66+
// used for all same-thread publishes and stats within that pump frame.
67+
inline uint64_t &GetFrameTimestampCache() noexcept {
68+
static thread_local uint64_t frameTimestampNs = 0;
69+
return frameTimestampNs;
70+
}
71+
72+
inline void SetFrameTimestampCache(uint64_t timestamp) noexcept {
73+
GetFrameTimestampCache() = timestamp;
74+
}
6975

7076
inline uint64_t GetTimestampNs() noexcept {
71-
return g_FrameTimestampNs ? g_FrameTimestampNs : GetTimestampNsRaw();
77+
const uint64_t frameTimestampNs = GetFrameTimestampCache();
78+
return frameTimestampNs ? frameTimestampNs : GetTimestampNsRaw();
7279
}
7380

7481
template <typename T, size_t N>

tests/ImcBusTest.cpp

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
#include "Core/Context.h"
2222
#include "Core/ImcBus.h"
23+
#include "Core/ImcBusSharedInternal.h"
2324
#include "TestKernel.h"
2425
#include "TestKernelBuilder.h"
2526
#include "TestModHelper.h"
@@ -1137,6 +1138,13 @@ TEST_F(ImcBusTest, PumpBudgetDistributesLoadAcrossSubscribers) {
11371138
EXPECT_EQ(cap1.count.load(), 1);
11381139
EXPECT_EQ(cap2.count.load(), 1);
11391140

1141+
BML_SubscriptionStats stats1 = BML_SUBSCRIPTION_STATS_INIT;
1142+
BML_SubscriptionStats stats2 = BML_SUBSCRIPTION_STATS_INIT;
1143+
ASSERT_EQ(BML_RESULT_OK, ImcGetSubscriptionStats(sub1, &stats1));
1144+
ASSERT_EQ(BML_RESULT_OK, ImcGetSubscriptionStats(sub2, &stats2));
1145+
EXPECT_EQ(stats1.queue_size, 3u);
1146+
EXPECT_EQ(stats2.queue_size, 3u);
1147+
11401148
// Drain remaining
11411149
Pump(0);
11421150
EXPECT_EQ(cap1.count.load(), 4);
@@ -1146,6 +1154,33 @@ TEST_F(ImcBusTest, PumpBudgetDistributesLoadAcrossSubscribers) {
11461154
EXPECT_EQ(BML_RESULT_OK, ImcUnsubscribe(sub2));
11471155
}
11481156

1157+
TEST(ImcBusSharedInternalTest, FrameTimestampCacheDoesNotLeakAcrossThreads) {
1158+
constexpr uint64_t kCachedTimestamp = 0x123456789ABCDEF0ull;
1159+
1160+
std::atomic<bool> workerReady{false};
1161+
std::atomic<bool> workerRelease{false};
1162+
1163+
std::thread worker([&] {
1164+
BML::Core::SetFrameTimestampCache(kCachedTimestamp);
1165+
workerReady.store(true, std::memory_order_release);
1166+
while (!workerRelease.load(std::memory_order_acquire)) {
1167+
std::this_thread::yield();
1168+
}
1169+
BML::Core::SetFrameTimestampCache(0);
1170+
});
1171+
1172+
while (!workerReady.load(std::memory_order_acquire)) {
1173+
std::this_thread::yield();
1174+
}
1175+
1176+
const uint64_t observedTimestamp = BML::Core::GetTimestampNs();
1177+
1178+
workerRelease.store(true, std::memory_order_release);
1179+
worker.join();
1180+
1181+
EXPECT_NE(observedTimestamp, kCachedTimestamp);
1182+
}
1183+
11491184
TEST_F(ImcBusTest, PublishingResumesAfterPumpClearsQueue) {
11501185
// Backpressure scenario: queue fills up, pump clears, publish succeeds again
11511186
BML_TopicId topic;

0 commit comments

Comments
 (0)