diff --git a/ddprof-lib/src/main/cpp/callTraceHashTable.cpp b/ddprof-lib/src/main/cpp/callTraceHashTable.cpp index cb0d7e608..fa344d4e4 100644 --- a/ddprof-lib/src/main/cpp/callTraceHashTable.cpp +++ b/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(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,8 +53,10 @@ 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) { @@ -46,13 +64,14 @@ class LongHashTable { } } - 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 diff --git a/ddprof-lib/src/main/cpp/callTraceHashTable.h b/ddprof-lib/src/main/cpp/callTraceHashTable.h index 351369015..5fab0d93e 100644 --- a/ddprof-lib/src/main/cpp/callTraceHashTable.h +++ b/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 _instance_id; // 64-bit instance ID for this hash table - atomic for thread-safe access CallTraceStorage* _parent_storage; // Parent storage for RefCountGuard access diff --git a/ddprof-lib/src/main/cpp/callTraceStorage.h b/ddprof-lib/src/main/cpp/callTraceStorage.h index 9be8cb038..5d1563e65 100644 --- a/ddprof-lib/src/main/cpp/callTraceStorage.h +++ b/ddprof-lib/src/main/cpp/callTraceStorage.h @@ -31,7 +31,9 @@ typedef std::function&)> 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; diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index 4ef2d5ae1..6d71315d6 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/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") \ diff --git a/ddprof-lib/src/test/cpp/test_callTraceStorage.cpp b/ddprof-lib/src/test/cpp/test_callTraceStorage.cpp index 2b2c96b9b..f303e0775 100644 --- a/ddprof-lib/src/test/cpp/test_callTraceStorage.cpp +++ b/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(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 makeHeapCallTraceHashTable() { + void* aligned_memory = std::aligned_alloc(alignof(CallTraceHashTable), sizeof(CallTraceHashTable)); + if (aligned_memory == nullptr) { + return std::unique_ptr( + nullptr, [](CallTraceHashTable*) {}); + } + return std::unique_ptr( + 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 trace_ids; + trace_ids.reserve(TOTAL_COUNT); + + for (u32 i = 0; i < TOTAL_COUNT; i++) { + ASGCT_CallFrame frame; + frame.bci = static_cast(i); + frame.method_id = reinterpret_cast(static_cast(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(static_cast(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(i); + frame.method_id = reinterpret_cast(static_cast(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 trace_ids; + trace_ids.reserve(TOTAL_COUNT); + + for (u32 i = 0; i < TOTAL_COUNT; i++) { + ASGCT_CallFrame frame; + frame.bci = static_cast(i); + frame.method_id = reinterpret_cast(static_cast(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); +}