Skip to content

Commit 2a8c230

Browse files
jbachorikjbachorik
committed
docs: clarify rotate() comment — JNI callers can insert during rotate
1 parent 5afc337 commit 2a8c230

1 file changed

Lines changed: 159 additions & 79 deletions

File tree

‎ddprof-lib/src/main/cpp/stringDictionary.h‎

Lines changed: 159 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -37,30 +37,122 @@ struct SBTable {
3737
SBRow rows[ROWS];
3838
};
3939

40+
// ─── StringArena ──────────────────────────────────────────────────────────
41+
//
42+
// Auto-growing bump allocator for key strings inside StringDictionaryBuffer.
43+
//
44+
// Memory is organised as a linked list of 512 KB chunks. alloc() is a single
45+
// atomic fetch_add on the current chunk — fully contention-free as long as
46+
// the chunk is not full. When a chunk fills up, grow() serialises creation of
47+
// the next chunk via a CAS spinlock; contention here is extremely rare.
48+
//
49+
// Threads that lose a CAS race in insert_with_id leave their arena allocation
50+
// as waste; space is recovered on the next reset().
51+
//
52+
// reset() frees all chunks after the first and resets the first chunk's
53+
// position counter. The first chunk is kept to avoid a malloc on the next
54+
// use. reset() must only be called when no concurrent alloc() calls are in
55+
// flight.
56+
class StringArena {
57+
static constexpr size_t CHUNK_SIZE = 512 * 1024;
58+
59+
// Plain struct allocated via calloc (zero-initialised); pos accessed via
60+
// __atomic builtins, consistent with the rest of the file.
61+
struct Chunk {
62+
Chunk* next; // singly-linked for traversal in reset() / ~StringArena()
63+
size_t pos; // bump pointer within data[]
64+
char data[CHUNK_SIZE];
65+
};
66+
67+
Chunk* _first; // head of chain; kept across resets
68+
std::atomic<Chunk*> _active; // current allocation target
69+
std::atomic<bool> _growing{false}; // serialises new-chunk creation
70+
71+
static Chunk* make_chunk() {
72+
return static_cast<Chunk*>(calloc(1, sizeof(Chunk)));
73+
}
74+
75+
void grow(Chunk* full) {
76+
// One thread at a time creates the next chunk. Others spin briefly
77+
// then re-check _active; if it has already advanced they return.
78+
bool expected = false;
79+
while (!_growing.compare_exchange_weak(expected, true,
80+
std::memory_order_acquire, std::memory_order_relaxed)) {
81+
if (_active.load(std::memory_order_relaxed) != full) return;
82+
expected = false;
83+
spinPause();
84+
}
85+
if (_active.load(std::memory_order_relaxed) != full) {
86+
_growing.store(false, std::memory_order_release);
87+
return;
88+
}
89+
Chunk* fresh = make_chunk();
90+
// On OOM store nullptr so alloc() returns nullptr instead of spinning.
91+
_active.store(fresh, std::memory_order_release);
92+
if (fresh) full->next = fresh; // link into chain for reset() traversal
93+
_growing.store(false, std::memory_order_release);
94+
}
95+
96+
public:
97+
StringArena() : _first(make_chunk()), _active(_first) {}
98+
99+
~StringArena() {
100+
Chunk* c = _first;
101+
while (c) { Chunk* n = c->next; free(c); c = n; }
102+
}
103+
104+
StringArena(const StringArena&) = delete;
105+
StringArena& operator=(const StringArena&) = delete;
106+
107+
char* alloc(size_t n) {
108+
n = (n + alignof(void*) - 1) & ~(alignof(void*) - 1);
109+
for (;;) {
110+
Chunk* c = _active.load(std::memory_order_acquire);
111+
if (!c) return nullptr; // OOM
112+
size_t off = __atomic_fetch_add(&c->pos, n, __ATOMIC_RELAXED);
113+
if (off + n <= CHUNK_SIZE) return c->data + off;
114+
grow(c);
115+
}
116+
}
117+
118+
// Free all chunks after the first; reset the first.
119+
// O(extra_chunks). Also clears the OOM state: if alloc() had returned
120+
// nullptr after a failed make_chunk(), the next alloc() after reset()
121+
// will succeed again (up to one chunk's worth).
122+
void reset() {
123+
Chunk* c = _first ? _first->next : nullptr;
124+
while (c) { Chunk* n = c->next; free(c); c = n; }
125+
if (_first) {
126+
_first->next = nullptr;
127+
__atomic_store_n(&_first->pos, (size_t)0, __ATOMIC_RELAXED);
128+
}
129+
_active.store(_first, std::memory_order_release);
130+
}
131+
};
132+
40133
// ─── StringDictionaryBuffer ────────────────────────────────────────────────
41134
//
42135
// Open-addressing concurrent hash table mapping string keys to u32 IDs.
43136
//
137+
// Key strings are owned by the per-buffer StringArena. Overflow SBTable
138+
// nodes are heap-allocated (calloc) and freed by freeOverflowNodes() on
139+
// clear() and destruction. This makes clear() O(number-of-overflow-nodes)
140+
// rather than O(number-of-entries), and eliminates per-key malloc/free.
141+
//
44142
// Concurrency model:
45-
// - Inserts (insert_with_id, copyFrom): use __sync_bool_compare_and_swap on
46-
// keys[c] (full fence on GCC) to claim a slot. id and keylen are stored
47-
// AFTER the CAS wins (release); a reader that observes keys[c] != null via
48-
// an acquire load must also check ids[c] != 0 before using it. On x86 the
49-
// CAS already implies a full memory barrier; on ARM the acquire/release
50-
// pairing provides the required ordering. id remains 0 until published.
51-
// - Reads (lookup): acquire-load keys[c]; if non-null and key matches, return
52-
// the id — only if ids[c] != 0 (not yet published = miss). Null means
53-
// "slot unclaimed" — treated as miss. There is a narrow window where a
54-
// inserter has started but not yet committed the CAS; this is safe (returns
55-
// miss = 0), identical to the existing Dictionary behaviour.
143+
// - Inserts (insert_with_id, copyFrom): CAS on keys[c] to claim a slot.
144+
// id stored AFTER winning CAS (release); readers check ids[c] != 0.
145+
// Losers of the CAS leave their arena allocation as recoverable waste.
146+
// - Reads (lookup): acquire-load keys[c]; miss on null or unpublished id.
56147
// - clear(): called only when no concurrent readers/writers are active.
57148
//
58-
// Not signal-safe for insert_with_id / copyFrom (call malloc).
149+
// Not signal-safe for insert_with_id / copyFrom (arena alloc + calloc).
59150
// Signal-safe for lookup (read-only, no allocation).
60151
class StringDictionaryBuffer {
61152
private:
62153
SBTable* _table;
63154
std::atomic<int> _size{0};
155+
StringArena _arena;
64156

65157
static unsigned int hash(const char* key, size_t length) {
66158
unsigned int h = 2166136261U;
@@ -72,21 +164,9 @@ class StringDictionaryBuffer {
72164
return strncmp(candidate, key, length) == 0 && candidate[length] == '\0';
73165
}
74166

75-
// Iterative DFS walk of the overflow tree. Each frame tracks one table and
76-
// the next row to visit. Because we descend into row->next immediately and
77-
// only pop the frame after every row has been processed, the stack holds at
78-
// most one frame per overflow-chain level. The hash rotation has period
79-
// 32 / gcd(32, ROW_BITS) = 32: after 32 levels the row index cycles back to
80-
// the original. In practice, reaching level N requires N*CELLS keys with
81-
// identical 32-bit FNV hashes (gcd(7,32)=1 means any two keys that share
82-
// the same row index across all 32 rotation steps must hash identically).
83-
// A chain longer than 33 levels therefore requires 99+ exact FNV collisions
84-
// — statistically negligible for real-world string inputs. stk[34] covers
85-
// this practical bound; the top < 34 guard is safety-overflow protection.
86-
// If it ever fires, entries beyond level 33 are silently dropped rather than
87-
// crashing — acceptable given how the bound is reached only by adversarial
88-
// or degenerate inputs.
89-
static void freeTable(SBTable* table) {
167+
// Free only overflow SBTable chain nodes (not key strings — arena-owned).
168+
// Same DFS traversal as the old freeTable but without the per-key free.
169+
static void freeOverflowNodes(SBTable* table) {
90170
struct Frame { SBTable* t; int row; };
91171
Frame stk[34];
92172
int top = 0;
@@ -99,9 +179,6 @@ class StringDictionaryBuffer {
99179
continue;
100180
}
101181
SBRow* row = &f.t->rows[f.row++];
102-
for (int j = 0; j < CELLS; j++) {
103-
if (row->keys[j]) free(row->keys[j]);
104-
}
105182
if (row->next && top < 34) stk[top++] = {row->next, 0};
106183
}
107184
}
@@ -130,15 +207,22 @@ class StringDictionaryBuffer {
130207

131208
public:
132209
StringDictionaryBuffer() {
133-
_table = (SBTable*)calloc(1, sizeof(SBTable));
210+
_table = static_cast<SBTable*>(calloc(1, sizeof(SBTable)));
134211
}
135212

136213
~StringDictionaryBuffer() {
137-
if (_table != nullptr) { freeTable(_table); }
138-
free(_table);
139-
_table = nullptr;
214+
if (_table != nullptr) {
215+
freeOverflowNodes(_table);
216+
free(_table);
217+
_table = nullptr;
218+
}
140219
}
141220

221+
StringDictionaryBuffer(const StringDictionaryBuffer&) = delete;
222+
StringDictionaryBuffer& operator=(const StringDictionaryBuffer&) = delete;
223+
StringDictionaryBuffer(StringDictionaryBuffer&&) = delete;
224+
StringDictionaryBuffer& operator=(StringDictionaryBuffer&&) = delete;
225+
142226
// Signal-safe read-only probe. Returns 0 on miss.
143227
u32 lookup(const char* key, size_t len) const {
144228
const SBTable* table = _table;
@@ -159,9 +243,8 @@ class StringDictionaryBuffer {
159243
return 0;
160244
}
161245

162-
// Insert with the given id. Returns the id stored for this key
163-
// (either the given id on a new slot, or the existing id on a duplicate).
164-
// NOT signal-safe (calls malloc).
246+
// Insert with the given id. Returns the id stored for this key.
247+
// NOT signal-safe (arena alloc; calloc for overflow nodes).
165248
u32 insert_with_id(const char* key, size_t len, u32 id) {
166249
SBTable* table = _table;
167250
unsigned int h = hash(key, len);
@@ -170,7 +253,7 @@ class StringDictionaryBuffer {
170253
for (int c = 0; c < CELLS; c++) {
171254
char* existing = __atomic_load_n(&row->keys[c], __ATOMIC_ACQUIRE);
172255
if (!existing) {
173-
char* new_key = (char*)malloc(len + 1);
256+
char* new_key = _arena.alloc(len + 1);
174257
if (!new_key) return 0;
175258
memcpy(new_key, key, len);
176259
new_key[len] = '\0';
@@ -179,7 +262,7 @@ class StringDictionaryBuffer {
179262
_size.fetch_add(1, std::memory_order_relaxed);
180263
return id;
181264
}
182-
free(new_key);
265+
// CAS lost — new_key is arena waste, recovered on clear().
183266
existing = __atomic_load_n(&row->keys[c], __ATOMIC_ACQUIRE);
184267
}
185268
if (existing && keyEquals(existing, key, len)) {
@@ -189,7 +272,7 @@ class StringDictionaryBuffer {
189272
}
190273
}
191274
if (!row->next) {
192-
SBTable* nt = (SBTable*)calloc(1, sizeof(SBTable));
275+
SBTable* nt = static_cast<SBTable*>(calloc(1, sizeof(SBTable)));
193276
if (nt == nullptr) return 0;
194277
if (!__sync_bool_compare_and_swap(&row->next, nullptr, nt)) free(nt);
195278
}
@@ -213,11 +296,13 @@ class StringDictionaryBuffer {
213296
collectTable(_table, out);
214297
}
215298

216-
// Free all keys and reset to empty. Call only with no concurrent accessors.
299+
// Free overflow nodes, zero the root table, reset the arena.
300+
// Call only with no concurrent accessors.
217301
void clear() {
218302
if (_table == nullptr) { _size.store(0, std::memory_order_relaxed); return; }
219-
freeTable(_table);
303+
freeOverflowNodes(_table);
220304
memset(_table, 0, sizeof(SBTable));
305+
_arena.reset();
221306
_size.store(0, std::memory_order_relaxed);
222307
}
223308

@@ -238,14 +323,20 @@ class StringDictionaryBuffer {
238323
// concurrent inserts in the rotation window:
239324
// phase 1: copy active → clearTarget (before rotate)
240325
// phase 2: copy old_active → new_active (after drain, catch late inserts)
241-
// lookupDuringDump(key): probes dump then active; inserts into both if new,
242-
// using the existing global ID (or a new fetch-add if truly new everywhere).
326+
// lookupDuringDump(key): probes dump then active; inserts into both if new.
243327
//
244-
// Signal safety:
328+
// Concurrency:
245329
// bounded_lookup acquires RefCountGuard on active before reading.
246330
// lookup also acquires RefCountGuard before inserting (not signal-safe due to
247-
// malloc, but the guard protects the buffer pointer lifetime).
331+
// arena alloc, but the guard protects the buffer pointer lifetime).
248332
// lookupDuringDump is NOT signal-safe; call from dump thread only.
333+
//
334+
// _accepting gates new RefCountGuard creation during clearAll(). The
335+
// key-string arena makes the TOCTOU window between the _accepting acquire-
336+
// load and the RefCountGuard count++ benign: even if clearAll()'s drain
337+
// misses a racing caller, that caller reads from the zeroed root table and
338+
// returns 0. The seq_cst recheck previously needed to close this window
339+
// is therefore unnecessary and has been removed from the hot path.
249340
class StringDictionary {
250341
std::atomic<u32> _next_id{1}; // starts at 1; id=0 reserved as "no entry"
251342
std::atomic<bool> _accepting{true}; // false while clearAll() is resetting buffers
@@ -268,8 +359,6 @@ class StringDictionary {
268359

269360
// Insert into active buffer; returns globally stable id. NOT signal-safe.
270361
u32 lookup(const char* key, size_t len) {
271-
// Bail immediately if clearAll() is resetting the buffers; creating a
272-
// RefCountGuard here could race with freeTable() inside clear().
273362
if (!_accepting.load(std::memory_order_acquire)) return 0;
274363
while (true) {
275364
StringDictionaryBuffer* active = _rot.active();
@@ -278,8 +367,6 @@ class StringDictionary {
278367
if (_rot.active() != active) continue;
279368
u32 id = active->lookup(key, len);
280369
if (id != 0) return id;
281-
// nextId() may be consumed without assignment if a concurrent insert wins
282-
// the CAS for the same key; IDs are unique but not guaranteed to be dense.
283370
u32 new_id = nextId();
284371
u32 result = active->insert_with_id(key, len, new_id);
285372
if (result == new_id) countInsert(len);
@@ -290,8 +377,6 @@ class StringDictionary {
290377
// Insert into active buffer if size < size_limit; returns 0 when at cap.
291378
// NOT signal-safe.
292379
u32 bounded_lookup(const char* key, size_t len, int size_limit) {
293-
// Bail immediately if clearAll() is resetting the buffers; creating a
294-
// RefCountGuard here could race with freeTable() inside clear().
295380
if (!_accepting.load(std::memory_order_acquire)) return 0;
296381
while (true) {
297382
StringDictionaryBuffer* active = _rot.active();
@@ -326,32 +411,31 @@ class StringDictionary {
326411
}
327412

328413
// Two-phase ID-preserving rotate.
329-
// Invariant: all insert paths are blocked for the duration of rotate():
330-
// - Signal-path lookup() callers are blocked by SignalBlocker at the
331-
// rotateDictsAndRun call site, which masks SIGPROF and SIGVTALRM
332-
// (the profiling signals) on the calling thread before rotate() is
333-
// invoked.
334-
// - JNI recording methods (e.g. recordTraceRoot) call
335-
// _locks[lock_index].tryLock() at the Profiler level before reaching
336-
// bounded_lookup(); lockAll() holds all _locks[], so those tryLock()
337-
// calls fail and the recording method returns without ever calling
338-
// bounded_lookup().
339-
// - lookupDuringDump() is only called on the dump thread, which is the
340-
// same thread executing rotateDictsAndRun() — no concurrency possible.
341-
// clearTarget() returns the scratch buffer that becomes the new active
342-
// after rotate(). It is always empty on entry because clearStandby()
343-
// is called after each cycle in rotateDictsAndRun() and JFR operations
344-
// are serialized by _state_lock, so cycle N+1 always starts after
345-
// clearStandby() of cycle N completes.
414+
// StringDictionary makes no assumption about which callers are blocked.
415+
// In the Profiler context, three caller-side invariants reduce the
416+
// concurrency that phase 2 must handle:
417+
// - Signal paths: the caller (rotateDictsAndRun) holds a SignalBlocker
418+
// that masks SIGPROF/SIGVTALRM on the calling thread during rotate(),
419+
// so no profiler signal fires on this thread between Phase 1 and 2.
420+
// - JNI callers (e.g. recordTrace0): they bypass lockAll() and CAN
421+
// still insert into old_active after Phase 1. Phase 2's
422+
// waitForRefCountToClear(old_active) drains those in-flight inserts
423+
// before copying — that is the reason phase 2 exists.
424+
// - lookupDuringDump(): same thread as the rotate() caller — no
425+
// concurrency.
426+
// clearTarget() is the buffer that becomes the new active after rotate().
427+
// The caller is responsible for ensuring it is empty on entry (Profiler
428+
// achieves this by calling clearStandby() after every cycle and
429+
// serialising JFR operations with _state_lock).
346430
void rotate() {
347431
StringDictionaryBuffer* old_active = _rot.active();
348432
// Phase 1: pre-populate clearTarget from active (before rotate).
349433
_rot.clearTarget()->copyFrom(*old_active);
350434
_rot.rotate();
351435
// Drain all in-flight accessors on old_active (now the dump buffer).
352436
RefCountGuard::waitForRefCountToClear(old_active);
353-
// Phase 2: all insert paths are blocked, so old_active cannot have
354-
// gained new entries between phase 1 and phase 2.
437+
// Phase 2: catch any entries inserted into old_active between Phase 1
438+
// and the drain completing.
355439
_rot.active()->copyFrom(*old_active);
356440
}
357441

@@ -395,16 +479,12 @@ class StringDictionary {
395479
}
396480

397481
// Reset all three buffers and restart the ID counter.
398-
// Sets _accepting=false so that bounded_lookup callers that bypass lockAll()
399-
// (JNI paths) do not create new RefCountGuards after the caller's initial
400-
// waitForAllRefCountsToClear() drains. Drains again internally to catch any
401-
// guard that started between the caller's drain and this flag flip, then
402-
// clears, then re-enables lookups.
482+
// _accepting=false gates new RefCountGuard creation; the subsequent drain
483+
// ensures no concurrent accessor is mid-read when clear() zeroes the root
484+
// table. clear() is O(overflow_nodes + extra_arena_chunks); both are
485+
// typically zero for small-to-medium dictionaries.
403486
void clearAll() {
404487
_accepting.store(false, std::memory_order_seq_cst);
405-
// Drain guards that were already past the _accepting check when the flag
406-
// was set. After this returns, no new guards will be created (they all
407-
// see _accepting=false) so the clear below is safe.
408488
RefCountGuard::waitForAllRefCountsToClear();
409489
_a.clear(); _b.clear(); _c.clear();
410490
_rot.reset();

0 commit comments

Comments
 (0)