fix: prevent duplicate trace_id after CallTraceHashTable expansion

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,7 +110,7 @@ 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;
 }
 
@@ -178,7 +199,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 +301,20 @@ 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.
+      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 +417,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

@@ -59,6 +59,14 @@ class CallTraceHashTable {
 public:
   static CallTrace _overflow_trace;
 
+  // Pure, allocation-free helpers backing the expansion-overflow guard in
+  // expandTableIfNeeded(); exposed here (rather than kept file-local in the
+  // .cpp) so tests can exercise the 2^32 slot-id boundary and the
+  // capacity-doubling behaviour directly, without needing billions of real
+  // put() calls to reach them.
+  static u64 nextGenerationCapacity(u32 capacity);
+  static bool wouldExceedSlotIdRange(u64 slot_base, u32 capacity);
+
 private:
   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

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(REMOTE_SYMBOLICATION_FRAMES, "remote_symbolication_frames")                \
   X(REMOTE_SYMBOLICATION_LIBS_WITH_BUILD_ID, "remote_symbolication_libs_with_build_id") \

ddprof-lib/src/test/cpp/test_callTraceStorage.cpp

@@ -874,3 +874,178 @@ TEST_F(CallTraceStorageTest, CollectFindsAllTracesAcrossExpandedChain) {
             << "Trace ID " << id << " was lost across expansion boundary";
     }
 }
+
+// PROF-15396 regression tests: trace_id collisions across CallTraceHashTable
+// table-expansion generations.
+//
+// These operate directly on a heap-allocated CallTraceHashTable (matching the
+// pattern used by ConcurrentTableExpansionRegression and
+// PutWithExistingIdNoInfiniteLoopWhenFull above) so that expansion can be
+// forced deterministically via distinct-frame counts alone, without relying
+// on CallTraceStorage::processTraces() rotation.
+//
+// INITIAL_CAPACITY (65536) and LOAD_RATIO (0.75) are documented public
+// behaviour of CallTraceHashTable (expansion triggers once fill exceeds
+// INITIAL_CAPACITY * LOAD_RATIO = 49152 entries); they are re-declared here
+// as shared constants since the class does not expose them as named
+// constants.
+namespace {
+constexpr u32 kInitialCapacity = 65536;
+constexpr double kLoadRatio = 0.75;
+constexpr u32 kExpansionThreshold = static_cast<u32>(kInitialCapacity * kLoadRatio); // 49152
+constexpr u64 kOverflowTraceId = 0x7fffffffffffffffULL;
+
+// Shared setup for the PROF-15396 expansion/trace_id tests below: a
+// heap-allocated CallTraceHashTable, matching the aligned_alloc + placement-new
+// pattern used by ConcurrentTableExpansionRegression and
+// PutWithExistingIdNoInfiniteLoopWhenFull above.
+std::unique_ptr<CallTraceHashTable, void(*)(CallTraceHashTable*)> makeHeapCallTraceHashTable() {
+    void* aligned_memory = std::aligned_alloc(alignof(CallTraceHashTable), sizeof(CallTraceHashTable));
+    if (aligned_memory == nullptr) {
+        return std::unique_ptr<CallTraceHashTable, void(*)(CallTraceHashTable*)>(
+            nullptr, [](CallTraceHashTable*) {});
+    }
+    return std::unique_ptr<CallTraceHashTable, void(*)(CallTraceHashTable*)>(
+        new (aligned_memory) CallTraceHashTable(),
+        [](CallTraceHashTable* ptr) {
+            ptr->~CallTraceHashTable();
+            std::free(ptr);
+        });
+}
+} // namespace
+
+// Test 1: forcing at least one expansion must not produce duplicate
+// trace_ids, and no successful put() may return OVERFLOW_TRACE_ID or
+// DROPPED_TRACE_ID.
+TEST_F(CallTraceStorageTest, ExpansionProducesNoDuplicateTraceIds) {
+    auto hash_table_ptr = makeHeapCallTraceHashTable();
+    ASSERT_NE(hash_table_ptr.get(), nullptr) << "Failed to allocate aligned memory for CallTraceHashTable";
+    CallTraceHashTable& hash_table = *hash_table_ptr;
+    hash_table.setInstanceId(1);
+
+    // Pre-expansion batch: strictly more than the 75% load-ratio threshold,
+    // so at least one expansion is forced deterministically.
+    const u32 PRE_EXPANSION_COUNT = kExpansionThreshold + 100;
+    // Post-expansion batch: further distinct stacks inserted after expansion.
+    const u32 POST_EXPANSION_COUNT = 1000;
+    const u32 TOTAL_COUNT = PRE_EXPANSION_COUNT + POST_EXPANSION_COUNT;
+
+    std::unordered_set<u64> trace_ids;
+    trace_ids.reserve(TOTAL_COUNT);
+
+    for (u32 i = 0; i < TOTAL_COUNT; i++) {
+        ASGCT_CallFrame frame;
+        frame.bci = static_cast<int>(i);
+        frame.method_id = reinterpret_cast<jmethodID>(static_cast<uintptr_t>(i + 1));
+        u64 trace_id = hash_table.put(1, &frame, false, 1);
+        ASSERT_NE(trace_id, kOverflowTraceId) << "Unexpected overflow at i=" << i;
+        ASSERT_NE(trace_id, CallTraceStorage::DROPPED_TRACE_ID) << "Unexpected drop at i=" << i;
+        trace_ids.insert(trace_id);
+    }
+
+    EXPECT_EQ(trace_ids.size(), TOTAL_COUNT)
+        << "Duplicate trace_id detected across pre- and post-expansion inserts combined";
+}
+
+// Test 2: re-inserting an already-known stack after a forced expansion must
+// return exactly the same trace_id it was originally assigned.
+TEST_F(CallTraceStorageTest, TraceIdStableAcrossExpansion) {
+    auto hash_table_ptr = makeHeapCallTraceHashTable();
+    ASSERT_NE(hash_table_ptr.get(), nullptr) << "Failed to allocate aligned memory for CallTraceHashTable";
+    CallTraceHashTable& hash_table = *hash_table_ptr;
+    hash_table.setInstanceId(1);
+
+    // Insert the stack whose trace_id stability we are testing.
+    ASGCT_CallFrame original_frame;
+    original_frame.bci = 999999999;
+    original_frame.method_id = reinterpret_cast<jmethodID>(static_cast<uintptr_t>(0xdeadbeef));
+    u64 original_trace_id = hash_table.put(1, &original_frame, false, 1);
+    ASSERT_GT(original_trace_id, CallTraceStorage::DROPPED_TRACE_ID);
+
+    // Insert enough further distinct stacks to force an expansion.
+    const u32 FILLER_COUNT = kExpansionThreshold + 100;
+    for (u32 i = 0; i < FILLER_COUNT; i++) {
+        ASGCT_CallFrame frame;
+        frame.bci = static_cast<int>(i);
+        frame.method_id = reinterpret_cast<jmethodID>(static_cast<uintptr_t>(i + 1));
+        hash_table.put(1, &frame, false, 1);
+    }
+
+    // Re-insert the original stack; its trace_id must be unchanged.
+    u64 second_trace_id = hash_table.put(1, &original_frame, false, 1);
+    EXPECT_EQ(second_trace_id, original_trace_id)
+        << "trace_id changed for an already-inserted stack after table expansion";
+}
+
+// Test 3: forcing two expansions within a single tenure (i.e. no rotation in
+// between) must still produce unique trace_ids across all three generations
+// combined.
+TEST_F(CallTraceStorageTest, TwoExpansionsWithinOneTenureNoDuplicateTraceIds) {
+    // _size is per-generation and restarts from 0 when a new LongHashTable is
+    // allocated, so the second expansion is gated on the *second* generation's
+    // own load-ratio threshold (0.75 * 2*INITIAL_CAPACITY = 98304 puts served
+    // by that generation), not on the cumulative insert count from i=0.
+    // The first expansion consumes kExpansionThreshold puts on generation 1;
+    // only puts after that land on generation 2, so the total must clear both
+    // thresholds back-to-back for both expansions to occur within one tenure.
+    static constexpr u32 SECOND_GENERATION_THRESHOLD = kExpansionThreshold * 2; // 98304
+
+    auto hash_table_ptr = makeHeapCallTraceHashTable();
+    ASSERT_NE(hash_table_ptr.get(), nullptr) << "Failed to allocate aligned memory for CallTraceHashTable";
+    CallTraceHashTable& hash_table = *hash_table_ptr;
+    hash_table.setInstanceId(1);
+
+    // Insert past both the first generation's and second generation's
+    // load-ratio thresholds, without any rotation (no processTraces()/clear()
+    // call) in between, so both expansions happen within the same tenure.
+    const u32 TOTAL_COUNT = kExpansionThreshold + SECOND_GENERATION_THRESHOLD + 100;
+
+    std::unordered_set<u64> trace_ids;
+    trace_ids.reserve(TOTAL_COUNT);
+
+    for (u32 i = 0; i < TOTAL_COUNT; i++) {
+        ASGCT_CallFrame frame;
+        frame.bci = static_cast<int>(i);
+        frame.method_id = reinterpret_cast<jmethodID>(static_cast<uintptr_t>(i + 1));
+        u64 trace_id = hash_table.put(1, &frame, false, 1);
+        ASSERT_NE(trace_id, kOverflowTraceId) << "Unexpected overflow at i=" << i;
+        ASSERT_NE(trace_id, CallTraceStorage::DROPPED_TRACE_ID) << "Unexpected drop at i=" << i;
+        trace_ids.insert(trace_id);
+    }
+
+    EXPECT_EQ(trace_ids.size(), TOTAL_COUNT)
+        << "Duplicate trace_id detected across two expansions within one tenure";
+}
+
+// Test 4: the expansion-overflow guard (CallTraceHashTable::wouldExceedSlotIdRange)
+// must reject slot_base/capacity combinations that would push slot_base +
+// local_slot past 2^32, accept the exact 2^32 boundary, and accept anything
+// clearly below it. Exercised directly since reaching this boundary via real
+// put() calls would require billions of inserts.
+TEST(CallTraceHashTableOverflowGuardTest, RejectsOnlyValuesThatExceedSlotIdRange) {
+    constexpr u32 kCapacity = 65536;
+    constexpr u64 kSlotIdRange = 0x100000000ull; // 2^32
+
+    // Well below the boundary: base + capacity + next capacity is nowhere
+    // near 2^32.
+    EXPECT_FALSE(CallTraceHashTable::wouldExceedSlotIdRange(0, kCapacity));
+
+    // Exact boundary: base + capacity + next capacity == 2^32 must be
+    // accepted (valid slots are [0, 2^32)).
+    u64 exact_boundary_base = kSlotIdRange - kCapacity - (u64)kCapacity * 2;
+    EXPECT_FALSE(CallTraceHashTable::wouldExceedSlotIdRange(exact_boundary_base, kCapacity));
+
+    // One past the boundary must be rejected.
+    EXPECT_TRUE(CallTraceHashTable::wouldExceedSlotIdRange(exact_boundary_base + 1, kCapacity));
+
+    // Far past the boundary must be rejected.
+    EXPECT_TRUE(CallTraceHashTable::wouldExceedSlotIdRange(kSlotIdRange, kCapacity));
+}
+
+// Test 5: the next-generation capacity used during expansion must be exactly
+// double the current capacity (CallTraceHashTable::nextGenerationCapacity).
+TEST(CallTraceHashTableOverflowGuardTest, NextGenerationCapacityIsDouble) {
+    EXPECT_EQ(CallTraceHashTable::nextGenerationCapacity(65536), 131072ull);
+    EXPECT_EQ(CallTraceHashTable::nextGenerationCapacity(1), 2ull);
+    EXPECT_EQ(CallTraceHashTable::nextGenerationCapacity(0), 0ull);
+}