fix: prevent duplicate trace_id after CallTraceHashTable expansion (#659)

Author: jbachorik

ddprof-lib/src/main/cpp/callTraceHashTable.cpp

@@ -17,6 +17,21 @@
 static const u32 INITIAL_CAPACITY = 65536;  // 64K initial table size (matches upstream)
 static const u32 CALL_TRACE_CHUNK = 8 * 1024 * 1024;
 static const u64 OVERFLOW_TRACE_ID = 0x7fffffffffffffffULL;  // Max 64-bit signed value
+// slot_base + local_slot must stay within a u32 so it never carries into
+// instance_id's bits of trace_id = (instance_id << 32) | (slot_base + slot).
+static const u64 SLOT_ID_RANGE = 0x100000000ull;  // 2^32
+
+// Pure, allocation-free helpers for the expansion-overflow guard below;
+// exposed via the header so tests can exercise the 2^32 slot-id boundary
+// directly instead of needing billions of real put() calls to reach it.
+u64 CallTraceHashTable::nextGenerationCapacity(u32 capacity) {
+  return (u64)capacity * 2;
+}
+
+bool CallTraceHashTable::wouldExceedSlotIdRange(u64 slot_base, u32 capacity) {
+  u64 prospective_base = slot_base + capacity;
+  return prospective_base + nextGenerationCapacity(capacity) > SLOT_ID_RANGE;
+}
 
 // Define the sentinel value for CallTraceSample
 CallTrace* const CallTraceSample::PREPARING = reinterpret_cast<CallTrace*>(1);
@@ -26,7 +41,8 @@ class LongHashTable {
   LongHashTable *_prev;
   void *_padding0;
   u32 _capacity;
-  u32 _padding1[15];
+  u32 _slot_base;
+  u32 _padding1[14];
   volatile u32 _size;
   u32 _padding2[15];
 
@@ -37,22 +53,25 @@ class LongHashTable {
   }
 
 public:
-  LongHashTable(LongHashTable *prev = nullptr, u32 capacity = 0, bool should_clean = true) 
-    : _prev(prev), _padding0(nullptr), _capacity(capacity), _size(0) {
+  LongHashTable(LongHashTable *prev = nullptr, u32 capacity = 0,
+                u32 slot_base = 0, bool should_clean = true)
+    : _prev(prev), _padding0(nullptr), _capacity(capacity),
+      _slot_base(slot_base), _size(0) {
     memset(_padding1, 0, sizeof(_padding1));
     memset(_padding2, 0, sizeof(_padding2));
     if (should_clean) {
       clear();
     }
   }
 
-  static LongHashTable *allocate(LongHashTable *prev, u32 capacity, LinearAllocator* allocator) {
+  static LongHashTable *allocate(LongHashTable *prev, u32 capacity,
+                                  u32 slot_base, LinearAllocator* allocator) {
     void *memory = allocator->alloc(getSize(capacity));
     if (memory != nullptr) {
       // Use placement new to invoke constructor in-place with parameters
       // LinearAllocator doesn't zero memory like OS::safeAlloc with anon mmap
       // so we need to explicitly clear the keys and values (should_clean = true)
-      LongHashTable *table = new (memory) LongHashTable(prev, capacity, true);
+      LongHashTable *table = new (memory) LongHashTable(prev, capacity, slot_base, true);
       return table;
     }
     return nullptr;
@@ -63,6 +82,8 @@ class LongHashTable {
 
   u32 capacity() { return _capacity; }
 
+  u32 slotBase() { return _slot_base; }
+
   u32 size() { return _size; }
 
   u32 incSize() { return __sync_add_and_fetch(&_size, 1); }
@@ -89,10 +110,14 @@ CallTraceHashTable::CallTraceHashTable() : _instance_id(0), _parent_storage(null
   // Instance ID will be set externally via setInstanceId()
 
   // Start with initial capacity, allowing expansion as needed
-  _table = LongHashTable::allocate(nullptr, INITIAL_CAPACITY, &_allocator);
+  _table = LongHashTable::allocate(nullptr, INITIAL_CAPACITY, 0, &_allocator);
   _overflow = 0;
 }
 
+void CallTraceHashTable::seedTableForTesting(u32 slot_base, u32 capacity) {
+  _table = LongHashTable::allocate(nullptr, capacity, slot_base, &_allocator);
+}
+
 CallTraceHashTable::~CallTraceHashTable() {
   // LinearAllocator handles all memory cleanup automatically
   // No need to explicitly destroy tables since they're allocated from LinearAllocator
@@ -178,7 +203,7 @@ ChunkList CallTraceHashTable::clearTableOnly() {
   // RELEASE: pairs with ACQUIRE loads in collect() and put() to ensure the
   // freshly-initialised table is visible on weakly-ordered architectures (aarch64).
   __atomic_store_n(&_table,
-      LongHashTable::allocate(nullptr, INITIAL_CAPACITY, &_allocator),
+      LongHashTable::allocate(nullptr, INITIAL_CAPACITY, 0, &_allocator),
       __ATOMIC_RELEASE);
   _overflow = 0;
 
@@ -280,8 +305,23 @@ void CallTraceHashTable::expandTableIfNeeded(LongHashTable* table, u32 size) {
   // EXPANSION LOGIC: Check if load ratio reached after incrementing size
   if (size >= (u32) (capacity * LOAD_RATIO)  &&
       table == __atomic_load_n(&_table, __ATOMIC_RELAXED)) { // quick check, if other thread already expanded the table
+    if (wouldExceedSlotIdRange(table->slotBase(), capacity)) {
+      // Expanding would push slot_base + local_slot past 2^32, carrying
+      // into instance_id's bit range. Skip expansion; put() keeps working
+      // on the current table at a higher load factor. This degraded state
+      // is bounded, not permanent: the next processTraces() rotation resets
+      // slot_base to 0 via clearTableOnly(), so normal JFR-flush cadence
+      // recovers it.
+      Counters::increment(CALLTRACE_STORAGE_EXPANSION_SKIPPED);
+      return;
+    }
+
+    u64 prospective_base = (u64)table->slotBase() + capacity;
+    u64 prospective_capacity = nextGenerationCapacity(capacity);
+
     // Allocate new table with double capacity using LinearAllocator
-    LongHashTable* new_table = LongHashTable::allocate(table, capacity * 2, &_allocator);
+    LongHashTable* new_table = LongHashTable::allocate(
+        table, (u32)prospective_capacity, (u32)prospective_base, &_allocator);
     if (new_table != nullptr) {
       // Atomic table swap - only one thread succeeds
       __atomic_compare_exchange_n(&_table, &table, new_table, false, __ATOMIC_ACQ_REL, __ATOMIC_RELAXED);
@@ -384,11 +424,13 @@ u64 CallTraceHashTable::put(int num_frames, ASGCT_CallFrame *frames,
       }
 
       if (trace == nullptr) {
-        // Generate unique trace ID: upper 32 bits = instance_id, lower 32 bits = slot
+        // Generate unique trace ID: upper 32 bits = instance_id, lower 32 bits =
+        // slot_base + local slot (slot_base makes the low bits unique across
+        // all LongHashTable generations of one active tenure)
         // ACQUIRE ordering synchronizes with RELEASE store in setInstanceId() to ensure
         // visibility of new instance_id on weakly-ordered architectures (aarch64, POWER)
         u64 instance_id = _instance_id.load(std::memory_order_acquire);
-        u64 trace_id = (instance_id << 32) | slot;
+        u64 trace_id = (instance_id << 32) | (table->slotBase() + slot);
         trace = storeCallTrace(num_frames, frames, truncated, trace_id);
         if (trace == nullptr) {
           // Allocation failure - reset trace first, then clear key

ddprof-lib/src/main/cpp/callTraceHashTable.h

@@ -56,10 +56,24 @@ class CallTraceStorage;
 class CallTraceHashTable {
   static constexpr double LOAD_RATIO = 3.0 / 4.0;
 
+  friend class CallTraceHashTableTestAccessor;
+  // Test-only: grants CallTraceHashTableOverflowGuardTestAccessor
+  // (test_callTraceStorage.cpp) access to the private overflow-guard
+  // helpers below, so CallTraceHashTableOverflowGuardTest can exercise the
+  // 2^32 slot-id boundary and the capacity-doubling behaviour directly,
+  // without needing billions of real put() calls to reach them.
+  friend class CallTraceHashTableOverflowGuardTestAccessor;
+
 public:
   static CallTrace _overflow_trace;
 
 private:
+  // Pure, allocation-free helpers backing the expansion-overflow guard in
+  // expandTableIfNeeded(); kept private and reached in tests only via
+  // CallTraceHashTableOverflowGuardTestAccessor (see friend declaration above).
+  static u64 nextGenerationCapacity(u32 capacity);
+  static bool wouldExceedSlotIdRange(u64 slot_base, u32 capacity);
+
   std::atomic<u64> _instance_id;  // 64-bit instance ID for this hash table - atomic for thread-safe access
   CallTraceStorage* _parent_storage;  // Parent storage for RefCountGuard access
 
@@ -83,6 +97,13 @@ class CallTraceHashTable {
 
   void expandTableIfNeeded(LongHashTable* table, u32 size);
 
+  // Test-only seam: replaces the live table with a synthetic one seeded at
+  // an explicit slot_base/capacity, so gtest can drive expandTableIfNeeded()
+  // through its real call path at the 2^32 slot-id boundary without
+  // billions of real put() calls to get there. Only ever invoked via
+  // CallTraceHashTableTestAccessor (test_callTraceStorage.cpp).
+  void seedTableForTesting(u32 slot_base, u32 capacity);
+
 public:
   CallTraceHashTable();
   ~CallTraceHashTable();

ddprof-lib/src/main/cpp/callTraceStorage.h

@@ -31,7 +31,9 @@ typedef std::function<void(std::unordered_set<u64>&)> LivenessChecker;
 class CallTraceStorage {
 public:
     // Reserved trace ID for dropped samples due to contention
-    // Real trace IDs are generated as (instance_id << 32) | slot, where instance_id starts from 1
+    // Real trace IDs are generated as (instance_id << 32) | (slot_base + slot),
+    // where instance_id starts from 1 and slot_base makes the low bits unique
+    // across all LongHashTable generations of one active tenure
     // Any ID with 0 in upper 32 bits is guaranteed to not clash with real trace IDs
     static constexpr u64 DROPPED_TRACE_ID = 1ULL;
 

ddprof-lib/src/main/cpp/counters.h

@@ -73,6 +73,7 @@
   X(UNWINDING_TIME_ASYNC, "unwinding_ticks_async")                             \
   X(UNWINDING_TIME_JVMTI, "unwinding_ticks_jvmti")                             \
   X(CALLTRACE_STORAGE_DROPPED, "calltrace_storage_dropped_traces")             \
+  X(CALLTRACE_STORAGE_EXPANSION_SKIPPED, "calltrace_storage_expansion_skipped") \
   X(LINE_NUMBER_TABLES, "line_number_tables")                                  \
   X(LINE_NUMBER_TABLE_UNREADABLE, "line_number_table_unreadable")              \
   X(REMOTE_SYMBOLICATION_FRAMES, "remote_symbolication_frames")                \