Skip to content

Commit 4847685

Browse files
authored
fix(profiler): lock-free class/endpoint/context maps via StringDictionary (#524)
1 parent 57d301f commit 4847685

31 files changed

Lines changed: 3052 additions & 963 deletions

.github/workflows/ci.yml

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ jobs:
128128
if: needs.check-for-pr.outputs.skip != 'true'
129129
outputs:
130130
configurations: ${{ steps.compute.outputs.configurations }}
131+
run_fuzz: ${{ steps.compute.outputs.run_fuzz }}
131132
steps:
132133
- name: Debounce label events
133134
if: github.event.action == 'labeled'
@@ -155,6 +156,13 @@ jobs:
155156
if echo "$labels" | grep -Fq "test:tsan"; then
156157
configs="$configs"',"tsan"'
157158
fi
159+
if echo "$labels" | grep -Fq "test:fuzz"; then
160+
echo "run_fuzz=true" >> $GITHUB_OUTPUT
161+
else
162+
echo "run_fuzz=false" >> $GITHUB_OUTPUT
163+
fi
164+
else
165+
echo "run_fuzz=false" >> $GITHUB_OUTPUT
158166
fi
159167
160168
configs="$configs]"
@@ -194,3 +202,37 @@ jobs:
194202
body-file: test-summary.md
195203
comment-id: ci-test-results
196204

205+
fuzz:
206+
needs: [check-for-pr, compute-configurations]
207+
if: needs.check-for-pr.outputs.skip != 'true' && needs.compute-configurations.outputs.run_fuzz == 'true'
208+
runs-on: ubuntu-latest
209+
continue-on-error: true
210+
timeout-minutes: 30
211+
steps:
212+
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
213+
- name: Cache Gradle Wrapper Binaries
214+
uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5
215+
with:
216+
path: ~/.gradle/wrapper/dists
217+
key: gradle-wrapper-${{ runner.os }}-${{ hashFiles('gradle/wrapper/gradle-wrapper.properties') }}
218+
restore-keys: |
219+
gradle-wrapper-${{ runner.os }}-
220+
- name: Cache Gradle User Home
221+
uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5
222+
with:
223+
path: ~/.gradle/caches
224+
key: gradle-caches-${{ runner.os }}-${{ hashFiles('**/*.gradle*', '**/gradle-wrapper.properties') }}
225+
restore-keys: |
226+
gradle-caches-${{ runner.os }}-
227+
- name: Setup OS
228+
run: |
229+
sudo apt-get update
230+
sudo apt-get install -y clang
231+
- name: Fuzz
232+
run: ./gradlew :ddprof-lib:fuzz:fuzz -Pfuzz-duration=120 --no-daemon
233+
- name: Upload crash artifacts
234+
if: failure()
235+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
236+
with:
237+
name: fuzz-crashes
238+
path: ddprof-lib/fuzz/build/fuzz-crashes/

.github/workflows/nightly.yml

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,38 @@ jobs:
1818
# C++ gtests (ASan + TSan) run on every PR via native-sanitizer-tests in ci.yml.
1919
# Skip them here so the nightly focuses on Java functional tests under ASan.
2020
skip_gtest: true
21+
fuzz:
22+
runs-on: ubuntu-latest
23+
continue-on-error: true
24+
timeout-minutes: 30
25+
steps:
26+
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
27+
- name: Cache Gradle Wrapper Binaries
28+
uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5
29+
with:
30+
path: ~/.gradle/wrapper/dists
31+
key: gradle-wrapper-${{ runner.os }}-${{ hashFiles('gradle/wrapper/gradle-wrapper.properties') }}
32+
restore-keys: |
33+
gradle-wrapper-${{ runner.os }}-
34+
- name: Cache Gradle User Home
35+
uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5
36+
with:
37+
path: ~/.gradle/caches
38+
key: gradle-caches-${{ runner.os }}-${{ hashFiles('**/*.gradle*', '**/gradle-wrapper.properties') }}
39+
restore-keys: |
40+
gradle-caches-${{ runner.os }}-
41+
- name: Setup OS
42+
run: |
43+
sudo apt-get update
44+
sudo apt-get install -y clang
45+
- name: Fuzz
46+
run: ./gradlew :ddprof-lib:fuzz:fuzz -Pfuzz-duration=120 --no-daemon
47+
- name: Upload crash artifacts
48+
if: failure()
49+
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
50+
with:
51+
name: fuzz-crashes
52+
path: ddprof-lib/fuzz/build/fuzz-crashes/
2153
report-failures:
2254
runs-on: ubuntu-latest
2355
needs: run-test

build-logic/conventions/src/main/kotlin/com/datadoghq/native/fuzz/FuzzTargetsPlugin.kt

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ class FuzzTargetsPlugin : Plugin<Project> {
8686
val includeFiles = buildIncludePaths(project, extension, homebrewLLVM)
8787

8888
// Build compiler/linker args
89-
val compilerArgs = buildFuzzCompilerArgs()
89+
val compilerArgs = buildFuzzCompilerArgs(project)
9090
val linkerArgs = buildFuzzLinkerArgs(homebrewLLVM, clangResourceDir, project.logger)
9191

9292
val fuzzSourceDir = extension.fuzzSourceDir.get().asFile
@@ -194,15 +194,17 @@ class FuzzTargetsPlugin : Plugin<Project> {
194194
return includes
195195
}
196196

197-
private fun buildFuzzCompilerArgs(): List<String> {
197+
private fun buildFuzzCompilerArgs(project: Project): List<String> {
198+
val version = project.version.toString()
198199
val args = mutableListOf(
199200
"-O1",
200201
"-g",
201202
"-fno-omit-frame-pointer",
202203
"-fsanitize=fuzzer,address,undefined",
203204
"-fvisibility=hidden",
204205
"-std=c++17",
205-
"-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION"
206+
"-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION",
207+
"-DPROFILER_VERSION=\"$version\""
206208
)
207209
if (PlatformUtils.currentPlatform == Platform.LINUX && PlatformUtils.isMusl()) {
208210
args.add("-D__musl__")

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

Lines changed: 1 addition & 225 deletions
Original file line numberDiff line numberDiff line change
@@ -6,240 +6,16 @@
66

77
#include "callTraceStorage.h"
88
#include "counters.h"
9+
#include "log.h"
910
#include "os.h"
1011
#include "common.h"
1112
#include "thread.h"
1213
#include "vmEntry.h" // For BCI_ERROR constant
1314
#include "arch.h" // For LP64_ONLY macro and COMMA macro
1415
#include "guards.h" // For table swap critical sections
15-
#include "primeProbing.h"
1616
#include "thread.h"
1717
#include <string.h>
1818
#include <atomic>
19-
#include <time.h>
20-
21-
// RefCountGuard static members
22-
RefCountSlot RefCountGuard::refcount_slots[RefCountGuard::MAX_THREADS];
23-
int RefCountGuard::slot_owners[RefCountGuard::MAX_THREADS];
24-
25-
26-
// RefCountGuard implementation
27-
int RefCountGuard::getThreadRefCountSlot() {
28-
// Signal-safe collision resolution: use OS::threadId() with semi-random prime step probing
29-
ProfiledThread* thrd = ProfiledThread::currentSignalSafe();
30-
int tid = thrd != nullptr ? thrd->tid() : OS::threadId();
31-
32-
// Semi-random prime step probing to eliminate secondary clustering
33-
HashProbe probe(static_cast<u64>(tid), MAX_THREADS);
34-
35-
int slot = probe.slot();
36-
for (int i = 0; i < MAX_PROBE_DISTANCE; i++) {
37-
// Try to claim this slot atomically
38-
int expected = 0; // Empty slot (no thread ID)
39-
if (__atomic_compare_exchange_n(&slot_owners[slot], &expected, tid, false, __ATOMIC_ACQ_REL, __ATOMIC_RELAXED)) {
40-
// Successfully claimed the slot
41-
return slot;
42-
}
43-
44-
// Check if we already own this slot (for reentrant calls)
45-
if (__atomic_load_n(&slot_owners[slot], __ATOMIC_ACQUIRE) == tid) {
46-
return slot;
47-
}
48-
49-
// Move to next slot using probe
50-
if (probe.hasNext()) {
51-
slot = probe.next();
52-
}
53-
}
54-
55-
// All probing attempts failed - return -1 to indicate failure
56-
return -1;
57-
}
58-
59-
RefCountGuard::RefCountGuard(CallTraceHashTable* resource) : _active(true), _my_slot(-1) {
60-
// Get thread refcount slot using signal-safe collision resolution
61-
_my_slot = getThreadRefCountSlot();
62-
63-
if (_my_slot == -1) {
64-
// Slot allocation failed - refcount guard is inactive
65-
_active = false;
66-
return;
67-
}
68-
69-
// CRITICAL ORDERING: Store pointer FIRST, then increment count
70-
// This ensures the pointer-first protocol for race-free operation
71-
//
72-
// Why this ordering is safe:
73-
// Between step 1 and 2, if scanner runs:
74-
// - Scanner loads count=0 (not yet incremented)
75-
// - Scanner sees slot as inactive, skips it
76-
// - Safe: we haven't "activated" protection yet
77-
//
78-
// After step 2, slot is fully active and protects the resource
79-
__atomic_store_n(&refcount_slots[_my_slot].active_table, resource, __ATOMIC_RELEASE);
80-
__atomic_fetch_add(&refcount_slots[_my_slot].count, 1, __ATOMIC_RELEASE);
81-
}
82-
83-
RefCountGuard::~RefCountGuard() {
84-
if (_active && _my_slot >= 0) {
85-
// CRITICAL ORDERING: Decrement count FIRST, then clear pointer
86-
// This ensures safe deactivation
87-
//
88-
// Why this ordering is safe:
89-
// After step 1, count=0 so scanner will skip this slot
90-
// Step 2 clears the pointer (cleanup)
91-
// No window where scanner thinks slot protects a table it doesn't
92-
__atomic_fetch_sub(&refcount_slots[_my_slot].count, 1, __ATOMIC_RELEASE);
93-
__atomic_store_n(&refcount_slots[_my_slot].active_table, nullptr, __ATOMIC_RELEASE);
94-
95-
// Release slot ownership
96-
__atomic_store_n(&slot_owners[_my_slot], 0, __ATOMIC_RELEASE);
97-
}
98-
}
99-
100-
RefCountGuard::RefCountGuard(RefCountGuard&& other) noexcept : _active(other._active), _my_slot(other._my_slot) {
101-
other._active = false;
102-
}
103-
104-
RefCountGuard& RefCountGuard::operator=(RefCountGuard&& other) noexcept {
105-
if (this != &other) {
106-
// Clean up current state with same ordering as destructor
107-
if (_active && _my_slot >= 0) {
108-
__atomic_fetch_sub(&refcount_slots[_my_slot].count, 1, __ATOMIC_RELEASE);
109-
__atomic_store_n(&refcount_slots[_my_slot].active_table, nullptr, __ATOMIC_RELEASE);
110-
__atomic_store_n(&slot_owners[_my_slot], 0, __ATOMIC_RELEASE);
111-
}
112-
113-
// Move from other
114-
_active = other._active;
115-
_my_slot = other._my_slot;
116-
117-
// Clear other
118-
other._active = false;
119-
}
120-
return *this;
121-
}
122-
123-
void RefCountGuard::waitForRefCountToClear(CallTraceHashTable* table_to_delete) {
124-
// Check refcount slots for the table we want to delete
125-
//
126-
// POINTER-FIRST PROTOCOL GUARANTEES:
127-
// - Constructor stores pointer then increments count
128-
// - Destructor decrements count then clears pointer
129-
// - Scanner checks count first (if 0, slot is inactive)
130-
//
131-
// TRACE DROP WINDOW (intentional design):
132-
// - Scanner can complete on FIRST iteration if all slots have count=0
133-
// - Guards in construction (pointer stored, count still 0) are treated as inactive
134-
// - Revalidation check in put() detects this race and drops the trace
135-
// - This trades a narrow trace-drop window (~10-100ns) for protocol simplicity
136-
// - USE-AFTER-FREE IS IMPOSSIBLE: Revalidation prevents table access after deletion
137-
138-
// PHASE 1: Fast path - spin with pause for short waits (common case)
139-
// Expected: refcounts clear within 1-20µs as put() operations complete
140-
const int SPIN_ITERATIONS = 100;
141-
for (int spin = 0; spin < SPIN_ITERATIONS; ++spin) {
142-
bool all_clear = true;
143-
144-
// Scan all slots (no bitmap optimization, but simpler logic)
145-
for (int i = 0; i < MAX_THREADS; ++i) {
146-
// CRITICAL: Check count FIRST (pointer-first protocol)
147-
uint32_t count = __atomic_load_n(&refcount_slots[i].count, __ATOMIC_ACQUIRE);
148-
if (count == 0) {
149-
continue; // Slot inactive, skip it
150-
}
151-
152-
// Count > 0, so slot is active - check which table it protects
153-
CallTraceHashTable* table = __atomic_load_n(&refcount_slots[i].active_table, __ATOMIC_ACQUIRE);
154-
if (table == table_to_delete) {
155-
all_clear = false;
156-
break;
157-
}
158-
}
159-
160-
if (all_clear) {
161-
return; // Fast path success - refcounts cleared quickly
162-
}
163-
spinPause(); // CPU pause instruction, ~10-50 cycles
164-
}
165-
166-
// PHASE 2: Slow path - async-signal-safe sleep for blocked thread case
167-
const int MAX_WAIT_ITERATIONS = 5000;
168-
struct timespec sleep_time = {0, 100000}; // 100 microseconds
169-
170-
for (int wait_count = 0; wait_count < MAX_WAIT_ITERATIONS; ++wait_count) {
171-
bool all_clear = true;
172-
173-
for (int i = 0; i < MAX_THREADS; ++i) {
174-
uint32_t count = __atomic_load_n(&refcount_slots[i].count, __ATOMIC_ACQUIRE);
175-
if (count == 0) {
176-
continue;
177-
}
178-
179-
CallTraceHashTable* table = __atomic_load_n(&refcount_slots[i].active_table, __ATOMIC_ACQUIRE);
180-
if (table == table_to_delete) {
181-
all_clear = false;
182-
break;
183-
}
184-
}
185-
186-
if (all_clear) {
187-
return; // Slow path success
188-
}
189-
190-
// nanosleep is POSIX async-signal-safe and does not call malloc
191-
nanosleep(&sleep_time, nullptr);
192-
}
193-
194-
// If we reach here, some refcounts didn't clear in time
195-
// This shouldn't happen in normal operation but we log it for debugging
196-
}
197-
198-
void RefCountGuard::waitForAllRefCountsToClear() {
199-
// PHASE 1: Fast path - spin with pause for short waits
200-
const int SPIN_ITERATIONS = 100;
201-
for (int spin = 0; spin < SPIN_ITERATIONS; ++spin) {
202-
bool any_refcounts = false;
203-
204-
for (int i = 0; i < MAX_THREADS; ++i) {
205-
uint32_t count = __atomic_load_n(&refcount_slots[i].count, __ATOMIC_ACQUIRE);
206-
if (count > 0) {
207-
any_refcounts = true;
208-
break;
209-
}
210-
}
211-
212-
if (!any_refcounts) {
213-
return; // Fast path success
214-
}
215-
spinPause();
216-
}
217-
218-
// PHASE 2: Slow path - async-signal-safe sleep
219-
const int MAX_WAIT_ITERATIONS = 5000;
220-
struct timespec sleep_time = {0, 100000}; // 100 microseconds
221-
222-
for (int wait_count = 0; wait_count < MAX_WAIT_ITERATIONS; ++wait_count) {
223-
bool any_refcounts = false;
224-
225-
for (int i = 0; i < MAX_THREADS; ++i) {
226-
uint32_t count = __atomic_load_n(&refcount_slots[i].count, __ATOMIC_ACQUIRE);
227-
if (count > 0) {
228-
any_refcounts = true;
229-
break;
230-
}
231-
}
232-
233-
if (!any_refcounts) {
234-
return; // Slow path success
235-
}
236-
237-
nanosleep(&sleep_time, nullptr);
238-
}
239-
240-
// If we reach here, some refcounts didn't clear in time
241-
}
242-
24319

24420
static const u64 OVERFLOW_TRACE_ID = 0x7fffffffffffffffULL; // Max 64-bit signed value
24521

0 commit comments

Comments
 (0)